linux-kernel - RE: [PATCH v5 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: 
 <SJ0PR03MB63432316EE8382033A4396369BDD2@SJ0PR03MB6343.namprd03.prod.outlook.com>
Date: Wed, 3 Jul 2024 15:55:11 +0000
From: "Agarwal, Utsav" <Utsav.Agarwal@...log.com>
To: Conor Dooley <conor@...nel.org>
CC: "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>,
        "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 v5 3/3] dt-bindings: input: Update dtbinding for adp5588

Hi Conor,

Thank you for your feedback.
> -----Original Message-----
> From: Conor Dooley <conor@...nel.org>
> Sent: Wednesday, July 3, 2024 4:20 PM
> To: Agarwal, Utsav <Utsav.Agarwal@...log.com>
> Cc: 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>; 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 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@...log.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@...log.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