Re: [PATCH v2 1/2] dt-bindings: input: Add bindings for Azoteq IQS7210A/7211A/E

[Jeff LaBundy <jeff@labundy.com>]

Hi Rob,

On Mon, Jun 12, 2023 at 09:29:25AM -0600, Rob Herring wrote:
> On Sun, Jun 11, 2023 at 08:06:24PM -0500, Jeff LaBundy wrote:
> > Hi Rob,
> > 
> > On Fri, Jun 09, 2023 at 08:25:38AM -0600, Rob Herring wrote:
> > > On Mon, May 29, 2023 at 07:33:47PM -0500, Jeff LaBundy wrote:
> > > > Add bindings for the Azoteq IQS7210A/7211A/E family of trackpad/
> > > > touchscreen controllers.
> > > > 
> > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > > > ---
> > > > Changes in v2:
> > > >  - Renamed 'azoteq,default-comms' to 'azoteq,forced-comms-default' and redefined
> > > >    0, 1 and 2 as unspecified, 0 and 1, respectively
> > > >  - Defined ATI upon its first occurrence
> > > >  - Redefined 'azoteq,gesture-angle' in units of degrees
> > > >  - Declared 'azoteq,rx-enable' to depend upon 'azoteq,tx-enable' within the
> > > >    'trackpad' node
> > > > 
> > > > Hi Rob,
> > > > 
> > > > I attempted to reference existing properties from a common binding [1] as per
> > > > your feedback in [2], however 'make DT_CHECKER_FLAGS=-m dt_binding_check' fails
> > > > with the message 'Vendor specific properties must have a type and description
> > > > unless they have a defined, common suffix.'
> > > 
> > > Is that because you have differing constraints in each case?
> > 
> > In the failing example [2], I have started with a simple boolean that carries
> > nothing but a type and description. From the new azoteq,common.yaml:
> > 
> > properties:
> >   [...]
> > 
> >   azoteq,use-prox:
> >     type: boolean
> >     description: foo
> > 
> > And from the first consumer:
> > 
> > patternProperties:
> >   "^hall-switch-(north|south)$":
> >     type: object
> >     allOf:
> >       - $ref: input.yaml#
> >       - $ref: azoteq,common.yaml#
> >     description: bar
> > 
> >     properties:
> >       linux,code: true
> >       azoteq,use-prox: true
> > 
> > However, the tooling presents the following:
> > 
> >   CHKDT   Documentation/devicetree/bindings/processed-schema.json
> > /home/jlabundy/work/linux/Documentation/devicetree/bindings/input/iqs62x-keys.yaml: patternProperties:^hall-switch-(north|south)$:properties:azoteq,use-prox: True is not of type 'object'
> > 	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
> > 	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
> > 
> > [...]
> > 
> > I am committed to addressing your feedback; to help me do so, can you help me
> > identify what I've done wrong, and/or point me to an example that successfully
> > passes dt_binding_check?
> 
> You're not doing anything wrong. There's 2 options here. The first is we 
> could just relax the check to allow boolean schema for vendor 
> properties. The main issue with that is we then have to look for that 
> improperly used and it doesn't help if you do have additional 
> constraints to add on top of the common definition. The former can 
> mostly be addressed by checking there is a type associated with the 
> property. I'm going to look into adding that.

Thank you for your feedback. I started with a boolean property at first to
simply test the idea before moving too far forward. In reality however, the
common binding has many uint32's and uint32-arrays as well, often with
different constraints among consumers. For this option to be effective, it
would need to be extended to all types IMO.

> 
> Alternatively, you could drop listing the properties and 
> use 'unevaluatedProperties'. That's not quite equal to what you have. 
> Presumably, you have 'additionalProperties' in this case, so listing 
> them serves the purpose of defining what subset of properties each node 
> uses from the referenced schema. We frequently don't worry if there are 
> common properties not used by a specific schema. This also wouldn't work 
> if you have additional constraints to add.

Because of varying constraints among each consumer, I do not believe this
option is viable either.

I also think adopting 'unevaluatedProperties' here would be confusing from
a customer perspective in this case. The new common binding has dozens of
properties for which some are shared between devices A and B but not C, or
devices B and C but not A.

Without each device's binding explicitly opting in for supported properties,
it's difficult for customers to know what is supported for a given device.
These particular devices are highly configurable yet void of nonvolatile
memory, so there is simply no way around having so many properties. Most are
touched in some way throughout various downstream applications.

Therefore I'd like to propose option (3), which is to move forward with patch
[1/2] as-is and decouple the merging of this driver from future enhancements
to the tooling. While patch [1/2] is admittedly a big binding with some repeat
descriptions, none of the duplicate properties introduce a conflicting type.

If in the future option (1) can support all property types and handle varying
constraints among consumers, I would be happy to be one of the first guinea
pigs. Does this path seem like a reasonable compromise?

> 
> Rob

Kind regards,
Jeff LaBundy