Re: [PATCH] dt-bindings: input: azoteq: Fix differing types
From: Jeff LaBundy
Date: Wed Jan 25 2023 - 20:51:04 EST
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@xxxxxxxxxx>
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