RE: [PATCH v1 2/2] dt-bindings: extcon: Add Realtek DHC RTD SoC Type-C

[Stanley Chang[昌育德]]

Hi Krzysztof,

> On 22/08/2023 12:28, Stanley Chang wrote:
> > Document the device-tree bindings for Realtek SoCs Type-C.
> > Realtek DHC (digital home center) RTD SoCs support a Type-C module.
> >
> > Signed-off-by: Stanley Chang <stanley_chang@xxxxxxxxxxx>
> > ---
> >  .../bindings/extcon/extcon-rtk-type-c.yaml    | 77 +++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/extcon/extcon-rtk-type-c.yaml
> 
> Filename like compatible.

I will rename.

> >
> > diff --git
> > a/Documentation/devicetree/bindings/extcon/extcon-rtk-type-c.yaml
> > b/Documentation/devicetree/bindings/extcon/extcon-rtk-type-c.yaml
> > new file mode 100644
> > index 000000000000..d14b9ee544b9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/extcon/extcon-rtk-type-c.yaml
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright
> > +2023 Realtek Semiconductor Corporation %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/extcon/extcon-rtk-type-c.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Realtek DHC RTD SoCs USB Type-C detection
> 
> Type-c usually go to usb directory.

This binding is not for a type-c controller.
It is an extcon device for type-c connector detection.
So I put it at extcon directory.

And I will add “connector” to the title.
title: Realtek DHC RTD SoCs USB Type-C Connector detection

> > +maintainers:
> > +  - Stanley Chang <stanley_chang@xxxxxxxxxxx>
> > +
> > +description: |
> 
> Do not need '|' unless you need to preserve formatting.

Ok

> > +  Realtek digital home center (DHC) RTD series SoCs include a type c
> module.
> > +  This module is able to detect the state of type c connector.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - realtek,rtd1295-type-c
> > +      - realtek,rtd1312c-type-c
> > +      - realtek,rtd1315e-type-c
> > +      - realtek,rtd1319-type-c
> > +      - realtek,rtd1319d-type-c
> > +      - realtek,rtd1395-type-c
> > +      - realtek,rtd1619-type-c
> > +      - realtek,rtd1619b-type-c
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description: Address and length of register set for type-c module.
> 
> Drop description, it's obvious.

Ok.

> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  nvmem-cells:
> > +    maxItems: 1
> > +    description:
> > +      The phandle to nvmem cell that contains the trimming data.
> > +      If unspecified, default value is used.
> > +
> > +  pinctrl-names:
> > +    const: default
> > +    description: A pinctrl state names "default" must be defined.
> 
> Drop, property not needed.

Ok

> > +
> > +  pinctrl-0:
> > +    description:
> > +      Should contain default pinctrl.
> 
> Drop, property not needed.
> 
Ok.
> 
> > +
> > +  nvmem-cell-names:
> > +    const: usb-cal
> 
> items:
>   - const: usb-cal

> 
> and move description to nvmem-cells.
> 
> > +    description:
> > +      The type c parameter trimming data specified via efuse.
> 
> Keep same properties next to each other. nvmem-cells should be followed by
> nvmem-cell-names.
> 
> > +
> > +  realtek,rd-ctrl-gpio:
> > +    description: The gpio node to control external Rd on board.
> 
> The names are always "gpios".

Do you mean "realtek,rd-ctrl-gpios" ?
> 
> > +    maxItems: 1
> > +
> 
> You miss here connector. Probably also VBUS supply and other supplies.
> 
Ok, I will add connector.

> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    type-c@7220 {
> > +        compatible = "realtek,rtd1619b-type-c";
> > +        reg = <0x7220 0x20>;
> > +        interrupts = <0 60 4>;
> 
> Use proper defines for common constants.

Ok, I will revise this.

> > +
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&usb_cc1_pins>, <&usb_cc2_pins>;

You mention that the property of pinctrl not need in doc.
Do I need to add the property of pintrl in example?

> > +        nvmem-cells = <&otp_usb_cal>;
> > +        nvmem-cell-names = "usb-cal";
> 
> Type-c without connector? This is incomplete.

Ok, I will add connector.


Thanks,
Stanley