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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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.

> 
>>
>>> +
>>> +  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.

> 
>>
>>> +
>>> +      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