On Wed, Mar 20, 2024 at 11:30 AM Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
>
> On Tue, 2024-03-19 at 21:27 +0000, Lothar Rubusch wrote:
> > Adds the spi-3wire feature and adds general refactoring to the
> > iio driver.
> >
> > The patch moves driver wide constants and fields into the
> > header. Thereby reduces redundant info struct definitions.
> > Allows to pass a function pointer from SPI/I2C specific probe,
> > and smaller refactorings. A regmap_update_bits() in the core
> > file replaces the regmap_write() to format_data.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
>
> On top of what Krzysztof already said I would also like for you to split the
> spi-3wire (which is adding a new feature) from the refactor in two different
> patches. One more comment inline...
Agree
> > drivers/iio/accel/adxl345.h | 44 +++++++++++-
> > drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++--------------
> > drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> > drivers/iio/accel/adxl345_spi.c | 50 ++++++++-----
> > 4 files changed, 153 insertions(+), 87 deletions(-)
> >
>
> ...
>
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > index 93ca349f1..e456b61c6 100644
> > --- a/drivers/iio/accel/adxl345_spi.c
> > +++ b/drivers/iio/accel/adxl345_spi.c
> > @@ -20,48 +20,62 @@ static const struct regmap_config
> > adxl345_spi_regmap_config = {
> > .read_flag_mask = BIT(7) | BIT(6),
> > };
> >
> > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> > +{
> > + struct spi_device *spi = container_of(dev, struct spi_device, dev);
> > + int ret;
> > +
> > + if (spi->mode & SPI_3WIRE) {
> > + ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > + ADXL345_DATA_FORMAT_SPI);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
>
> I think this would be neater:
>
> if (!(spi->mode & SPI_3WIRE))
> return 0;
>
> return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> ADXL345_DATA_FORMAT_SPI);
Definitely. Thank you.
Best,
L
