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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4153 bytes --]

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.

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]