Re: [PATCH v5 3/3] dt-bindings: input: Update dtbinding for adp5588

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2024-07-03 at 16:57 +0100, Conor Dooley wrote:
> On Wed, Jul 03, 2024 at 03:55:11PM +0000, Agarwal, Utsav wrote:
> > Hi Conor,
> > 
> > Thank you for your feedback.
> > > -----Original Message-----
> > > From: Conor Dooley <conor@kernel.org>
> > > Sent: Wednesday, July 3, 2024 4:20 PM
> > > To: Agarwal, Utsav <Utsav.Agarwal@analog.com>
> > > Cc: Hennerich, Michael <Michael.Hennerich@analog.com>; Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com>; Rob Herring <robh@kernel.org>; Krzysztof
> > > Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Sa,
> > > Nuno <Nuno.Sa@analog.com>; linux-input@vger.kernel.org;
> > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Artamonovs,
> > > Arturs <Arturs.Artamonovs@analog.com>; Bimpikas, Vasileios
> > > <Vasileios.Bimpikas@analog.com>; Gaskell, Oliver
> > > <Oliver.Gaskell@analog.com>
> > > Subject: Re: [PATCH v5 3/3] dt-bindings: input: Update dtbinding for
> > > adp5588
> > > 
> > > On Wed, Jul 03, 2024 at 11:58:16AM +0100, Utsav Agarwal via B4 Relay
> > > wrote:
> > > > From: Utsav Agarwal <utsav.agarwal@analog.com>
> > > > 
> > > > Updating dt bindings for adp5588. Since the device can now function in a
> > > > purely gpio mode, the following keypad specific properties are now made
> > > > optional:
> > > > 	- interrupts
> > > > 	- keypad,num-rows
> > > > 	- keypad,num-columns
> > > > 	- linux,keymap
> > > > 
> > > > However since the above properties are required to be specified when
> > > > configuring the device as a keypad, dependencies have been added
> > > > such that specifying either one would require the remaining as well.
> > > > 
> > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com>
> > > > ---
> > > >  .../devicetree/bindings/input/adi,adp5588.yaml     | 33
> > > ++++++++++++++++++----
> > > >  1 file changed, 28 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > > b/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > > > index 26ea66834ae2..6c06464f822b 100644
> > > > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > > > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > > > @@ -49,7 +49,10 @@ properties:
> > > >    interrupt-controller:
> > > >      description:
> > > >        This property applies if either keypad,num-rows lower than 8 or
> > > > -      keypad,num-columns lower than 10.
> > > > +      keypad,num-columns lower than 10. This property does not apply if
> > > > +      keypad,num-rows or keypad,num-columns are not specified since the
> > > > +      device then acts as gpio only, during which interrupts are not
> > > > +      utilized.
> > > > 
> > > >    '#interrupt-cells':
> > > >      const: 2
> > > > @@ -65,13 +68,15 @@ properties:
> > > >      minItems: 1
> > > >      maxItems: 2
> > > > 
> > > > +dependencies:
> > > > +  keypad,num-rows: ["keypad,num-columns"]
> > > > +  keypad,num-cols: ["keypad,num-rows"]
> > > > +  linux,keymap: ["keypad,num-rows"]
> > > 
> > > Is what you've got here sufficient? Adding "keypad,num-rows" won't
> > > mandate "linux,keymap" which I think is wrong. I think all 3 entries
> > > here need to contain both of the other two.
> > > 
> > 
> > Ah, I can see the issue, thank you for pointing it out - I will be
> > correcting that.
> > 
> > > > +  interrupts: ["linux,keymap"]
> > > 
> > > I still don't understand why interrupts are only allowed when the keymap
> > > is present. I'd cover the interrupts with something like
> > > 
> > > if:
> > >   required:
> > >     - linux,keymap
> > >   then:
> > >     required:
> > >       - interrupts
> > > 
> > > so that interrupts can be used while not in keypad mode. Unless of
> > > course there's something (unmentioned in this patch) that prevents that.
> > 
> > In case when the device is not in keypad mode, i.e, is purely using gpio -
> > it doesn't trigger the interrupt.
> > Due to this, I had restricted the same to keypad mode only(as a
> > requirement). This was mentioned 
> > here:
> > https://lore.kernel.org/all/d4661ddc1d253678fd62be4c7e19eb0cff4174f6.camel@gmail.com/
> 
> This says "not required", not "not functional". How come generating
> interrupts becomes impossible when not in keypad mode? That's what needs
> to be explained.
> 

I should have read the patch before replying in v4. Yes, I agree with Conor that
what we need is to make the interrupt __required__ when using the keypad. In the
case we have gpios, it's optional but it does not mean we should remove it. One
usecase would be to use gpios still as inputs through gpio-keys.

- Nuno Sá