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

[Rob Herring <robh@kernel.org>]

On Mon, Jun 12, 2023 at 07:58:54PM -0500, Jeff LaBundy wrote:
> 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?

Yes, that's fine.

Rob