Re: [PATCH 1/2] dt-bindings: i3c: add binding for Realtek RTS490x I3C HUB

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/04/2026 14:13, zain_zhou@realsil.com.cn wrote:
> From: zain_zhou <zain_zhou@realsil.com.cn>
> 
> Add DT binding schema for Realtek RTS490x series I3C HUB devices.

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> The binding describes configuration properties for:
>   - LDO enable/disable and voltage level per port group
>   - Pull-up resistance per port group
>   - IO driver strength per port
>   - Per target-port mode (I3C/SMBus/GPIO/disabled), pull-up,
>     IO mode, SMBus clock frequency and polling interval
>   - Hub network always-I3C mode
>   - Hardware identification via CSEL pin (id) and CP1 pins (id-cp1)
> 
> Signed-off-by: zain_zhou <zain_zhou@realsil.com.cn>

Don't use login name as full name, but rather latin transliteration.
Name in your native language would also be fine.

But login name is not fine.


> ---
>  .../bindings/i3c/realtek,rts490x-i3c-hub.yaml | 410 ++++++++++++++++++
>  MAINTAINERS                                   |   6 +
>  2 files changed, 416 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i3c/realtek,rts490x-i3c-hub.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i3c/realtek,rts490x-i3c-hub.yaml b/Documentation/devicetree/bindings/i3c/realtek,rts490x-i3c-hub.yaml
> new file mode 100644
> index 000000000000..30295eefee89
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i3c/realtek,rts490x-i3c-hub.yaml
> @@ -0,0 +1,410 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i3c/realtek,rts490x-i3c-hub.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: I3C HUB

Proper name here of your device


> +
> +maintainers:
> +  - zain_zhou <zain_zhou@realsil.com.cn>
> +
> +description: |
> +  I3C HUB is smart device which provides multiple functionality:


Are you describing particular device or I3C HUB?

> +  * enabling voltage compatibility across I3C Controller and Target devices,
> +  * bus capacitance isolation
> +  * address conflict isolation
> +  * I3C port expansion
> +  * two controllers in a single I3C bus
> +  * I3C and SMBus device compatibility
> +  * GPIO expansion
> +
> +  Having such big number of features, there is a need to have some DT knobs to tell the I3C HUB
> +  driver which features shall be enabled and how they shall be configured. I3C HUB driver read,

Please wrap code according to the preferred limit expressed in Kernel
coding style (checkpatch is not a coding style description, but only a
tool).  However don't wrap blindly (see Kernel coding style).

> +  validate DT knobs and set corresponding registers with the right way to satisfy user requests from
> +  DT.

Irrelevant. This is binding, so describe hardware. Not your drivers.

DT is irrelevant as well.

> +
> +  All the DT properties for I3C HUB are located under dedicated (for I3C HUB) DT entry. I3C HUB DT
> +  entry structure is aligned with regular I3C device DT entry described in i3c.yaml.
> +

Irrelevant. Describe your hardware.

> +allOf:
> +  - $ref: i3c.yaml#
> +
> +properties:
> +  $nodename:
> +    pattern: "^hub@0,0$"
> +
> +  cp0-ldo-en:
> +    enum:
> +      - disabled
> +      - enabled

Sorry, but what? Your binding does not look like accepted bindings AT
ALL. Please read writing bindings carefully or go via DTS101 slides.



> +    description: |
> +      I3C HUB Controller Port 0 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  cp1-ldo-en:
> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Controller Port 1 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  tp0145-ldo-en:
> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Target Ports 0/1/4/5 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  tp2367-ldo-en:
> +    enum:
> +      - disabled
> +      - enabled
> +    description: |
> +      I3C HUB Target Ports 2/3/6/7 LDO disabling/enabling setting. If enabled, voltage produced by
> +      on-die LDO will be available externally on dedicated pin. This option could be used to supply
> +      external pull-up resistors or for any other purpose which does not cross LDO capabilities.
> +
> +      This property is optional. If not provided, LDO will be disabled.
> +
> +  cp0-ldo-volt:

Nope. There are no such properties. Missing vendor prefix, missing
proper unit suffix. Look at other bindings.

