linux-kernel - Re: [PATCH v3 04/18] iio: accel: bma220: split original driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAHp75VdWQojOj2MLS4dOvMKjSGcAunc4ND9SPsGrZBBctPdQgQ@mail.gmail.com>
Date: Sun, 14 Sep 2025 15:45:07 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, 
	Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 04/18] iio: accel: bma220: split original driver

On Sat, Sep 13, 2025 at 6:40 PM Petre Rodan <petre.rodan@...dimension.ro> wrote:
>
> In preparation for the i2c module, move the original code into multiple
> source files without any other functional change.
>
> Create the additional bma220_core module which currently is not
> providing an abstracted bus type (this will change with the regmap
> patch).
> Fix checkpatch warning about GPL v2 license in bma220_spi.c.

Wny not a prerequisite fix?

> Fix a few includes to make the change functional and prepare for the
> next patches.

Ditto?

...

>           To compile this driver as a module, choose M here: the
> -         module will be called bma220_spi.
> +         module will be called bma220_core and you will also get
> +         bma220_spi if SPI is enabled.

I'm not sure the last part is practical, as one needs to list all
buses next time a new one will be added.

...

> +++ b/drivers/iio/accel/bma220.h

> +#ifndef _BMA220_H
> +#define _BMA220_H
> +
> +#include <linux/pm.h>
> +
> +extern const struct dev_pm_ops bma220_pm_ops;

> +struct spi_device;

Besides the location of this (I would expect it to follow up include
linux/*) convert the existing driver to regmap first and remove this
unneeded churn.

> +int bma220_common_probe(struct spi_device *dev);
> +
> +#endif

...

> +#include <linux/bits.h>

> +#include <linux/kernel.h>

You should not use this header.

> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/types.h>
> +#include <linux/spi/spi.h>

...

> +       /**

Do not inherit problems from the past.

> +        * The chip can be suspended/woken up by a simple register read.
> +        * So, we need up to 2 register reads of the suspend register
> +        * to make sure that the device is in the desired state.
> +        */

...

I haven't reviewed the rest as I believe it's just ~1:1 copy of the
existing code, but I still think that the result will be better if
this series starts from small fixes, like kernel doc, and other
things, followed by the regmap conversion and only _after_ the split
is made.


--
With Best Regards,
Andy Shevchenko