Hi Rob, Thank you for your thorough review. On Thu, Apr 30, 2020 at 10:11:08AM -0500, Rob Herring wrote: > On Sun, Apr 19, 2020 at 06:47:47PM -0500, Jeff LaBundy wrote: > > This patch adds device tree bindings for the Azoteq IQS269A > > capacitive touch controller. > > > > Signed-off-by: Jeff LaBundy <jeff@xxxxxxxxxxx> > > --- > > .../devicetree/bindings/input/iqs269a.yaml | 591 +++++++++++++++++++++ > > 1 file changed, 591 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/input/iqs269a.yaml > > Kind of a lot of properties compared to other devices. Why so many? That > said, nothing looks to be obviously something that doesn't belong in DT. I don't disagree; this device simply has a lot of knobs for accommodating multiple sensing modes across different applications. Once I added support for the ones I expected to be most commonly used, however, I didn't see a reason to exclude the remaining minority and risk having to add something later. > > No interdependencies between properties? If there are, use > 'dependencies'. Strictly speaking, no; each property can be specified independently of any other property and the device's registers will be updated accordingly. That being said, a couple of properties do impose restrictions on others within specific channels for certain applications. It wasn't clear if/how 'dependencies' could describe these conditional relationships, so I opted to include a note in the descriptions where applicable. [...] > > + > > + azoteq,rate-np-ms: > > + allOf: > > + - $ref: /schemas/types.yaml#/definitions/uint32 > > With a unit suffix, you can drop the type $ref. Sure thing, thank you for catching these. Once I remove $ref in these cases, is an 'allOf' still required above the remaining minimum/maximum/etc.? > > > + - minimum: 0 > > + maximum: 255 > > + default: 16 > > + description: Specifies the report rate (in ms) during normal-power mode. > > + On a related note, should all items under an 'allOf' be preceded by a hyphen? Kind regards, Jeff LaBundy
