Re: [PATCH v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Fri, 29 Mar 2024 01:33:01 +0100
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:20 +0000
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >  
> > > Add a setup function implementation to the spi module to enable spi-3wire
> > > as option when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > > ---
> > >  drivers/iio/accel/adxl345.h     |  2 ++
> > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 4ea9341d4..e6bc3591c 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -30,6 +30,8 @@
> > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > >
> > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > +
> > >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0) /* Set the g range */
> > >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2) /* Left-justified (MSB) mode */
> > >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > > index 1c0513bd3..f145d5c1d 100644
> > > --- a/drivers/iio/accel/adxl345_spi.c
> > > +++ b/drivers/iio/accel/adxl345_spi.c
> > > @@ -20,6 +20,16 @@ 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);
> > > +
> > > +     if (spi->mode & SPI_3WIRE)
> > > +             return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > > +                                 ADXL345_DATA_FORMAT_SPI_3WIRE);  
> > Your earlier patch carefully (I think) left one or two fields alone, then
> > this write just comes in and changes them. In particular INT_INVERT.
> >  
> Why do you refer here to INT_INVERT? In this code above I try to set
> SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> it happens in adxl345_spi.c. Passing this function to the bus
> independent adxl345_core.c file allows to configure the bus first.
> Therefore, I'm using the update function in core masking out the SPI
> filag.

Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
other bits that you also masked out.  I thought intent was to leave
them untouched for some reason.  Given they don't matter in the driver
at the moment (no interrupt support) then no problem.

> 
> My reason why I try to keep INT_INVERT out is different. There is
> another driver for the same part in the kernel:
> ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> interrupts of the adxl345 for the input device implementation. I
> assumed, that in the iio implementation there won't be interrupt
> handling for the adx345, since it is not an input device. Does this
> make sense?

No. You can't use these two drivers at the same time.  They will almost
certainly trample over each other in actually reading channels etc.

Their is some legacy of old drivers in input from a long time back.
Given this driver clearly doesn't have full functionality yet in IIO there
and the different userspace ABI, we've just left the input driver alone.

> 
> > If it doesn't makes sense to write it there, either write that bit
> > every time here, or leave it alone every time.  Not decide on whether
> > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > are unconnected features.
> >  
> I think I did not understand. Could you please specify a bit more?
> When spi-3wire is configured in the DT it has to be parsed and handled
> in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> SPI_3WIRE there. I don't want to place SPI specific code in the core
> file.

My confusion was that you were deliberately not touching the other unused
flags.  In reality you are touching the but only if you enable 3wire.
I would write them register to 0 in the !3wire case so all other values
are the same in both paths.

> 
> > > +     return 0;
> > > +}
> > > +
> > >  static int adxl345_spi_probe(struct spi_device *spi)
> > >  {
> > >       struct regmap *regmap;
> > > @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > >       if (IS_ERR(regmap))
> > >               return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> > >
> > > -     return adxl345_core_probe(&spi->dev, regmap, NULL);
> > > +     return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > >  }
> > >
> > >  static const struct adxl345_chip_info adxl345_spi_info = {  
> >