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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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?
> 
>>  1 file changed, 82 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/hid/silabs,cp2112.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml
>> new file mode 100644
>> index 000000000000..49287927c63f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hid/silabs,cp2112.yaml
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/hid/silabs,cp2112.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: CP2112 HID USB to SMBus/I2C Bridge
>> +
>> +maintainers:
>> +  - Danny Kaehn <kaehndan@gmail.com>
>> +
>> +description:
>> +  This is a USB HID device which includes an I2C controller and 8 GPIO pins.
> 
> s/This is/CP2112 is/
> 
>> +  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.
> 
>> +
>> +properties:
>> +  compatible:
>> +    const: usb10c4,ea90
> 
> So this is an USB device, so I guess they all go to usb?
> 
> Missing blank line.
> 
>> +  reg:
>> +    maxItems: 1
>> +    description: The USB port number on the host controller
> 
> Blank line
> 
>> +  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.

> 
> Missing unevaluatedProperties: false, anyway.
> 
>> +  gpio:
>> +    $ref: /schemas/gpio/gpio.yaml#
> 
> Same comments.

Description, unevaluatedProperties and constraints on properties (line
names, reserved ranges, ranges).



> 
>> +
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/input/input.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    usb1 {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
> 
> Drop, not related.
> 
>> +
>> +      usb@1 {
>> +        compatible = "usb424,2514";
>> +        reg = <1>;
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        device@1 {	/* CP2112 I2C Bridge */
>> +          compatible = "usb10c4,ea90";
>> +          reg = <1>;
>> +
>> +          cp2112_i2c0: i2c {
> 
> Drop unneeded labels.
> 
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +            /* Child I2C Devices can be described as normal here */

Drop also this comment.

Best regards,
Krzysztof