> +    enum:
> +      - 1.0V
> +      - 1.1V
> +      - 1.2V
> +      - 1.8V
> +    description: |
> +      I3C HUB Controller Port 0 LDO setting to control the Controller Port 1 voltage level. This
> +      property is optional.
> +

...


> +
> +additionalProperties: true

No, you cannot have it. Please read writing bindings, writing schema or
open ANY other binding.

> +
> +examples:
> +  - |
> +    i3c-master@d040000 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      hub@0,0 {
> +        cp0-ldo-en = "disabled";
> +        cp1-ldo-en = "enabled";
> +        cp0-ldo-volt = "1.0V";
> +        cp1-ldo-volt = "1.1V";
> +        tp0145-ldo-en = "enabled";
> +        tp2367-ldo-en = "disabled";
> +        tp0145-ldo-volt = "1.2V";
> +        tp2367-ldo-volt = "1.8V";
> +        tp0145-pullup = "2k";
> +        tp2367-pullup = "500R";
> +        tp0145-io-strength = "50Ohms";
> +        tp2367-io-strength = "30Ohms";
> +        cp0-io-strength = "20Ohms";
> +        cp1-io-strength = "40Ohms";
> +
> +        target-port@0 {
> +          mode = "i3c";
> +          pullup = "enabled";
> +          always_enable;
> +        };
> +        target-port@1 {
> +          mode = "smbus";
> +          pullup = "enabled";
> +          clock-frequency = <1000000>;
> +          polling-interval-ms = <10>;
> +          backend@10{
> +              compatible = "i2c-slave-mqueue";
> +              reg = <(0x10 | I2C_OWN_SLAVE_ADDRESS)>;
> +          };
> +        };
> +        target-port@2 {
> +          mode = "gpio";
> +          pullup = "disabled";
> +        };
> +        target-port@3 {
> +          mode = "disabled";
> +          pullup = "disabled";
> +        };
> +      };
> +    };
> +
> +  - |
> +    i3c-master@d040000 {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      hub@70,3C000000100 {

No way it passes validation. You just said it CANNOT be anything else
than hub@0,0

> +        reg = <0x70 0x3C0 0x00000100>;

Lower case hex. Anyway, one example is enough.

> +        assigned-address = <0x70>;
> +        dcr = <0xC2>;
> +
> +        cp0-ldo-en = "disabled";
> +        cp1-ldo-en = "enabled";
> +        cp0-ldo-volt = "1.0V";
> +        cp1-ldo-volt = "1.1V";
> +        tp0145-ldo-en = "enabled";
> +        tp2367-ldo-en = "disabled";
> +        tp0145-ldo-volt = "1.2V";
> +        tp2367-ldo-volt = "1.8V";
> +        tp0145-pullup = "2k";
> +        tp2367-pullup = "500R";
> +        tp0145-io-strength = "50Ohms";
> +        tp2367-io-strength = "30Ohms";
> +        cp0-io-strength = "20Ohms";
> +        cp1-io-strength = "40Ohms";
> +
> +        target-port@0 {
> +          mode = "i3c";
> +          pullup = "enabled";
> +          always-enable;
> +        };
> +        target-port@1 {
> +          mode = "smbus";
> +          pullup = "enabled";
> +          backend@12{
> +              compatible = "i2c-slave-mqueue";
> +              reg = <(0x12 | I2C_OWN_SLAVE_ADDRESS)>;
> +          };
> +        };
> +        target-port@2 {
> +          mode = "gpio";
> +          pullup = "disabled";
> +        };
> +        target-port@3 {
> +          mode = "disabled";
> +          pullup = "disabled";
> +        };
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2fb1c75afd16..71ee5071ac0f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12214,6 +12214,12 @@ S:	Supported
>  F:	Documentation/devicetree/bindings/i3c/renesas,i3c.yaml
>  F:	drivers/i3c/master/renesas-i3c.c
>  
> +I3C HUB DRIVER FOR REALTEK RTS490X
> +M:	zain_zhou <zain_zhou@realsil.com.cn>
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/i3c/realtek,rts490x-i3c-hub.yaml
> +F:	drivers/staging/rts490x/

There is no such directory.



Best regards,
Krzysztof