Re: [PATCH 1/2] dt-bindings: input: iqs7222: Add properties for Azoteq IQS7222D

[Jeff LaBundy <jeff@labundy.com>]

Hi Krzysztof,

On Fri, Jun 02, 2023 at 09:21:00AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2023 03:23, Jeff LaBundy wrote:
> >>>  
> >>>    reg:
> >>>      maxItems: 1
> >>> @@ -173,6 +174,148 @@ properties:
> >>>      maximum: 3000
> >>>      description: Specifies the report rate (in ms) during ultra-low-power mode.
> >>>  
> >>> +  touchscreen-size-x: true
> >>> +  touchscreen-size-y: true
> >>> +  touchscreen-inverted-x: true
> >>> +  touchscreen-inverted-y: true
> >>> +  touchscreen-swapped-x-y: true
> >>
> >> Why? Aren't they coming from common schema?
> > 
> > Yes, but because additionalProperties is set to false here, we must explicitly
> > include the subset of properties from the common schema that are allowed for
> > this particular instance. I counted over a dozen other bindings doing the same.
> > 
> > In case I have misunderstood, please let me know.
> 
> If you are listing now most of touchscreen properties, it is a sign you
> should use just unevaluatedProperties: false (instead
> additionalProperties) and then no need for any of these here.

At present, touchscreen.yaml offers 17 properties. I am only replicating 5
of them, so this binding may not be such a case for this idea. That said, I
tried your suggestion locally.

In the present implementation (additionalProperties: false), the tooling
rightfully presents an error if the example includes a property that is not
explicitly offered (e.g. 'touchscreen-x-plate-ohms').

If I change the binding to use "unevaluatedProperties: false", the tooling
accepts an example which includes this same property that is not supported
by this particular hardware. This seems like a regression.

In my humble opinion, the present implementation is optimal. It is for that
reason we see multiple bindings with this same block of inherited properties.
I will leave this unchanged; please let me know in case I have misunderstood.

