Hi Conor, > -----Original Message----- > From: Nuno Sá <noname.nuno@xxxxxxxxx> > Sent: Thursday, July 4, 2024 8:19 AM > To: Conor Dooley <conor@xxxxxxxxxx>; Agarwal, Utsav > <Utsav.Agarwal@xxxxxxxxxx> > Cc: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof > Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Sa, > Nuno <Nuno.Sa@xxxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Artamonovs, > Arturs <Arturs.Artamonovs@xxxxxxxxxx>; Bimpikas, Vasileios > <Vasileios.Bimpikas@xxxxxxxxxx>; Gaskell, Oliver > <Oliver.Gaskell@xxxxxxxxxx> > Subject: Re: [PATCH v5 3/3] dt-bindings: input: Update dtbinding for adp5588 > > [External] > > 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@xxxxxxxxxx> > > > > Sent: Wednesday, July 3, 2024 4:20 PM > > > > To: Agarwal, Utsav <Utsav.Agarwal@xxxxxxxxxx> > > > > Cc: Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>; Dmitry > Torokhov > > > > <dmitry.torokhov@xxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; > Krzysztof > > > > Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley > <conor+dt@xxxxxxxxxx>; Sa, > > > > Nuno <Nuno.Sa@xxxxxxxxxx>; linux-input@xxxxxxxxxxxxxxx; > > > > devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > Artamonovs, > > > > Arturs <Arturs.Artamonovs@xxxxxxxxxx>; Bimpikas, Vasileios > > > > <Vasileios.Bimpikas@xxxxxxxxxx>; Gaskell, Oliver > > > > <Oliver.Gaskell@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > > > > > > > > > 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@xxxxxxxxxx> > > > > > --- > > > > > .../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://urldefense.com/v3/__https://lore.kernel.org/all/d4661ddc1d253678f > d62be4c7e19eb0cff4174f6.camel@xxxxxxxxx/__;!!A3Ni8CS0y2Y!8MV0PPuz1 > XKzaNus-NtAS3oXtrcgaUWHi4sf7zsPFS5zH_NUaNz-qBqUXPBTbTE09wRTVa- > zYLaSdTte8ymnPyRB$ > > > > 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á I see the issue, thank you for highlighting the same. I will make the changes now. Thanks, Utsav
