linux-kernel - Re: [PATCH] dt-bindings: input: azoteq: Fix differing types

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]
Date:   Wed, 25 Jan 2023 19:50:51 -0600
From:   Jeff LaBundy <jeff@...undy.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: input: azoteq: Fix differing types

Hi Rob,

On Wed, Jan 25, 2023 at 04:14:16PM -0600, Rob Herring wrote:
> 'azoteq,ati-base' and 'azoteq,thresh' properties are defined in multiple
> bindings, but have differing types defined. Both 'uint32' and
> 'uint32-array' are used. Unify these to use 'uint32-array' everywhere.
> 
> Signed-off-by: Rob Herring <robh@...nel.org>

Thank you for the patch. While this is a step forward in moving toward
a common binding for this vendor like we have discussed in the past, I
do not agree with this approach and will instead propose an alternative
that accomplishes the same goal.

For all of these devices, a single sensing channel takes a base and a
threshold property. IQS626A is unique in that a fixed number of channels
form a trackpad, and I decided at the time to simply define the base and
target properties for all channels as a uint32-array.

For all other existing drivers, as well as others coming down the pipe,
base and threshold are uint32s. I find it confusing to redefine all of
those as single-element arrays, especially on account of one device.

In hindsight, a better design would have been to define a child node
for each channel under the trackpad node, with each child node accepting
a uint32 base and threshold. That would follow other devices, be more
representative of the actual hardware, and keep the definitions the same
across each binding.

So, that's what I propose to do here instead. I happen to have a fix in
review [1] that addresses a bug related to this parsing code, and would
be happy to build this solution on top assuming it can wait until the
next cycle. Does this compromise sound OK?

[1] https://patchwork.kernel.org/patch/https://patchwork.kernel.org/patch/13087768/

> ---
>  .../bindings/input/azoteq,iqs7222.yaml        | 12 ++++---
>  .../devicetree/bindings/input/iqs269a.yaml    | 34 +++++++++++--------
>  .../devicetree/bindings/input/iqs626a.yaml    | 12 ++++---
>  3 files changed, 33 insertions(+), 25 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> index 9ddba7f2e7aa..f2382a56884d 100644
> --- a/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> +++ b/Documentation/devicetree/bindings/input/azoteq,iqs7222.yaml
> @@ -354,10 +354,11 @@ patternProperties:
>          description: Specifies the channel's ATI target.
>  
>        azoteq,ati-base:
> -        $ref: /schemas/types.yaml#/definitions/uint32
> -        multipleOf: 16
> -        minimum: 0
> -        maximum: 496
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - multipleOf: 16
> +            minimum: 0
> +            maximum: 496
>          description: Specifies the channel's ATI base.
>  
>        azoteq,ati-mode:
> @@ -440,7 +441,8 @@ patternProperties:
>                slider gesture).
>  
>            azoteq,thresh:
> -            $ref: /schemas/types.yaml#/definitions/uint32
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            maxItems: 1
>              description:
>                Specifies the threshold for the event. Valid entries range from
>                0-127 and 0-255 for proximity and touch events, respectively.
> diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
> index 3c430d38594f..4fa20f0f6847 100644
> --- a/Documentation/devicetree/bindings/input/iqs269a.yaml
> +++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
> @@ -334,9 +334,10 @@ patternProperties:
>            3: Full
>  
>        azoteq,ati-base:
> -        $ref: /schemas/types.yaml#/definitions/uint32
> -        enum: [75, 100, 150, 200]
> -        default: 100
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - enum: [75, 100, 150, 200]
> +            default: 100
>          description: Specifies the channel's ATI base.
>  
>        azoteq,ati-target:
> @@ -391,10 +392,11 @@ patternProperties:
>  
>          properties:
>            azoteq,thresh:
> -            $ref: /schemas/types.yaml#/definitions/uint32
> -            minimum: 0
> -            maximum: 255
> -            default: 10
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            items:
> +              - minimum: 0
> +                maximum: 255
> +                default: 10
>              description: Specifies the threshold for the event.
>  
>            linux,code: true
> @@ -408,10 +410,11 @@ patternProperties:
>  
>          properties:
>            azoteq,thresh:
> -            $ref: /schemas/types.yaml#/definitions/uint32
> -            minimum: 0
> -            maximum: 255
> -            default: 8
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            items:
> +              - minimum: 0
> +                maximum: 255
> +                default: 8
>              description: Specifies the threshold for the event.
>  
>            azoteq,hyst:
> @@ -432,10 +435,11 @@ patternProperties:
>  
>          properties:
>            azoteq,thresh:
> -            $ref: /schemas/types.yaml#/definitions/uint32
> -            minimum: 0
> -            maximum: 255
> -            default: 26
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            items:
> +              - minimum: 0
> +                maximum: 255
> +                default: 26
>              description: Specifies the threshold for the event.
>  
>            azoteq,hyst:
> diff --git a/Documentation/devicetree/bindings/input/iqs626a.yaml b/Documentation/devicetree/bindings/input/iqs626a.yaml
> index 7a27502095f3..dbd63d48605c 100644
> --- a/Documentation/devicetree/bindings/input/iqs626a.yaml
> +++ b/Documentation/devicetree/bindings/input/iqs626a.yaml
> @@ -234,8 +234,9 @@ patternProperties:
>            about the available RUI options.
>  
>        azoteq,ati-base:
> -        $ref: /schemas/types.yaml#/definitions/uint32
> -        enum: [75, 100, 150, 200]
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        items:
> +          - enum: [75, 100, 150, 200]
>          description:
>            Specifies the channel's ATI base. The default value is a function
>            of the channel and the device's RUI.
> @@ -475,9 +476,10 @@ patternProperties:
>  
>          properties:
>            azoteq,thresh:
> -            $ref: /schemas/types.yaml#/definitions/uint32
> -            minimum: 0
> -            maximum: 255
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            items:
> +              - minimum: 0
> +                maximum: 255
>              description: Specifies the threshold for the event.
>  
>            azoteq,hyst:
> -- 
> 2.39.0
> 

Kind regards,
Jeff LaBundy