| Message ID | 20230127151041.65751-2-jonathanh@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add device-tree support for Cypress CYPD4226 | expand |
On 27/01/2023 16:10, Jon Hunter wrote: > From: Wayne Chang <waynec@nvidia.com> > > Add the device-tree binding documentation for Cypress cypd4226 dual > Type-C controller. > > Signed-off-by: Wayne Chang <waynec@nvidia.com> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > V9: added 'unevaluatedProperties' and 'additionalProperties' > V8: removed 'additionalProperties' > V7: updated example to use 'typec' for the node name > V6: no changes > V5: updated subject and updated binding to use 'firmware-name'. > V4: no changes > V3: fix additionalProperties warning on new schema > V2: based on the review comments. Fix some addressed issues on Thanks, this is looking good, although few more questions popped up while comparing it with other bindings. (...) > + firmware-name: > + enum: > + - nvidia,gpu > + - nvidia,jetson-agx-xavier > + description: | > + The name of the CCGx firmware built for product series. > + should be set one of following: > + - "nvidia,gpu" for the NVIDIA RTX product series > + - "nvidia,jetson-agx-xavier" for the NVIDIA Jetson product series > + > +patternProperties: > + '^connector@[0-1]+$': How many connectors do you expect/support? 1111 is valid? I guess you wanted only [01]? > + $ref: /schemas/connector/usb-connector.yaml# > + unevaluatedProperties: false > + properties: > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - interrupts I would assume that at least one connector is required (oneOf: required:). > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/tegra194-gpio.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <2>; > + > + typec@8 { > + compatible = "cypress,cypd4226"; > + reg = <0x08>; > + interrupt-parent = <&gpio_aon>; > + interrupts = <TEGRA194_AON_GPIO(BB, 2) IRQ_TYPE_LEVEL_LOW>; > + firmware-name = "nvidia,jetson-agx-xavier"; > + #address-cells = <1>; > + #size-cells = <0>; > + connector@0 { > + compatible = "usb-c-connector"; > + reg = <0>; > + label = "USB-C"; > + data-role = "dual"; > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + endpoint { > + remote-endpoint = <&usb_role_switch0>; > + }; > + }; > + }; > + }; > + }; > + }; Best regards, Krzysztof
On 28/01/2023 10:29, Krzysztof Kozlowski wrote: > On 27/01/2023 16:10, Jon Hunter wrote: >> From: Wayne Chang <waynec@nvidia.com> >> >> Add the device-tree binding documentation for Cypress cypd4226 dual >> Type-C controller. >> >> Signed-off-by: Wayne Chang <waynec@nvidia.com> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> V9: added 'unevaluatedProperties' and 'additionalProperties' >> V8: removed 'additionalProperties' >> V7: updated example to use 'typec' for the node name >> V6: no changes >> V5: updated subject and updated binding to use 'firmware-name'. >> V4: no changes >> V3: fix additionalProperties warning on new schema >> V2: based on the review comments. Fix some addressed issues on > > Thanks, this is looking good, although few more questions popped up > while comparing it with other bindings. > > (...) > >> + firmware-name: >> + enum: >> + - nvidia,gpu >> + - nvidia,jetson-agx-xavier >> + description: | >> + The name of the CCGx firmware built for product series. >> + should be set one of following: >> + - "nvidia,gpu" for the NVIDIA RTX product series >> + - "nvidia,jetson-agx-xavier" for the NVIDIA Jetson product series >> + >> +patternProperties: >> + '^connector@[0-1]+$': > > How many connectors do you expect/support? 1111 is valid? I guess you > wanted only [01]? Yes only two are supported. So yes this should be just [01]. >> + $ref: /schemas/connector/usb-connector.yaml# >> + unevaluatedProperties: false >> + properties: >> + reg: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - interrupts > > I would assume that at least one connector is required (oneOf: required:). Yes there should be at least one. OK, thanks, I will add this. Jon
On 28/01/2023 10:29, Krzysztof Kozlowski wrote: ... >> + $ref: /schemas/connector/usb-connector.yaml# >> + unevaluatedProperties: false >> + properties: >> + reg: >> + maxItems: 1 >> + >> +required: >> + - compatible >> + - reg >> + - interrupts > > I would assume that at least one connector is required (oneOf: required:). I have been looking at this and wondered if we need the 'oneOf' in this case? Shouldn't we just add 'connector@0' to the required properties? At first I added ... oneOf: - required: - connector@0 - required: - connector@1 But this is not correct, because the above will cause warnings if both connector@0 and connector@1 are present. Jon
On 30/01/2023 22:10, Jon Hunter wrote: > > On 28/01/2023 10:29, Krzysztof Kozlowski wrote: > > ... > >>> + $ref: /schemas/connector/usb-connector.yaml# >>> + unevaluatedProperties: false >>> + properties: >>> + reg: >>> + maxItems: 1 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >> >> I would assume that at least one connector is required (oneOf: required:). > > > I have been looking at this and wondered if we need the 'oneOf' in this > case? Shouldn't we just add 'connector@0' to the required properties? > > At first I added ... > > oneOf: > - required: > - connector@0 > - required: > - connector@1 > > But this is not correct, because the above will cause warnings if both > connector@0 and connector@1 are present. Right, then anyOf should do the trick. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml new file mode 100644 index 000000000000..b68f9ba621ee --- /dev/null +++ b/Documentation/devicetree/bindings/usb/cypress,cypd4226.yaml @@ -0,0 +1,92 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/usb/cypress,cypd4226.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Cypress cypd4226 Type-C Controller + +maintainers: + - Wayne Chang <waynec@nvidia.com> + +description: + The Cypress cypd4226 is a dual Type-C controller that is controlled + via an I2C interface. + +properties: + compatible: + const: cypress,cypd4226 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + reg: + const: 0x08 + + interrupts: + items: + - description: cypd4226 host interrupt + + firmware-name: + enum: + - nvidia,gpu + - nvidia,jetson-agx-xavier + description: | + The name of the CCGx firmware built for product series. + should be set one of following: + - "nvidia,gpu" for the NVIDIA RTX product series + - "nvidia,jetson-agx-xavier" for the NVIDIA Jetson product series + +patternProperties: + '^connector@[0-1]+$': + $ref: /schemas/connector/usb-connector.yaml# + unevaluatedProperties: false + properties: + reg: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/tegra194-gpio.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <2>; + + typec@8 { + compatible = "cypress,cypd4226"; + reg = <0x08>; + interrupt-parent = <&gpio_aon>; + interrupts = <TEGRA194_AON_GPIO(BB, 2) IRQ_TYPE_LEVEL_LOW>; + firmware-name = "nvidia,jetson-agx-xavier"; + #address-cells = <1>; + #size-cells = <0>; + connector@0 { + compatible = "usb-c-connector"; + reg = <0>; + label = "USB-C"; + data-role = "dual"; + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + endpoint { + remote-endpoint = <&usb_role_switch0>; + }; + }; + }; + }; + }; + };