| Message ID | 20240703-adp5588_gpio_support-v5-3-49fcead0d390@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adp5588-keys: Support for dedicated gpio operation | expand |
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. > + 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. Thanks, Conor. > + > required: > - compatible > - reg > - - interrupts > - - keypad,num-rows > - - keypad,num-columns > - - linux,keymap > > unevaluatedProperties: false > > @@ -108,4 +113,22 @@ examples: > >; > }; > }; > + > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/input/input.h> > + #include <dt-bindings/gpio/gpio.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + gpio@34 { > + compatible = "adi,adp5588"; > + reg = <0x34>; > + > + #gpio-cells = <2>; > + gpio-controller; > + }; > + }; > + > ... > > -- > 2.34.1 > >
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/ Moreover, this was also included in one of the earlier feedback comments that I had received: https://lore.kernel.org/all/1fc545709ad2aee0b70a8d1c561516d94cb6fb1e.camel@gmail.com/ Thanks, Utsav
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.
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á
Hi Conor, > -----Original Message----- > From: Nuno Sá <noname.nuno@gmail.com> > Sent: Thursday, July 4, 2024 8:19 AM > To: Conor Dooley <conor@kernel.org>; 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 > > [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@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://urldefense.com/v3/__https://lore.kernel.org/all/d4661ddc1d253678f > d62be4c7e19eb0cff4174f6.camel@gmail.com/__;!!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
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"] + interrupts: ["linux,keymap"] + required: - compatible - reg - - interrupts - - keypad,num-rows - - keypad,num-columns - - linux,keymap unevaluatedProperties: false @@ -108,4 +113,22 @@ examples: >; }; }; + + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/input/input.h> + #include <dt-bindings/gpio/gpio.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + gpio@34 { + compatible = "adi,adp5588"; + reg = <0x34>; + + #gpio-cells = <2>; + gpio-controller; + }; + }; + ...