| Message ID | 20230125221416.3058051-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dt-bindings: input: azoteq: Fix differing types | expand |
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
Hi Rob, On Wed, Jan 25, 2023 at 09:10:33PM -0600, Rob Herring wrote: > 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? There are no known users at this time. > > We have the same properties with 2 definitions. At the end of the day, > I just want to fix that... Agreed on all counts. I've folded my proposal into the existing fix for this driver and sent [1] for your consideration. [1] https://patchwork.kernel.org/patch/13119464/ > > Rob Kind regards, Jeff LaBundy
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:
'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(-)