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