> 
> > 
> >>
> >>> +
> >>> +  trackpad:
> >>> +    type: object
> >>> +    description: Represents all channels associated with the trackpad.
> >>> +
> >>> +    properties:
> >>> +      azoteq,channel-select:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +        minItems: 1
> >>> +        maxItems: 12
> >>> +        items:
> >>> +          minimum: 0
> >>> +          maximum: 13
> >>> +        description:
> >>> +          Specifies the order of the channels that participate in the trackpad.
> >>> +          Specify 255 to omit a given channel for the purpose of mapping a non-
> >>> +          rectangular trackpad.
> >>> +
> >>> +      azoteq,num-rows:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        minimum: 1
> >>> +        maximum: 12
> >>> +        description: Specifies the number of rows that comprise the trackpad.
> >>> +
> >>> +      azoteq,num-cols:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        minimum: 1
> >>> +        maximum: 12
> >>> +        description: Specifies the number of columns that comprise the trackpad.
> >>> +
> >>> +      azoteq,top-speed:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        multipleOf: 4
> >>> +        minimum: 0
> >>> +        maximum: 1020
> >>> +        description:
> >>> +          Specifies the speed of movement after which coordinate filtering is
> >>> +          no longer applied.
> >>
> >> Units?
> > 
> > This is a ratiometric, i.e. unitless value that represents a hardware filter
> > coefficient. It already exists in this binding prior to this patch under the
> > slider-0/1 node and is simply re-used here.
> 
> Then mention the ratio (e.g. "speed of movement expressed as ratio
> of..."). Description said "speed" and we usually measure speed in very
> specific units.

ACK; this is a good call-out. Please note that the existing binding uses the
same language elsewhere; I will mirror the updated language to the existing
instances in a separate patch.

> 
> > 
> >>
> >>> +
> >>> +      azoteq,bottom-speed:
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        minimum: 0
> >>> +        maximum: 255
> >>> +        description:
> >>> +          Specifies the speed of movement after which coordinate filtering is
> >>> +          linearly reduced.
> >>
> >> Units?
> > 
> > Same here.
> > 
> >>
> >>> +
> >>> +      azoteq,use-prox:
> >>> +        type: boolean
> >>> +        description:
> >>> +          Directs the trackpad to respond to the proximity states of the se-
> >>> +          lected channels instead of their corresponding touch states. Note
> >>
> >> Don't split the words.
> > 
> > ACK.
> > 
> >>
> >>> +          the trackpad cannot report granular coordinates during a state of
> >>> +          proximity.
> >>> +
> >>> +    patternProperties:
> >>> +      "^azoteq,lower-cal-(x|y)$":
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        minimum: 0
> >>> +        maximum: 255
> >>> +        description: Specifies the trackpad's lower starting points.
> >>
> >> Why would you need this property? Why does this represent hardware property?
> > 
> > This property and its cousin below define the physical boundaries of the
> > touch surface. They are typically used to mask areas that cannot elicit
> > an electrical response due to manufacturing tolerances or the presence of
> > an overlay. For that reason, they descend directly from properties of the
> > hardware.
> > 
> > Similar properties already exist in this binding for the slider case; this
> > device simply extends the functionality to a second dimenstion.
> 
> OK
> 
> > 
> >>
> >>> +
> >>> +      "^azoteq,upper-cal-(x|y)$":
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +        minimum: 0
> >>> +        maximum: 255
> >>> +        description: Specifies the trackpad's upper starting points.
> >>> +
> >>> +      "^event-(press|tap|(swipe|flick)-(x|y)-(pos|neg))$":
> >>> +        type: object
> >>> +        $ref: input.yaml#
> >>> +        description:
> >>> +          Represents a press or gesture event reported by the trackpad. Specify
> >>> +          'linux,code' under the press event to report absolute coordinates.
> >>> +
> >>> +        properties:
> >>> +          linux,code: true
> >>> +
> >>> +          azoteq,gesture-angle-tighten:
> >>> +            type: boolean
> >>> +            description:
> >>> +              Limits the tangent of the gesture angle to 0.5 (axial gestures
> >>> +              only). If specified in one direction, the effect is applied in
> >>> +              either direction.
> >>> +
> >>> +          azoteq,gesture-max-ms:
> >>> +            multipleOf: 16
> >>> +            minimum: 0
> >>> +            maximum: 4080
> >>> +            description:
> >>> +              Specifies the length of time (in ms) within which a tap, swipe
> >>> +              or flick gesture must be completed in order to be acknowledged
> >>> +              by the device. The number specified for any one swipe or flick
> >>> +              gesture applies to all other swipe or flick gestures.
> >>> +
> >>> +          azoteq,gesture-min-ms:
> >>> +            multipleOf: 16
> >>> +            minimum: 0
> >>> +            maximum: 4080
> >>> +            description:
> >>> +              Specifies the length of time (in ms) for which a tap gesture must
> >>> +              be held in order to be acknowledged by the device.
> >>> +
> >>> +          azoteq,gesture-dist:
> >>> +            $ref: /schemas/types.yaml#/definitions/uint32
> >>> +            minimum: 0
> >>> +            maximum: 65535
> >>> +            description:
> >>> +              Specifies the distance across which a tap, swipe or flick gesture
> >>> +              must travel in order to be acknowledged by the device. The number
> >>> +              specified for any one swipe or flick gesture applies to all other
> >>> +              swipe or flick gestures.
> >>> +
> >>> +          azoteq,gpio-select:
> >>> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> +            minItems: 1
> >>> +            maxItems: 3
> >>> +            items:
> >>> +              minimum: 0
> >>> +              maximum: 2
> >>> +            description: |
> >>> +              Specifies one or more GPIO mapped to the event as follows:
> >>> +              0: GPIO0
> >>> +              1: GPIO3
> >>> +              2: GPIO4
> >>> +
> >>> +              Note that although multiple events can be mapped to a single
> >>> +              GPIO, they must all be of the same type (proximity, touch or
> >>> +              trackpad gesture).
> >>> +
> >>> +        additionalProperties: false
> >>> +
> >>> +    required:
> >>> +      - azoteq,channel-select
> >>> +
> >>> +    additionalProperties: false
> >>> +
> >>>  patternProperties:
> >>>    "^cycle-[0-9]$":
> >>>      type: object
> >>> @@ -288,6 +431,10 @@ patternProperties:
> >>>            Activates the reference channel in response to proximity events
> >>>            instead of touch events.
> >>>  
> >>> +      azoteq,counts-filt-enable:
> >>> +        type: boolean
> >>> +        description: Applies counts filtering to the channel.
> >>> +
> >>>        azoteq,ati-band:
> >>>          $ref: /schemas/types.yaml#/definitions/uint32
> >>>          enum: [0, 1, 2, 3]
> >>> @@ -432,12 +579,12 @@ patternProperties:
> >>>              description: |
> >>>                Specifies one or more GPIO mapped to the event as follows:
> >>>                0: GPIO0
> >>> -              1: GPIO3 (IQS7222C only)
> >>> -              2: GPIO4 (IQS7222C only)
> >>> +              1: GPIO3
> >>> +              2: GPIO4
> >>
> >> Why changing this? Is it valid for IQS7222A?
> > 
> > It's not, only for 'C' and now 'D'. However, the restriction for 'A' is already
> > conveyed in an if/then schema in the original binding. So rather than updating
> > this text to say "(IQS7222C and IQS7222D only)", I opted to drop the open-coded
> > text and rely on the existing schema.
> 
> OK
> 
> > 
> 
> 
> Best regards,
> Krzysztof
> 

Kind regards,
Jeff LaBundy