Messages in this thread | | | Date | Mon, 27 Jun 2022 16:21:15 +0200 | Subject | Re: [PATCH 1/4] dt-bindings: usb: typec: add bindings for stm32g0 controller | From | Fabrice Gasnier <> |
| |
On 6/24/22 18:16, Krzysztof Kozlowski wrote: > On 24/06/2022 17:54, Fabrice Gasnier wrote: >> This patch adds DT schema documentation for the STM32G0 Type-C controller. > > No "This patch"
Hi Krzysztof,
ack,
> > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > >> STM32G0 provides an integrated USB Type-C and power delivery interface. >> It can be programmed with a firmware to handle UCSI protocol over I2C >> interface. A GPIO is used as an interrupt line. >> It may be used as a wakeup source, so use optional "wakeup-source" and >> "power-domains" properties to support wakeup. >> >> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> --- >> .../bindings/usb/st,typec-stm32g0.yaml | 83 +++++++++++++++++++ >> 1 file changed, 83 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> >> diff --git a/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> new file mode 100644 >> index 0000000000000..b2729bd015a1a >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/usb/st,typec-stm32g0.yaml >> @@ -0,0 +1,83 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/usb/st,typec-stm32g0.yaml#" >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > No quotes.
ack,
> >> + >> +title: STMicroelectronics STM32G0 Type-C controller bindings > > s/bindings//
ack,
> >> + >> +description: | >> + The STM32G0 MCU can be programmed to control Type-C connector(s) through I2C >> + typically using the UCSI protocol over I2C, with a dedicated alert >> + (interrupt) pin. >> + >> +maintainers: >> + - Fabrice Gasnier <fabrice.gasnier@foss.st.com> >> + >> +properties: >> + compatible: >> + const: st,stm32g0-typec >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + connector: >> + type: object> + allOf: >> + - $ref: ../connector/usb-connector.yaml# > > Full path, so /schemas/connector/... > > unevaluatedProperties: false
ack,
> >> + >> + firmware-name: >> + description: | >> + Should contain the name of the default firmware image >> + file located on the firmware search path >> + >> + wakeup-source: true >> + power-domains: true > > maxItems
Do you mean maxItems regarding the "power-domains" property ? This will depend on the user platform, where it's used as an I2C device. So I'm not sure this can / should be specified here. Could please you clarify ?
> >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + i2c5 { > > Just "i2c"
ack,
> >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + stm32g0@53 { > > Generic node name describing class of the device.
I wasn't aware of generic node name for an I2C device (not talking of the controller). I may have missed it.
Could you please clarify ?
> >> + compatible = "st,stm32g0-typec"; >> + reg = <0x53>; >> + /* Alert pin on GPIO PE12 */ >> + interrupts = <12 IRQ_TYPE_EDGE_FALLING>; >> + interrupt-parent = <&gpioe>; >> + >> + /* Example with one type-C connector */ >> + connector { >> + compatible = "usb-c-connector"; >> + label = "USB-C"; >> + >> + port { > > This does not look like proper schema of connector.yaml.
This refers to graph.yaml [1], where similar example is seen [2].
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L79
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/graph.yaml#L207
device-1 { port { device_1_output: endpoint { remote-endpoint = <&device_2_input>; }; }; }; device-2 { port { device_2_input: endpoint { remote-endpoint = <&device_1_output>; }; }; };
Could you please clarify this point too ?
> >> + con_usb_c_ep: endpoint { >> + remote-endpoint = <&usbotg_hs_ep>; >> + }; >> + }; >> + }; >> + }; >> + }; >> + >> + usbotg_hs { > > Generic node names, no underscores in node names.
ack, I guess you'd recommend "usb" here. I'll update it.
Thanks for reviewing, Best Regards, Fabrice
> >> + usb-role-switch; >> + port { >> + usbotg_hs_ep: endpoint { >> + remote-endpoint = <&con_usb_c_ep>; >> + }; >> + }; >> + }; >> +... > > > Best regards, > Krzysztof
| |