Re: [PATCH 1/4] dt-bindings: hid: Add CP2112 HID USB to SMBus Bridge

[Daniel Kaehn <kaehndan@gmail.com>]

Thanks for all of the comments. All feedback is ACK'd and will be
added in v2 -- what follows is just commentary on some comments.

On Sun, Jan 29, 2023 at 5:33 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/01/2023 12:05, Krzysztof Kozlowski wrote:
> > On 28/01/2023 21:26, Danny Kaehn wrote:
> >> This is a USB HID device which includes an I2C controller and 8 GPIO pins.
> >>
> > Thank you for your patch. There is something to discuss/improve.
> >
> >> The binding allows describing the chip's gpio and i2c controller in DT
> >> using the subnodes named "gpio" and "i2c", respectively. This is
> >> intended to be used in configurations where the CP2112 is permanently
> >> connected in hardware.
> >>
> >> Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
> >> ---
> >>  .../bindings/hid/silabs,cp2112.yaml           | 82 +++++++++++++++++++
> >
> > There is no "hid" directory, so I think such devices where going to
> > different place, didn't they?

Good point, I didn't notice other hid-related bindings went into
input/ -- will change

> >
> >> +  While USB devices typically aren't described in DeviceTree, doing so with the
> >> +  CP2112 allows use of its i2c and gpio controllers with other DT nodes when
> >> +  the chip is expected to be found on a USB port.
> >
> > Drop these three and replace with description of the hardware.

Understood. I noticed that a similar usb-based binding included
a similar description (net/marvell,mvusb.yaml) but I understand why
we would not want this in new bindings.

> >
> >> +  i2c:
> >> +    $ref: /schemas/i2c/i2c-controller.yaml#
> >
> > This is not specific enough. What controller is there?
>
> OK, assuming this is tightly wired (with cp2112 I2C controller), then
> the compatible could be skipped as it is inferred from parent one. Yet
> still you need description and unevaluatedProperties.
>

Great point, will update -- I didn't quite understand that child nodes of the
root could have properties/unevaluatedProperties/etc.. but I see now that
that is well-documented (just not often done in existing bindings)!

> >
> > Missing unevaluatedProperties: false, anyway.
> >
> >> +  gpio:
> >> +    $ref: /schemas/gpio/gpio.yaml#
> >
> > Same comments.
>
> Description, unevaluatedProperties and constraints on properties (line
> names, reserved ranges, ranges).
>

Will add.

Thanks,
Danny Kaehn