linux-kernel - RE: [PATCH v9 3/3] dt-bindings: input: Update dtbinding for adp5588

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <SJ0PR03MB6343E06A1009171EB4E9AD9F9BBF2@SJ0PR03MB6343.namprd03.prod.outlook.com>
Date: Tue, 6 Aug 2024 10:05:04 +0000
From: "Agarwal, Utsav" <Utsav.Agarwal@...log.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
        "Hennerich, Michael"
	<Michael.Hennerich@...log.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, "Sa, Nuno" <Nuno.Sa@...log.com>
CC: "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Artamonovs,
 Arturs" <Arturs.Artamonovs@...log.com>,
        "Bimpikas, Vasileios"
	<Vasileios.Bimpikas@...log.com>,
        "Gaskell, Oliver"
	<Oliver.Gaskell@...log.com>
Subject: RE: [PATCH v9 3/3] dt-bindings: input: Update dtbinding for adp5588



> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@...nel.org>
> Sent: Tuesday, August 6, 2024 10:04 AM
> To: Agarwal, Utsav <Utsav.Agarwal@...log.com>; Hennerich, Michael
> <Michael.Hennerich@...log.com>; Dmitry Torokhov
> <dmitry.torokhov@...il.com>; Rob Herring <robh@...nel.org>; Krzysztof
> Kozlowski <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Sa,
> Nuno <Nuno.Sa@...log.com>
> Cc: linux-input@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; Artamonovs, Arturs
> <Arturs.Artamonovs@...log.com>; Bimpikas, Vasileios
> <Vasileios.Bimpikas@...log.com>; Gaskell, Oliver
> <Oliver.Gaskell@...log.com>
> Subject: Re: [PATCH v9 3/3] dt-bindings: input: Update dtbinding for adp5588
> 
> [External]
> 
> On 06/08/2024 10:48, Utsav Agarwal via B4 Relay wrote:
> > From: Utsav Agarwal <utsav.agarwal@...log.com>
> >
> 
> A nit, subject: drop second/last, redundant "dtbinding". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v6.7-
> rc8/source/Documentation/devicetree/bindings/submitting-
> patches.rst*L18__;Iw!!A3Ni8CS0y2Y!_px-
> SkCaAOezCa_s0eiP5BTliLmvyA110d4MXahf8mHkAXr0vJtpM0EXu7EsoG2jWg
> 5qW9FgaYf2gCZf$
> 
> Subject: everything is an update. Be descriptive and specific.
> 
> > Updating dt bindings for adp5588. Since the device can now function in a
> > purely gpio mode, the following keypad specific properties are now made
> 
> Hardware changed? How?

The hardware was not changed, rather support was added for an already
present functionality in regard to a new use case where the chip was being 
used purely for gpio. I will update the commit description to be more elaborate.

> > optional:
> > 	- interrupts
> > 	- keypad,num-rows
> > 	- keypad,num-columns
> > 	- linux,keymap
> >
> > However 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.
> >
> > Note that interrupts are optional, but required when the device has
> > either been configured in keypad mode or as an interrupt controller.
> >
> > Signed-off-by: Utsav Agarwal <utsav.agarwal@...log.com>
> > ---
> >  .../devicetree/bindings/input/adi,adp5588.yaml     | 51
> +++++++++++++++++++---
> >  1 file changed, 45 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> b/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > index 26ea66834ae2..827d72ece54b 100644
> > --- a/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > +++ b/Documentation/devicetree/bindings/input/adi,adp5588.yaml
> > @@ -49,7 +49,12 @@ 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 is optional if
> > +      keypad,num-rows or keypad,num-columns are not specified since the
> > +      device then acts as gpio only, during which interrupts may or may
> > +      not be utilized. If specified however, interrupts must be also be
> > +      provided as all interrupt communication is h
> 
> Don't repeat constraints in free form text.
> 
Thank you for pointing the same out, I will remove it.
> 
> andled via a single
> > +      interrupt line.
> >
> >    '#interrupt-cells':
> >      const: 2
> > @@ -65,13 +70,30 @@ properties:
> >      minItems: 1
> >      maxItems: 2
> >
> > +
> > +dependencies:
> > +  keypad,num-rows:
> > +    - linux,keymap
> > +    - keypad,num-columns
> > +  keypad,num-columns:
> > +    - linux,keymap
> > +    - keypad,num-rows
> > +  linux,keymap:
> > +    - keypad,num-rows
> > +    - keypad,num-columns
> > +  interrupt-controller:
> > +    - interrupts
> 
> How is this related to this patchset? Why? I don't understand what are
> you trying to achieve here. Hardware changed? Are you fixing something?
> How many issues are you fixing in one (!!!) commit?

Apologies for the confusion, but the issue I'm wanting to address is the lack of
support for a pure gpio mode. Since in this case, interrupts are not used, they
are made optional, but in doing so, I also need to be careful of its requirements
as a keypad, which it was originally structured as. As a result, there is an
interdependency that was established between the keypad properties
(prior to this as you can see, they were all required). This was followed 
by a requirement to make the interrupts required if the device 
was intended to be an interrupt controller in the last review
(https://lore.kernel.org/all/Zq17uABHdNENnwVq@google.com/)

> 
> > +
> > +if:
> > +  required:
> > +    - linux,keymap
> > +then:
> > +  required:
> > +    - interrupts
> > +
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> > -  - keypad,num-rows
> > -  - keypad,num-columns
> > -  - linux,keymap
> >
> >  unevaluatedProperties: false
> >
> > @@ -108,4 +130,21 @@ examples:
> >              >;
> >          };
> >      };
> > -...
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/input/input.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> 
> Where do you use these headers?

I will remove the extra headers
> 

Utsav