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@xxxxxxxxxxx> > > > > > --- > > > > > 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
