Received-SPF: pass (google.com: domain of linux-kernel+bounces-127700-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240327220320.15509-1-l.rubusch@gmail.com> <20240327220320.15509-8-l.rubusch@gmail.com>
 <20240328133927.7e49f3bf@jic23-huawei> <CAFXKEHZva5AE=sinx_i0Gec2FbB4ZfyEu8mWg52omzGBvr5uXw@mail.gmail.com>
 <20240330152401.034aedad@jic23-huawei> <CAFXKEHZJZU69udvM86rDSNU1J9o=v62BJcJfTrB=Y9Tgsjm4fw@mail.gmail.com>
 <20240401175304.000022ed@Huawei.com>
In-Reply-To: <20240401175304.000022ed@Huawei.com>
From: Lothar Rubusch <l.rubusch@gmail.com>
Date: Tue, 2 Apr 2024 12:11:43 +0200
Message-ID: <CAFXKEHYrqA2iaQJeMqPbQJYEu10xiRt92c-ySGcfH76MSriufw@mail.gmail.com>
Subject: Re: [PATCH v5 7/7] iio: accel: adxl345: Add spi-3wire option
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jonathan Cameron <jic23@kernel.org>, lars@metafoo.de, Michael.Hennerich@analog.com, 
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, eraretuya@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 1, 2024 at 6:53=E2=80=AFPM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 1 Apr 2024 18:06:24 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Sat, Mar 30, 2024 at 4:24=E2=80=AFPM Jonathan Cameron <jic23@kernel.=
org> wrote:
> > >
> > > On Fri, 29 Mar 2024 01:33:01 +0100
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > On Thu, Mar 28, 2024 at 2:39=E2=80=AFPM Jonathan Cameron <jic23@ker=
nel.org> wrote:
> > > > >
> > > > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com>
> > > > > > ---
> > > > > >  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/ad=
xl345.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 r=
esolution */
> > > > > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/acce=
l/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_sp=
i_regmap_config =3D {
> > > > > >       .read_flag_mask =3D BIT(7) | BIT(6),
> > > > > >  };
> > > > > >
> > > > > > +static int adxl345_spi_setup(struct device *dev, struct regmap=
 *regmap)
> > > > > > +{
> > > > > > +     struct spi_device *spi =3D container_of(dev, struct spi_d=
evice, dev);
> > > > > > +
> > > > > > +     if (spi->mode & SPI_3WIRE)
> > > > > > +             return regmap_write(regmap, ADXL345_REG_DATA_FORM=
AT,
> > > > > > +                                 ADXL345_DATA_FORMAT_SPI_3WIRE=
);
> > > > > Your earlier patch carefully (I think) left one or two fields alo=
ne, then
> > > > > this write just comes in and changes them. In particular INT_INVE=
RT.
> > > > >
> > > > Why do you refer here to INT_INVERT? In this code above I try to se=
t
> > > > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specifi=
c,
> > > > 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 SP=
I
> > > > 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 drive=
r
> > > 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 almo=
st
> > > 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 alo=
ne.
> > >
> > 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 partic=
ular
> 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.

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?

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 :)

> Jonathan
>
>
> >
> > > >
> > > > > If it doesn't makes sense to write it there, either write that bi=
t
> > > > > every time here, or leave it alone every time.  Not decide on whe=
ther
> > > > > to write the bit based on SPI_3WIRE or not.  As far as I know the=
y
> > > > > 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 hand=
led
> > > > in the bus specific part, i.e. the adxl345_spi.c Therefore I config=
ure
> > > > SPI_3WIRE there. I don't want to place SPI specific code in the cor=
e
> > > > file.
> > >
> > > My confusion was that you were deliberately not touching the other un=
used
> > > 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 valu=
es
> > > 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_devic=
e *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 =3D {
> > > > >
> > >
> >
>