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

[Jonathan Cameron]

On Tue, 2 Apr 2024 12:11:43 +0200
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> On Mon, Apr 1, 2024 at 6:53 PM Jonathan Cameron
> <Jonathan.Cameron@xxxxxxxxxx> wrote:
> >
> > On Mon, 1 Apr 2024 18:06:24 +0200
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >  
> > > On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:  
> > > >
> > > > 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.
> > > >  
> > > Going by the git history gave this impression, too. But it was still a
> > > bit confusing to me.
> > >
> > > The IIO driver so far does not handle any of the interrupt features.
> > > The older driver also seems to support more of the chip's features.
> > > Would it make sense to continue implement/port what's missing -
> > > feature by feature - for the IIO driver in order to make the input
> > > driver obsolete (one day)?  
> >
> > Yes, though it will be challenging because of the ABI differences.
> > We do have a few cross subsystem bridge drivers, but the few goes we've
> > had at an accel bridge driver haven't made it into the kernel.  In particular
> > we don't have an in kernel interface for threshold events and similar in IIO.
> > It would be easy enough to add one, but no one has ever cared enough to
> > do the work. :(
> >  
> Perhaps there are easier things (precision, power saving measures,
> etc) of that particular accelerometer to port first. Open issues which
> even I could give a try here. Sounds really exciting to me, though,
> but before I definitely need a bit more experience with community and
> APIs.

That would be great.

> 
> What do you mean with cross subsystem bridge drivers, or this accel
> bridge driver? I mean, can you provide me with a link to that driver
> to have a look into what direction that is going to?
The last go at this was probably this one.

https://lore.kernel.org/linux-iio/d52cf9ee5944c90c69f6e74a46d844cef51e487e.1555362312.git.hns@xxxxxxxxxxxxx/

> 
> Anyway, I really appreciate already your patience with my patches, the
> direct and helpful answers! I appreciate the support. Thinking and
> re-thinking over every particular line of code is really an
> experience. I don't want to go too much into off-topic discussions
> here, if this is not the forum for that, please let me know :)

People used to sometimes be on IRC but I'm not sure anyone is any more :(
I rarely joined because of network restrictions at work.

It's fine to ask questions on this mailing list though.
Nature of kernel development is that people need to get good at skipping
threads they aren't interested in!

Jonathan




> 
> > Jonathan
> >
> >  
> > >  
> > > > >  
> > > > > > 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 = {  
> > > > > >  
> > > >  
> > >  
> >