[PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller

Chris Packham posted 5 patches 2 months, 1 week ago
There is a newer version of this series
[PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
Posted by Chris Packham 2 months, 1 week ago
Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
controllers on this SoC are part of the "switch" block which is
represented here as a syscon node. The SCL pins are dependent on the I2C
controller (GPIO8 for the first controller, GPIO 17 for the second). The
SDA pins can be assigned to either one of the I2C controllers (but not
both).

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---

Notes:
    This does hit generate the following dt_binding_check warning
    
    realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
    
    Which is totally correct. I haven't given this thing a reg property
    because I'm using an offset from the parent syscon node. I'm also not
    calling the first offset "offset" but I don't think that'd help.
    
    I looked at a couple of other examples of devices that are children of
    syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
    a reg property in the dts but as far as I can see from the code it's not
    actually used, instead the register offsets are in the code looked up
    from the driver data (in at least one-case the reg offset is for a
    legacy usage).
    
    So I'm a little unsure what to do here. I can add a reg property and
    update the driver to use that to get the offset for the first set of
    registers (or just not use it). Or I could drop the @36c from the node
    names but then I coudn't distinguish the two controllers without failing
    the $nodename: requirement from i2c-controller.yaml.

 .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml

diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
new file mode 100644
index 000000000000..5b74a1986720
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL I2C Controller
+
+maintainers:
+  - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: |
+  The RTL9300 SoC has two I2C controllers. Each of these has an SCL line (which
+  if not-used for SCL can be a GPIO). There are 8 common SDA lines that can be
+  assigned to either I2C controller.
+
+properties:
+  compatible:
+    const: realtek,rtl9300-i2c
+
+  realtek,control-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset of the registers for this I2C controller
+
+  realtek,global-control-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Offset of the I2C global control register (common between
+      controllers).
+
+  clock-frequency:
+    enum: [ 100000, 400000 ]
+
+  realtek,sda-pin:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 7
+    description:
+      SDA pin associated with this I2C controller.
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+unevaluatedProperties: false
+
+required:
+  - compatible
+  - realtek,control-offset
+  - realtek,global-control-offset
+
+examples:
+  - |
+    switch@1b000000 {
+      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
+      reg = <0x1b000000 0x10000>;
+
+      i2c@36c {
+        compatible = "realtek,rtl9300-i2c";
+        realtek,control-offset = <0x36c>;
+        realtek,global-control-offset = <0x384>;
+        clock-frequency = <100000>;
+        realtek,sda-pin = <2>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+
+      i2c@388 {
+        compatible = "realtek,rtl9300-i2c";
+        realtek,control-offset = <0x388>;
+        realtek,global-control-offset = <0x384>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index f328373463b0..ccb1125444f4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19887,6 +19887,12 @@ S:	Maintained
 T:	git https://github.com/pkshih/rtw.git
 F:	drivers/net/wireless/realtek/rtl8xxxu/
 
+RTL9300 I2C DRIVER (rtl9300-i2c)
+M:	Chris Packham <chris.packham@alliedtelesis.co.nz>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
+
 RTRS TRANSPORT DRIVERS
 M:	Md. Haris Iqbal <haris.iqbal@ionos.com>
 M:	Jack Wang <jinpu.wang@ionos.com>
-- 
2.46.1
Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
Posted by Rob Herring 2 months, 1 week ago
On Wed, Sep 18, 2024 at 11:29:28AM +1200, Chris Packham wrote:
> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
> controllers on this SoC are part of the "switch" block which is
> represented here as a syscon node. The SCL pins are dependent on the I2C
> controller (GPIO8 for the first controller, GPIO 17 for the second). The
> SDA pins can be assigned to either one of the I2C controllers (but not
> both).
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     This does hit generate the following dt_binding_check warning
>     
>     realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
>     
>     Which is totally correct. I haven't given this thing a reg property
>     because I'm using an offset from the parent syscon node. I'm also not
>     calling the first offset "offset" but I don't think that'd help.
>     
>     I looked at a couple of other examples of devices that are children of
>     syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>     a reg property in the dts but as far as I can see from the code it's not
>     actually used, instead the register offsets are in the code looked up
>     from the driver data (in at least one-case the reg offset is for a
>     legacy usage).
>     
>     So I'm a little unsure what to do here. I can add a reg property and
>     update the driver to use that to get the offset for the first set of
>     registers (or just not use it). Or I could drop the @36c from the node
>     names but then I coudn't distinguish the two controllers without failing
>     the $nodename: requirement from i2c-controller.yaml.

Use 'reg'. Looks like you need 2 entries for it.

Whether a driver of some OS decides to use it or not is irrelevant to 
the binding.

> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> new file mode 100644
> index 000000000000..5b74a1986720
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL I2C Controller
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |

Don't need '|' if no formatting.

> +  The RTL9300 SoC has two I2C controllers. Each of these has an SCL line (which
> +  if not-used for SCL can be a GPIO). There are 8 common SDA lines that can be
> +  assigned to either I2C controller.
> +
> +properties:
> +  compatible:
> +    const: realtek,rtl9300-i2c
> +
> +  realtek,control-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset of the registers for this I2C controller
> +
> +  realtek,global-control-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Offset of the I2C global control register (common between
> +      controllers).
> +
> +  clock-frequency:
> +    enum: [ 100000, 400000 ]
> +
> +  realtek,sda-pin:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0

0 is already the minimum.

> +    maximum: 7
> +    description:
> +      SDA pin associated with this I2C controller.
> +
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +
> +unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - realtek,control-offset
> +  - realtek,global-control-offset
> +
> +examples:
> +  - |
> +    switch@1b000000 {
> +      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
> +      reg = <0x1b000000 0x10000>;
> +
> +      i2c@36c {
> +        compatible = "realtek,rtl9300-i2c";
> +        realtek,control-offset = <0x36c>;
> +        realtek,global-control-offset = <0x384>;
> +        clock-frequency = <100000>;
> +        realtek,sda-pin = <2>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +      };
> +
> +      i2c@388 {
> +        compatible = "realtek,rtl9300-i2c";
> +        realtek,control-offset = <0x388>;
> +        realtek,global-control-offset = <0x384>;

Humm, normally we don't want the same address in multiple 'reg' entries. 
Is this offset known to vary? It could just be hardcoded in the driver 
or implicit from the compatible (different compatible is the usual way 
to deal with differing register layouts).

Rob
Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
Posted by Chris Packham 2 months, 1 week ago
Hi Rob,

On 19/09/24 02:14, Rob Herring wrote:
> On Wed, Sep 18, 2024 at 11:29:28AM +1200, Chris Packham wrote:
>> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
>> controllers on this SoC are part of the "switch" block which is
>> represented here as a syscon node. The SCL pins are dependent on the I2C
>> controller (GPIO8 for the first controller, GPIO 17 for the second). The
>> SDA pins can be assigned to either one of the I2C controllers (but not
>> both).
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>
>> Notes:
>>      This does hit generate the following dt_binding_check warning
>>      
>>      realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
>>      
>>      Which is totally correct. I haven't given this thing a reg property
>>      because I'm using an offset from the parent syscon node. I'm also not
>>      calling the first offset "offset" but I don't think that'd help.
>>      
>>      I looked at a couple of other examples of devices that are children of
>>      syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>>      a reg property in the dts but as far as I can see from the code it's not
>>      actually used, instead the register offsets are in the code looked up
>>      from the driver data (in at least one-case the reg offset is for a
>>      legacy usage).
>>      
>>      So I'm a little unsure what to do here. I can add a reg property and
>>      update the driver to use that to get the offset for the first set of
>>      registers (or just not use it). Or I could drop the @36c from the node
>>      names but then I coudn't distinguish the two controllers without failing
>>      the $nodename: requirement from i2c-controller.yaml.
> Use 'reg'. Looks like you need 2 entries for it.
>
> Whether a driver of some OS decides to use it or not is irrelevant to
> the binding.

OK thanks for that clarification.

>>   .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>>   MAINTAINERS                                   |  6 ++
>>   2 files changed, 79 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>> new file mode 100644
>> index 000000000000..5b74a1986720
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
>> @@ -0,0 +1,73 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Realtek RTL I2C Controller
>> +
>> +maintainers:
>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>> +
>> +description: |
> Don't need '|' if no formatting.

Ack

>> +  The RTL9300 SoC has two I2C controllers. Each of these has an SCL line (which
>> +  if not-used for SCL can be a GPIO). There are 8 common SDA lines that can be
>> +  assigned to either I2C controller.
>> +
>> +properties:
>> +  compatible:
>> +    const: realtek,rtl9300-i2c
>> +
>> +  realtek,control-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Offset of the registers for this I2C controller
>> +
>> +  realtek,global-control-offset:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Offset of the I2C global control register (common between
>> +      controllers).
>> +
>> +  clock-frequency:
>> +    enum: [ 100000, 400000 ]
>> +
>> +  realtek,sda-pin:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
> 0 is already the minimum.
Ack
>> +    maximum: 7
>> +    description:
>> +      SDA pin associated with this I2C controller.
>> +
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +
>> +unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - realtek,control-offset
>> +  - realtek,global-control-offset
>> +
>> +examples:
>> +  - |
>> +    switch@1b000000 {
>> +      compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
>> +      reg = <0x1b000000 0x10000>;
>> +
>> +      i2c@36c {
>> +        compatible = "realtek,rtl9300-i2c";
>> +        realtek,control-offset = <0x36c>;
>> +        realtek,global-control-offset = <0x384>;
>> +        clock-frequency = <100000>;
>> +        realtek,sda-pin = <2>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +      };
>> +
>> +      i2c@388 {
>> +        compatible = "realtek,rtl9300-i2c";
>> +        realtek,control-offset = <0x388>;
>> +        realtek,global-control-offset = <0x384>;
> Humm, normally we don't want the same address in multiple 'reg' entries.
> Is this offset known to vary? It could just be hardcoded in the driver
> or implicit from the compatible (different compatible is the usual way
> to deal with differing register layouts).

It's one of those annoying registers that is sandwiched between the two 
I2C controllers that affects both of them so you kind of need to use it 
whether your using I2C1 or I2C2. For the RTL9300 it'll always be 0x384. 
There is a RTL9310 that appears to have it at a different offset but I'm 
not planning on adding support for that chip any time soon.

If I switch to "reg" for the I2C controller registers as suggested above 
I can just go with a hard coded register value in the driver for the 
global control.
Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
Posted by Rob Herring (Arm) 2 months, 1 week ago
On Wed, 18 Sep 2024 11:29:28 +1200, Chris Packham wrote:
> Add dtschema for the I2C controller on the RTL9300 SoC. The I2C
> controllers on this SoC are part of the "switch" block which is
> represented here as a syscon node. The SCL pins are dependent on the I2C
> controller (GPIO8 for the first controller, GPIO 17 for the second). The
> SDA pins can be assigned to either one of the I2C controllers (but not
> both).
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> 
> Notes:
>     This does hit generate the following dt_binding_check warning
> 
>     realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
> 
>     Which is totally correct. I haven't given this thing a reg property
>     because I'm using an offset from the parent syscon node. I'm also not
>     calling the first offset "offset" but I don't think that'd help.
> 
>     I looked at a couple of other examples of devices that are children of
>     syscon nodes (e.g. armada-ap806-thermal, ap806-cpu-clock) these do have
>     a reg property in the dts but as far as I can see from the code it's not
>     actually used, instead the register offsets are in the code looked up
>     from the driver data (in at least one-case the reg offset is for a
>     legacy usage).
> 
>     So I'm a little unsure what to do here. I can add a reg property and
>     update the driver to use that to get the offset for the first set of
>     registers (or just not use it). Or I could drop the @36c from the node
>     names but then I coudn't distinguish the two controllers without failing
>     the $nodename: requirement from i2c-controller.yaml.
> 
>  .../bindings/i2c/realtek,rtl9300-i2c.yaml     | 73 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dts:22.19-30.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@36c: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dts:32.19-38.13: Warning (unit_address_vs_reg): /example-0/switch@1b000000/i2c@388: node has a unit name, but no reg or ranges property
Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.example.dtb: /example-0/switch@1b000000: failed to match any schema with compatible: ['realtek,rtl9302c-switch', 'syscon', 'simple-mfd']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240917232932.3641992-2-chris.packham@alliedtelesis.co.nz

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.