dt-bindings: input: azoteq: Fix differing types

'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@kernel.org>
---
 .../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(-)

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@kernel.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

On Wed, Jan 25, 2023 at 7:51 PM Jeff LaBundy <jeff@labundy.com> wrote:
>
> 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@kernel.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?

Shrug

How exactly are you going to change an existing property and not break
existing users? Or are there not any users?

We have the same properties with 2 definitions. At the end of the day,
I just want to fix that...

Rob