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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 7 Dec 2024 13:10:22 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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".

Easy. If they aren't both provided, no interrupts are used.
Driver carries on working bug less functionality.  That's fine.

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

We don't care.  The required list should be about requirements for the
hardware to function in a useful fashion, not if the driver currently supports
that mode.  So it should never have been required even if the driver at the
time required it because no one had done the work to make it work without.

In theory you could provide a default for interrupt-names I guess if
do want to be nice to the legacy driver.

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

Agreed no interrupts are required for device to do something useful.
(not sure I follow the rest of this entry).
> 
> 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.

Only way I can see to be nice about this is to specify a default.
However, if someone is using the input driver and we have interrupt names
that don't match the default, all bets are off.  That setup doesn't work
today anyway, so do we care? I don't think so.

So in conclusion. Drop the required entry for interrupts, but consider
if a default can work for interrupt-names, or whether we should add
the logic to require interrupt-names if interrupts are provided.

Jonathan

> 
> 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.