| Message ID | 20240701-adp5588_gpio_support-v4-2-44bba0445e90@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | adp5588-keys: Support for dedicated gpio operation | expand |
On Mon, Jul 01, 2024 at 04:04:51PM +0100, Utsav Agarwal via B4 Relay wrote: > From: Utsav Agarwal <utsav.agarwal@analog.com> > > Updating dt bindings for adp5588. Following properties are now made > optional: > - interrupts > - keypad,num-rows > - keypad,num-columns > - linux,keymap > The proposed new property "gpio-only" has been added as an optional > property with an additional example. I can see that as it is clear in the diff, but this doesn't explain why, which is what you need to do in your commit message. > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > --- > .../devicetree/bindings/input/adi,adp5588.yaml | 28 ++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > index 26ea66834ae2..158fbf02cc16 100644 > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > @@ -46,6 +46,11 @@ properties: > '#gpio-cells': > const: 2 > > + gpio-only: > + description: > + This property applies if keypad,num-rows, keypad,num-columns and > + linux,keypad are not specified. All keys will be marked as gpio. Why is a property required for this? Is the absence of the 3 keypad properties not sufficient to determine that you're in this mode? > interrupt-controller: > description: > This property applies if either keypad,num-rows lower than 8 or > @@ -68,10 +73,6 @@ properties: > required: > - compatible > - reg > - - interrupts I don't understand why interrupts is no longer required. > - - keypad,num-rows > - - keypad,num-columns > - - linux,keymap I think you should configure "dependencies:" such that if one of these properties is added, then all 3 of them must be to preserve the current requirements while the device is being used in keypad mode. Thanks, Conor. > > unevaluatedProperties: false > > @@ -108,4 +109,23 @@ 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; > + gpio-only; > + }; > + }; > + > ... > > -- > 2.34.1 > >
On Mon, 01 Jul 2024 16:04:51 +0100, Utsav Agarwal wrote: > Updating dt bindings for adp5588. Following properties are now made > optional: > - interrupts > - keypad,num-rows > - keypad,num-columns > - linux,keymap > The proposed new property "gpio-only" has been added as an optional > property with an additional example. > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > --- > .../devicetree/bindings/input/adi,adp5588.yaml | 28 ++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adi,adp5588.yaml: gpio-only: missing type definition doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240701-adp5588_gpio_support-v4-2-44bba0445e90@analog.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Mon, Jul 01, 2024 at 04:46:12PM +0100, Conor Dooley wrote: > On Mon, Jul 01, 2024 at 04:04:51PM +0100, Utsav Agarwal via B4 Relay wrote: > > From: Utsav Agarwal <utsav.agarwal@analog.com> > > > > Updating dt bindings for adp5588. Following properties are now made > > optional: > > - interrupts > > - keypad,num-rows > > - keypad,num-columns > > - linux,keymap > > The proposed new property "gpio-only" has been added as an optional > > property with an additional example. > > I can see that as it is clear in the diff, but this doesn't explain why, > which is what you need to do in your commit message. > > > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > > --- > > .../devicetree/bindings/input/adi,adp5588.yaml | 28 ++++++++++++++++++---- > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > index 26ea66834ae2..158fbf02cc16 100644 > > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > @@ -46,6 +46,11 @@ properties: > > '#gpio-cells': > > const: 2 > > > > + gpio-only: > > + description: > > + This property applies if keypad,num-rows, keypad,num-columns and > > + linux,keypad are not specified. All keys will be marked as gpio. > > Why is a property required for this? Is the absence of the 3 keypad > properties not sufficient to determine that you're in this mode? Yes, I think it should be enough. > > > > interrupt-controller: > > description: > > This property applies if either keypad,num-rows lower than 8 or > > @@ -68,10 +73,6 @@ properties: > > required: > > - compatible > > - reg > > - - interrupts > > I don't understand why interrupts is no longer required. I think it should be possible to use this chip as a GPIO controller but not an interrupt controller, in which case one does not have to wire up the interrupt line from it. However this requires much more elaborate binding description (i.e. no keys and no "interrupt-controller" property). Thanks.
Hi Utsav,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 1c52cf5e79d30ac996f34b64284f2c317004d641]
url: https://github.com/intel-lab-lkp/linux/commits/Utsav-Agarwal-via-B4-Relay/Input-adp5588-keys-add-support-for-pure-gpio/20240701-234752
base: 1c52cf5e79d30ac996f34b64284f2c317004d641
patch link: https://lore.kernel.org/r/20240701-adp5588_gpio_support-v4-2-44bba0445e90%40analog.com
patch subject: [PATCH v4 2/2] dt-bindings: input: Update dtbinding for adp5588
config: arc-randconfig-051-20240702
compiler: arc-elf-gcc (GCC) 13.2.0
dtschema version: 2024.6.dev3+g650bf2d
reproduce (this is a W=1 build):
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407020347.kJZwFaOT-lkp@intel.com/
dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/input/adi,adp5588.yaml: gpio-only: missing type definition
Hi Rob, > -----Original Message----- > From: Rob Herring (Arm) <robh@kernel.org> > Sent: Monday, July 1, 2024 5:28 PM > To: Agarwal, Utsav <Utsav.Agarwal@analog.com> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>; linux-kernel@vger.kernel.org; > Hennerich, Michael <Michael.Hennerich@analog.com>; > devicetree@vger.kernel.org; Artamonovs, Arturs > <Arturs.Artamonovs@analog.com>; Sa, Nuno <Nuno.Sa@analog.com>; > Dmitry Torokhov <dmitry.torokhov@gmail.com>; Bimpikas, Vasileios > <Vasileios.Bimpikas@analog.com>; Conor Dooley <conor+dt@kernel.org>; > Gaskell, Oliver <Oliver.Gaskell@analog.com>; linux-input@vger.kernel.org > Subject: Re: [PATCH v4 2/2] dt-bindings: input: Update dtbinding for adp5588 > > [External] > > > On Mon, 01 Jul 2024 16:04:51 +0100, Utsav Agarwal wrote: > > Updating dt bindings for adp5588. Following properties are now made > > optional: > > - interrupts > > - keypad,num-rows > > - keypad,num-columns > > - linux,keymap > > The proposed new property "gpio-only" has been added as an optional > > property with an additional example. > > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > > --- > > .../devicetree/bindings/input/adi,adp5588.yaml | 28 > ++++++++++++++++++---- > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review- > ci/linux/Documentation/devicetree/bindings/input/adi,adp5588.yaml: gpio- > only: missing type definition Thank you for the feedback, I will fix this and confirm with dt_binding_check before resubmitting the next version. > > doc reference errors (make refcheckdocs): > > See > https://urldefense.com/v3/__https://patchwork.ozlabs.org/project/devicetr > ee-bindings/patch/20240701-adp5588_gpio_support-v4-2- > 44bba0445e90@analog.com__;!!A3Ni8CS0y2Y!6DaQ5a3iw3kzzWwCpDz9K5X > ABnIHXNsXMTyaGAc53YJ1gYBgSwKyiPMSl620Xo2okCIQ1P1ftU0rJkD5$ > > The base for the series is generally the latest rc1. A different dependency > should be noted in *this* patch. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade > > Please check and re-submit after running the above command yourself. Note > that DT_SCHEMA_FILES can be set to your schema file to speed up checking > your schema. However, it must be unset to test all examples with your > schema.
Hi Connor, > -----Original Message----- > From: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Sent: Monday, July 1, 2024 6:46 PM > To: Conor Dooley <conor@kernel.org> > Cc: Agarwal, Utsav <Utsav.Agarwal@analog.com>; Hennerich, Michael > <Michael.Hennerich@analog.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 v4 2/2] dt-bindings: input: Update dtbinding for adp5588 > > [External] > > On Mon, Jul 01, 2024 at 04:46:12PM +0100, Conor Dooley wrote: > > On Mon, Jul 01, 2024 at 04:04:51PM +0100, Utsav Agarwal via B4 Relay > wrote: > > > From: Utsav Agarwal <utsav.agarwal@analog.com> > > > > > > Updating dt bindings for adp5588. Following properties are now made > > > optional: > > > - interrupts > > > - keypad,num-rows > > > - keypad,num-columns > > > - linux,keymap > > > The proposed new property "gpio-only" has been added as an optional > > > property with an additional example. > > > > I can see that as it is clear in the diff, but this doesn't explain why, > > which is what you need to do in your commit message. > > I will add more description to this commit message for context. > > > > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > > > --- > > > .../devicetree/bindings/input/adi,adp5588.yaml | 28 > ++++++++++++++++++---- > > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > index 26ea66834ae2..158fbf02cc16 100644 > > > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > @@ -46,6 +46,11 @@ properties: > > > '#gpio-cells': > > > const: 2 > > > > > > + gpio-only: > > > + description: > > > + This property applies if keypad,num-rows, keypad,num-columns and > > > + linux,keypad are not specified. All keys will be marked as gpio. > > > > Why is a property required for this? Is the absence of the 3 keypad > > properties not sufficient to determine that you're in this mode? > > Yes, I think it should be enough. The idea behind introducing a new property was to simplify the usage in addition to making it easier to document a pure gpio mode being supported. Would it still be better to remove this? > > > > > > > > interrupt-controller: > > > description: > > > This property applies if either keypad,num-rows lower than 8 or > > > @@ -68,10 +73,6 @@ properties: > > > required: > > > - compatible > > > - reg > > > - - interrupts > > > > I don't understand why interrupts is no longer required. > > I think it should be possible to use this chip as a GPIO controller but > not an interrupt controller, in which case one does not have to wire up > the interrupt line from it. However this requires much more elaborate > binding description (i.e. no keys and no "interrupt-controller" > property). I will add a more detailed description in the binding. > > Thanks. > > -- > Dmitry Thanks, Utsav
On Mon, Jul 01, 2024 at 10:46:09AM -0700, Dmitry Torokhov wrote: > On Mon, Jul 01, 2024 at 04:46:12PM +0100, Conor Dooley wrote: > > On Mon, Jul 01, 2024 at 04:04:51PM +0100, Utsav Agarwal via B4 Relay wrote: > > > From: Utsav Agarwal <utsav.agarwal@analog.com> > > > > > > Updating dt bindings for adp5588. Following properties are now made > > > optional: > > > - interrupts > > > - keypad,num-rows > > > - keypad,num-columns > > > - linux,keymap > > > The proposed new property "gpio-only" has been added as an optional > > > property with an additional example. > > > > I can see that as it is clear in the diff, but this doesn't explain why, > > which is what you need to do in your commit message. > > > > > > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > > > --- > > > .../devicetree/bindings/input/adi,adp5588.yaml | 28 ++++++++++++++++++---- > > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > index 26ea66834ae2..158fbf02cc16 100644 > > > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > @@ -46,6 +46,11 @@ properties: > > > '#gpio-cells': > > > const: 2 > > > > > > + gpio-only: > > > + description: > > > + This property applies if keypad,num-rows, keypad,num-columns and > > > + linux,keypad are not specified. All keys will be marked as gpio. > > > > Why is a property required for this? Is the absence of the 3 keypad > > properties not sufficient to determine that you're in this mode? > > Yes, I think it should be enough. > > > > > > > > interrupt-controller: > > > description: > > > This property applies if either keypad,num-rows lower than 8 or > > > @@ -68,10 +73,6 @@ properties: > > > required: > > > - compatible > > > - reg > > > - - interrupts > > > > I don't understand why interrupts is no longer required. > > I think it should be possible to use this chip as a GPIO controller but > not an interrupt controller, in which case one does not have to wire up > the interrupt line from it. However this requires much more elaborate > binding description (i.e. no keys and no "interrupt-controller" > property). Aye. I can totally understand why you might want to make the interrupt portion optional - but it seems unrelated to the rest of the changes in the patch (use as a keypad without interrupts could be possible, right?) and is unexplained. Cheers, Conor.
On Tue, 2024-07-02 at 16:42 +0100, Conor Dooley wrote: > On Mon, Jul 01, 2024 at 10:46:09AM -0700, Dmitry Torokhov wrote: > > On Mon, Jul 01, 2024 at 04:46:12PM +0100, Conor Dooley wrote: > > > On Mon, Jul 01, 2024 at 04:04:51PM +0100, Utsav Agarwal via B4 Relay > > > wrote: > > > > From: Utsav Agarwal <utsav.agarwal@analog.com> > > > > > > > > Updating dt bindings for adp5588. Following properties are now made > > > > optional: > > > > - interrupts > > > > - keypad,num-rows > > > > - keypad,num-columns > > > > - linux,keymap > > > > The proposed new property "gpio-only" has been added as an optional > > > > property with an additional example. > > > > > > I can see that as it is clear in the diff, but this doesn't explain why, > > > which is what you need to do in your commit message. > > > > > > > > > > > Signed-off-by: Utsav Agarwal <utsav.agarwal@analog.com> > > > > --- > > > > .../devicetree/bindings/input/adi,adp5588.yaml | 28 > > > > ++++++++++++++++++---- > > > > 1 file changed, 24 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > > b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > > index 26ea66834ae2..158fbf02cc16 100644 > > > > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml > > > > @@ -46,6 +46,11 @@ properties: > > > > '#gpio-cells': > > > > const: 2 > > > > > > > > + gpio-only: > > > > + description: > > > > + This property applies if keypad,num-rows, keypad,num-columns and > > > > + linux,keypad are not specified. All keys will be marked as gpio. > > > > > > Why is a property required for this? Is the absence of the 3 keypad > > > properties not sufficient to determine that you're in this mode? > > > > Yes, I think it should be enough. > > > > > > > > > > > > interrupt-controller: > > > > description: > > > > This property applies if either keypad,num-rows lower than 8 or > > > > @@ -68,10 +73,6 @@ properties: > > > > required: > > > > - compatible > > > > - reg > > > > - - interrupts > > > > > > I don't understand why interrupts is no longer required. > > > > I think it should be possible to use this chip as a GPIO controller but > > not an interrupt controller, in which case one does not have to wire up > > the interrupt line from it. However this requires much more elaborate > > binding description (i.e. no keys and no "interrupt-controller" > > property). > > Aye. I can totally understand why you might want to make the interrupt > portion optional - but it seems unrelated to the rest of the changes in > the patch (use as a keypad without interrupts could be possible, right?) > and is unexplained. > IMO, it is related as it's the new usecase (of only using the gpios) that trigger the interrupt not being required anymore. No, I don't think we can use the keypad without the interrupt line. I guess (as you suggested before) we should check if one of the rows/columns property is present and in that case still make 'interrupts' required. Agree it should be better explained. - Nuno Sá >
diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml b/Documentation/devicetree/bindings/input/adi,adp5588.yaml index 26ea66834ae2..158fbf02cc16 100644 --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml @@ -46,6 +46,11 @@ properties: '#gpio-cells': const: 2 + gpio-only: + description: + This property applies if keypad,num-rows, keypad,num-columns and + linux,keypad are not specified. All keys will be marked as gpio. + interrupt-controller: description: This property applies if either keypad,num-rows lower than 8 or @@ -68,10 +73,6 @@ properties: required: - compatible - reg - - interrupts - - keypad,num-rows - - keypad,num-columns - - linux,keymap unevaluatedProperties: false @@ -108,4 +109,23 @@ 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; + gpio-only; + }; + }; + ...