Re: [PATCH v5 06/10] dt-bindings: iio: accel: add interrupt-names

Lothar Rubusch
2024-12-07T12:10:59Z

On Fri, Dec 6, 2024 at 6:29 PM Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> On Fri, Dec 6, 2024 at 6:08 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 05, 2024 at 08:41:52PM +0100, Lothar Rubusch wrote:
> > > On Thu, Dec 5, 2024 at 6:54 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Thu, Dec 05, 2024 at 05:13:39PM +0000, Lothar Rubusch wrote:
> > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > sensor. Only one line will be connected for incoming events. The driver
> > > > > needs to be configured accordingly. If no interrupt line is set up, the
> > > > > sensor will still measure, but no events are possible.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > > .../devicetree/bindings/iio/accel/adi,adxl345.yaml | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > index 280ed479ef5..67e2c029a6c 100644
> > > > > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > > > > @@ -37,6 +37,11 @@ properties:
> > > > > interrupts:
> > > > > maxItems: 1
> > > > >
> > > > > + interrupt-names:
> > > > > + description: Use either INT1 or INT2 for events, or ignore events.
> > > > > + items:
> > > > > + - enum: [INT1, INT2]
> > > >
> > > > The description for this ", or ignore events" does not make sense. Just
> > > > drop it, it's clear what happens if you don't provide interrupts.
> > > >
> > > > However, interrupts is a required property but interrupt-names is not.
> > > > Seems rather pointless not making interrupt-names a required property
> > > > (in the binding!) since if you only add interrupts and not
> > > > interrupt-names you can't even use the interrupt as you do not know
> > > > whether or not it is INT1 or INT2?
> > >
> > > What I meant is, yes, the sensor needs an interrupt line.
> > > Interrupt-names is optional. The sensor always can measure. When
> > > interrupt-names is specified, though, the sensor will setup a FIFO and
> > > can use events, such as data ready, watermark, single tap, freefall,
> > > etc. Without the interrupt-names, the sensor goes into a "FIFO bypass
> > > mode" without its specific events.
> >
> > What I'm talking about here is how it is ultimately pointless for
> > interrupts to be a required property if it can never be used without
> > interrupt-names as you cannot know which interrupt is in use. I think
> > both should be made mandatory or neither.
> >
>
> Ah, now I can see your point. I agree that it should be equally
> mandatory as the interrupt. Legacy implementations used simply always
> just INT1. I'd like to make it configurable in the IIO driver but
> tried to avoid the DT topic for now (which was not a smart decision
> either). Hence, I added the interrupt-names.
> I'm unsure should I make "interrupt-names" a required property now?
> What about the existing DTS files using this sensor? There are no
> interrupt-names specified, so if made required, the missing
> interrupt-names there would break binding check, or not?
>

Sorry, I have to clarify myself, yesterday I was not focussed..

1. I agree this is kind of half way. Either, both are required or none of them.
If both were required, also the older DTS files using the ADXL345 would
need to be "fixed". If I add interrupt-names, it works with my patches for the
"newer" IIO driver, because since I implement it it's using interrupt-names.
The older input driver for that using interrupt, does not use interrupt-names.
Hence, it requires the interrupt in the DT. But it does not require
interrupt-names
(historical stuff).

2. AFAIK the sensor can operate w/o interrupts.
A) w/o INT line: measuring is possible; FIFO bypassed; no events
B) w/ INT line: measuring is possible; can use FIFO; events are possible
When setting the interrupt in DT, the interrupt line name can/could be
configured also via SW (setting up the registers of the sensor). So, it's not
impossible. This is AFAIR the approach in the legacy input driver. Now, there
is devicetree, and both should probably be better configured somewhere in
the DT

3. IMHO neither one, not the interrupt, nor the interrupt-names need to be
a required DT-binding.
If interrupt is required and interrupt-names not, it's a half-way approach,
which leaves specifying the IRQ line open to be solved partly in DT
(declaration of the interrupt) and partly in SW (configuration of the
interrupt line to use), e.g. hardcoded or configurable somewhere in the
driver via sysfs or the like. Not nice.

Pls, let me know what you think, and in case, if I need to take some
action, here.
Best,
L

> > > Hence, I better drop the description entirely, since it rather seems
> > > to be confusing.