* [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
2024-09-17 23:29 [PATCH 0/5] i2c: RTL9300 support Chris Packham
@ 2024-09-17 23:29 ` Chris Packham
2024-09-18 0:32 ` Rob Herring (Arm)
2024-09-18 14:14 ` Rob Herring
2024-09-17 23:29 ` [PATCH 2/5] i2c: Add driver for the " Chris Packham
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-17 23:29 UTC (permalink / raw)
To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham
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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
2024-09-17 23:29 ` [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
@ 2024-09-18 0:32 ` Rob Herring (Arm)
2024-09-18 14:14 ` Rob Herring
1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring (Arm) @ 2024-09-18 0:32 UTC (permalink / raw)
To: Chris Packham
Cc: conor+dt, krzk+dt, linux-mips, linux-kernel, tsbogend, linux-i2c,
andi.shyti, devicetree
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
2024-09-17 23:29 ` [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
2024-09-18 0:32 ` Rob Herring (Arm)
@ 2024-09-18 14:14 ` Rob Herring
2024-09-18 20:43 ` Chris Packham
1 sibling, 1 reply; 16+ messages in thread
From: Rob Herring @ 2024-09-18 14:14 UTC (permalink / raw)
To: Chris Packham
Cc: andi.shyti, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller
2024-09-18 14:14 ` Rob Herring
@ 2024-09-18 20:43 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-18 20:43 UTC (permalink / raw)
To: Rob Herring
Cc: andi.shyti, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller
2024-09-17 23:29 [PATCH 0/5] i2c: RTL9300 support Chris Packham
2024-09-17 23:29 ` [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
@ 2024-09-17 23:29 ` Chris Packham
2024-09-18 20:27 ` Andi Shyti
2024-09-17 23:29 ` [PATCH 3/5] mips: dts: realtek: Add I2C controllers Chris Packham
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Chris Packham @ 2024-09-17 23:29 UTC (permalink / raw)
To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham
Add support for the I2C controller on the RTL9300 SoC. This is based on
the openwrt implementation[1] but cleaned up to make use of the regmap
APIs.
[1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
MAINTAINERS | 1 +
drivers/i2c/busses/Kconfig | 10 +
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-rtl9300.c | 377 +++++++++++++++++++++++++++++++
4 files changed, 389 insertions(+)
create mode 100644 drivers/i2c/busses/i2c-rtl9300.c
diff --git a/MAINTAINERS b/MAINTAINERS
index ccb1125444f4..9e123e9839a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19892,6 +19892,7 @@ 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
+F: drivers/i2c/busses/i2c-rtl9300.c
RTRS TRANSPORT DRIVERS
M: Md. Haris Iqbal <haris.iqbal@ionos.com>
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a22f9125322a..927b583002c7 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1041,6 +1041,16 @@ config I2C_RK3X
This driver can also be built as a module. If so, the module will
be called i2c-rk3x.
+config I2C_RTL9300
+ tristate "Realtek RTL9300 I2C controller"
+ depends on MACH_REALTEK_RTL || COMPILE_TEST
+ help
+ Say Y here to include support for the I2C controller in Realtek
+ RTL9300 SoCs.
+
+ This driver can also be built as a module. If so, the module will
+ be called i2c-rtl9300.
+
config I2C_RZV2M
tristate "Renesas RZ/V2M adapter"
depends on ARCH_RENESAS || COMPILE_TEST
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 78d0561339e5..ac2f9f22803c 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -102,6 +102,7 @@ obj-$(CONFIG_I2C_QCOM_GENI) += i2c-qcom-geni.o
obj-$(CONFIG_I2C_QUP) += i2c-qup.o
obj-$(CONFIG_I2C_RIIC) += i2c-riic.o
obj-$(CONFIG_I2C_RK3X) += i2c-rk3x.o
+obj-$(CONFIG_I2C_RTL9300) += i2c-rtl9300.o
obj-$(CONFIG_I2C_RZV2M) += i2c-rzv2m.o
obj-$(CONFIG_I2C_S3C2410) += i2c-s3c2410.o
obj-$(CONFIG_I2C_SH7760) += i2c-sh7760.o
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
new file mode 100644
index 000000000000..f16e9b6343bf
--- /dev/null
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+struct rtl9300_i2c {
+ struct regmap *regmap;
+ struct device *dev;
+ struct i2c_adapter adap;
+ u32 i2c_mst_ofs;
+ u32 i2c_mst_glb_ofs;
+ u8 bus_freq;
+ u8 sda_pin;
+};
+
+#define I2C_MST_CTRL1 0x0
+#define MEM_ADDR_OFS 8
+#define MEM_ADDR_MASK 0xffffff
+#define SDA_OUT_SEL_OFS 4
+#define SDA_OUT_SEL_MASK 0x7
+#define GPIO_SCL_SEL BIT(3)
+#define RWOP BIT(2)
+#define I2C_FAIL BIT(1)
+#define I2C_TRIG BIT(0)
+#define I2C_MST_CTRL2 0x4
+#define RD_MODE BIT(15)
+#define DEV_ADDR_OFS 8
+#define DEV_ADDR_MASK 0x7f
+#define DATA_WIDTH_OFS 4
+#define DATA_WIDTH_MASK 0xf
+#define MEM_ADDR_WIDTH_OFS 2
+#define MEM_ADDR_WIDTH_MASK 0x3
+#define SCL_FREQ_OFS 0
+#define SCL_FREQ_MASK 0x3
+#define I2C_MST_DATA_WORD0 0x8
+#define I2C_MST_DATA_WORD1 0xc
+#define I2C_MST_DATA_WORD2 0x10
+#define I2C_MST_DATA_WORD3 0x14
+
+#define RTL9300_I2C_STD_FREQ 0
+#define RTL9300_I2C_FAST_FREQ 1
+
+DEFINE_MUTEX(i2c_lock);
+
+static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
+{
+ u32 val, mask;
+ int ret;
+
+ val = len << MEM_ADDR_WIDTH_OFS;
+ mask = MEM_ADDR_WIDTH_MASK << MEM_ADDR_WIDTH_OFS;
+
+ ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL2, mask, val);
+ if (ret)
+ return ret;
+
+ val = reg << MEM_ADDR_OFS;
+ mask = MEM_ADDR_MASK << MEM_ADDR_OFS;
+
+ ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rtl9300_i2c_config_io(struct rtl9300_i2c *i2c, u8 sda_pin)
+{
+ int ret;
+
+ ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_glb_ofs, BIT(sda_pin), BIT(sda_pin));
+ if (ret)
+ return ret;
+
+ ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
+ (SDA_OUT_SEL_MASK << SDA_OUT_SEL_OFS) | GPIO_SCL_SEL,
+ (sda_pin << SDA_OUT_SEL_OFS) | GPIO_SCL_SEL);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rtl9300_i2c_config_xfer(struct rtl9300_i2c *i2c, u16 addr, u16 len)
+{
+ u32 val, mask;
+
+ val = i2c->bus_freq << SCL_FREQ_OFS;
+ mask = SCL_FREQ_MASK << SCL_FREQ_OFS;
+
+ val |= addr << DEV_ADDR_OFS;
+ mask |= DEV_ADDR_MASK << DEV_ADDR_OFS;
+
+ val |= ((len - 1) & 0xf) << DATA_WIDTH_OFS;
+ mask |= DATA_WIDTH_MASK << DATA_WIDTH_OFS;
+
+ mask |= RD_MODE;
+
+ return regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL2, mask, val);
+}
+
+static int rtl9300_i2c_read(struct rtl9300_i2c *i2c, u8 *buf, int len)
+{
+ u32 vals[4] = {};
+ int i, ret;
+
+ if (len > 16)
+ return -EIO;
+
+ ret = regmap_bulk_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
+ vals, ARRAY_SIZE(vals));
+ if (ret)
+ return ret;
+
+ for (i = 0; i < len; i++) {
+ buf[i] = vals[i/4] & 0xff;
+ vals[i/4] >>= 8;
+ }
+
+ return len;
+}
+
+static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
+{
+ u32 vals[4] = {};
+ int i, ret;
+
+ if (len > 16)
+ return -EIO;
+
+ for (i = 0; i < len; i++) {
+ if (i % 4 == 0)
+ vals[i/4] = 0;
+ vals[i/4] <<= 8;
+ vals[i/4] |= buf[i];
+ }
+
+ ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
+ vals, ARRAY_SIZE(vals));
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
+{
+ int ret;
+
+ ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
+ int size, union i2c_smbus_data *data, int len)
+{
+ u32 val, mask;
+ int ret;
+
+ if (read_write == I2C_SMBUS_READ)
+ val = 0;
+ else
+ val = RWOP;
+ mask = RWOP;
+
+ val |= I2C_TRIG;
+ mask |= I2C_TRIG;
+
+ ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
+ if (ret)
+ return ret;
+
+ ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
+ val, !(val & I2C_TRIG), 100, 2000);
+ if (ret)
+ return ret;
+
+ if (val & I2C_FAIL)
+ return -EIO;
+
+ if (read_write == I2C_SMBUS_READ) {
+ if (size == I2C_SMBUS_BYTE || size == I2C_SMBUS_BYTE_DATA) {
+ ret = regmap_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, &val);
+ if (ret)
+ return ret;
+ data->byte = val & 0xff;
+ } else if (size == I2C_SMBUS_WORD_DATA) {
+ ret = regmap_read(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, &val);
+ if (ret)
+ return ret;
+ data->word = val & 0xffff;
+ } else {
+ rtl9300_i2c_read(i2c, &data->block[0], len);
+ }
+ }
+
+ return 0;
+}
+
+static int rtl9300_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
+ unsigned short flags, char read_write,
+ u8 command, int size, union i2c_smbus_data *data)
+{
+ struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+ int len = 0, ret;
+
+ mutex_lock(&i2c_lock);
+ switch (size) {
+ case I2C_SMBUS_QUICK:
+ rtl9300_i2c_config_xfer(i2c, addr, 0);
+ rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+ break;
+
+ case I2C_SMBUS_BYTE:
+ if (read_write == I2C_SMBUS_WRITE) {
+ rtl9300_i2c_config_xfer(i2c, addr, 0);
+ rtl9300_i2c_reg_addr_set(i2c, command, 1);
+ } else {
+ rtl9300_i2c_config_xfer(i2c, addr, 1);
+ rtl9300_i2c_reg_addr_set(i2c, 0, 0);
+ }
+ break;
+
+ case I2C_SMBUS_BYTE_DATA:
+ rtl9300_i2c_reg_addr_set(i2c, command, 1);
+ rtl9300_i2c_config_xfer(i2c, addr, 1);
+ if (read_write == I2C_SMBUS_WRITE)
+ rtl9300_i2c_writel(i2c, data->byte);
+ break;
+
+ case I2C_SMBUS_WORD_DATA:
+ rtl9300_i2c_reg_addr_set(i2c, command, 1);
+ rtl9300_i2c_config_xfer(i2c, addr, 2);
+ if (read_write == I2C_SMBUS_WRITE)
+ rtl9300_i2c_writel(i2c, data->word);
+ break;
+
+ case I2C_SMBUS_BLOCK_DATA:
+ rtl9300_i2c_reg_addr_set(i2c, command, 1);
+ rtl9300_i2c_config_xfer(i2c, addr, data->block[0]);
+ if (read_write == I2C_SMBUS_WRITE)
+ rtl9300_i2c_write(i2c, &data->block[1], data->block[0]);
+ len = data->block[0];
+ break;
+
+ default:
+ dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
+ ret = -EOPNOTSUPP;
+ goto out_unlock;
+ }
+
+ ret = rtl9300_i2c_execute_xfer(i2c, read_write, size, data, len);
+
+out_unlock:
+ mutex_unlock(&i2c_lock);
+
+ return ret;
+}
+
+static u32 rtl9300_i2c_func(struct i2c_adapter *a)
+{
+ return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
+ I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
+ I2C_FUNC_SMBUS_BLOCK_DATA;
+}
+
+static const struct i2c_algorithm rtl9300_i2c_algo = {
+ .smbus_xfer = rtl9300_i2c_smbus_xfer,
+ .functionality = rtl9300_i2c_func,
+};
+
+struct i2c_adapter_quirks rtl9300_i2c_quirks = {
+ .flags = I2C_AQ_NO_CLK_STRETCH,
+ .max_read_len = 16,
+ .max_write_len = 16,
+};
+
+static int rtl9300_i2c_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rtl9300_i2c *i2c;
+ struct i2c_adapter *adap;
+ u32 clock_freq, sda_pin;
+ int ret;
+
+ i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+ if (!i2c)
+ return -ENOMEM;
+
+ i2c->regmap = syscon_node_to_regmap(dev->parent->of_node);
+ if (IS_ERR(i2c->regmap))
+ return PTR_ERR(i2c->regmap);
+ i2c->dev = dev;
+
+ ret = device_property_read_u32(dev, "realtek,control-offset", &i2c->i2c_mst_ofs);
+ if (ret)
+ return ret;
+
+ ret = device_property_read_u32(dev, "realtek,global-control-offset", &i2c->i2c_mst_glb_ofs);
+ if (ret)
+ return ret;
+
+ ret = device_property_read_u32(dev, "clock-frequency", &clock_freq);
+ if (ret)
+ clock_freq = I2C_MAX_STANDARD_MODE_FREQ;
+
+ switch (clock_freq) {
+ case I2C_MAX_STANDARD_MODE_FREQ:
+ i2c->bus_freq = RTL9300_I2C_STD_FREQ;
+ break;
+
+ case I2C_MAX_FAST_MODE_FREQ:
+ i2c->bus_freq = RTL9300_I2C_FAST_FREQ;
+ break;
+ default:
+ dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
+ return -EINVAL;
+ }
+
+ ret = device_property_read_u32(dev, "realtek,sda-pin", &sda_pin);
+ if (ret)
+ sda_pin = 0;
+ i2c->sda_pin = sda_pin;
+
+ adap = &i2c->adap;
+ adap->owner = THIS_MODULE;
+ adap->algo = &rtl9300_i2c_algo;
+ adap->quirks = &rtl9300_i2c_quirks;
+ adap->retries = 3;
+ adap->dev.parent = dev;
+ i2c_set_adapdata(adap, i2c);
+ adap->dev.of_node = dev->of_node;
+ strscpy(adap->name, dev_name(dev), sizeof(adap->name));
+
+ platform_set_drvdata(pdev, i2c);
+
+ ret = rtl9300_i2c_config_io(i2c, i2c->sda_pin);
+ if (ret)
+ return ret;
+
+ return i2c_add_adapter(adap);
+}
+
+static void rtl9300_i2c_remove(struct platform_device *pdev)
+{
+ struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
+
+ i2c_del_adapter(&i2c->adap);
+}
+
+static const struct of_device_id i2c_rtl9300_dt_ids[] = {
+ { .compatible = "realtek,rtl9300-i2c" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
+
+static struct platform_driver rtl9300_i2c_driver = {
+ .probe = rtl9300_i2c_probe,
+ .remove = rtl9300_i2c_remove,
+ .driver = {
+ .name = "i2c-rtl9300",
+ .of_match_table = i2c_rtl9300_dt_ids,
+ },
+};
+
+module_platform_driver(rtl9300_i2c_driver);
+
+MODULE_DESCRIPTION("RTL9300 I2C controller driver");
+MODULE_LICENSE("GPL");
+
--
2.46.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller
2024-09-17 23:29 ` [PATCH 2/5] i2c: Add driver for the " Chris Packham
@ 2024-09-18 20:27 ` Andi Shyti
2024-09-18 21:18 ` Chris Packham
0 siblings, 1 reply; 16+ messages in thread
From: Andi Shyti @ 2024-09-18 20:27 UTC (permalink / raw)
To: Chris Packham
Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
Hi Chris,
On Wed, Sep 18, 2024 at 11:29:29AM GMT, Chris Packham wrote:
> Add support for the I2C controller on the RTL9300 SoC. This is based on
> the openwrt implementation[1] but cleaned up to make use of the regmap
> APIs.
Can you please add a few more words to describe the device?
> [1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
...
> +#define I2C_MST_CTRL1 0x0
> +#define MEM_ADDR_OFS 8
> +#define MEM_ADDR_MASK 0xffffff
> +#define SDA_OUT_SEL_OFS 4
> +#define SDA_OUT_SEL_MASK 0x7
> +#define GPIO_SCL_SEL BIT(3)
> +#define RWOP BIT(2)
> +#define I2C_FAIL BIT(1)
> +#define I2C_TRIG BIT(0)
> +#define I2C_MST_CTRL2 0x4
> +#define RD_MODE BIT(15)
> +#define DEV_ADDR_OFS 8
> +#define DEV_ADDR_MASK 0x7f
> +#define DATA_WIDTH_OFS 4
> +#define DATA_WIDTH_MASK 0xf
> +#define MEM_ADDR_WIDTH_OFS 2
> +#define MEM_ADDR_WIDTH_MASK 0x3
can we have these masked already shifted? You could use
GENMASK().
> +#define SCL_FREQ_OFS 0
> +#define SCL_FREQ_MASK 0x3
> +#define I2C_MST_DATA_WORD0 0x8
> +#define I2C_MST_DATA_WORD1 0xc
> +#define I2C_MST_DATA_WORD2 0x10
> +#define I2C_MST_DATA_WORD3 0x14
Can we use a prefix for all these defines?
> +
> +#define RTL9300_I2C_STD_FREQ 0
> +#define RTL9300_I2C_FAST_FREQ 1
This can also be an enum.
> +
> +DEFINE_MUTEX(i2c_lock);
...
> +static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
> +{
> + u32 vals[4] = {};
> + int i, ret;
> +
> + if (len > 16)
> + return -EIO;
> +
> + for (i = 0; i < len; i++) {
> + if (i % 4 == 0)
> + vals[i/4] = 0;
> + vals[i/4] <<= 8;
> + vals[i/4] |= buf[i];
> + }
> +
> + ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
> + vals, ARRAY_SIZE(vals));
> + if (ret)
> + return ret;
> +
> + return len;
why returning "len"? And in any case this is ignored.
> +}
> +
> +static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
> +{
> + int ret;
> +
> + ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_write(...) ?
In any case, the returned value of these functions is completely
ignored, not even printed. Should we either:
- condier the return value in the _xfer() functions
or
- make all these functions void?
> +}
> +
> +static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
> + int size, union i2c_smbus_data *data, int len)
> +{
> + u32 val, mask;
> + int ret;
> +
> + if (read_write == I2C_SMBUS_READ)
> + val = 0;
> + else
> + val = RWOP;
> + mask = RWOP;
> +
> + val |= I2C_TRIG;
> + mask |= I2C_TRIG;
how about "mask = RWOP | I2C_TRIG" to make it in one line?
Also val can be simplified as:
val = I2C_TRIG;
if (read_write == I2C_SMBUS_WRITE)
val |= RWOP;
Not a binding commeent, as you wish.
> +
> + ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
> + val, !(val & I2C_TRIG), 100, 2000);
> + if (ret)
> + return ret;
> +
> + if (val & I2C_FAIL)
where is val taking taking this bit?
> + return -EIO;
> +
...
> + switch (size) {
> + case I2C_SMBUS_QUICK:
...
> + case I2C_SMBUS_BYTE:
...
> + case I2C_SMBUS_BYTE_DATA:
...
> + case I2C_SMBUS_WORD_DATA:
...
> + case I2C_SMBUS_BLOCK_DATA:
...
> + default:
> + dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
dev_err() ?
> + ret = -EOPNOTSUPP;
> + goto out_unlock;
> + }
...
> + switch (clock_freq) {
> + case I2C_MAX_STANDARD_MODE_FREQ:
...
> + case I2C_MAX_FAST_MODE_FREQ:
...
> + default:
> + dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
> + return -EINVAL;
If we are returning an error we should print an error, let's make
it a "return dev_err_probe()"
But, I was thinking that by default we can assign
I2C_MAX_STANDARD_MODE_FREQ and if the DTS defines a different
frequency we could just print an error and stick to the default
value. Makes sense?
> + }
...
> + return i2c_add_adapter(adap);
return devm_i2c_add_adapter(adap);
and the remove function is not needed.
> +}
> +
> +static void rtl9300_i2c_remove(struct platform_device *pdev)
> +{
> + struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
> +
> + i2c_del_adapter(&i2c->adap);
> +}
> +
> +static const struct of_device_id i2c_rtl9300_dt_ids[] = {
> + { .compatible = "realtek,rtl9300-i2c" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
> +
> +static struct platform_driver rtl9300_i2c_driver = {
> + .probe = rtl9300_i2c_probe,
> + .remove = rtl9300_i2c_remove,
> + .driver = {
> + .name = "i2c-rtl9300",
> + .of_match_table = i2c_rtl9300_dt_ids,
> + },
> +};
> +
> +module_platform_driver(rtl9300_i2c_driver);
> +
> +MODULE_DESCRIPTION("RTL9300 I2C controller driver");
> +MODULE_LICENSE("GPL");
> +
Just a trailing blank line here.
Thanks,
Andi
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/5] i2c: Add driver for the RTL9300 I2C controller
2024-09-18 20:27 ` Andi Shyti
@ 2024-09-18 21:18 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-18 21:18 UTC (permalink / raw)
To: Andi Shyti
Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
Hi Andy,
On 19/09/24 08:27, Andi Shyti wrote:
> Hi Chris,
>
> On Wed, Sep 18, 2024 at 11:29:29AM GMT, Chris Packham wrote:
>> Add support for the I2C controller on the RTL9300 SoC. This is based on
>> the openwrt implementation[1] but cleaned up to make use of the regmap
>> APIs.
> Can you please add a few more words to describe the device?
Sure will do.
>> [1] - https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/realtek/files-5.15/drivers/i2c/busses/i2c-rtl9300.c
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ...
>
>> +#define I2C_MST_CTRL1 0x0
>> +#define MEM_ADDR_OFS 8
>> +#define MEM_ADDR_MASK 0xffffff
>> +#define SDA_OUT_SEL_OFS 4
>> +#define SDA_OUT_SEL_MASK 0x7
>> +#define GPIO_SCL_SEL BIT(3)
>> +#define RWOP BIT(2)
>> +#define I2C_FAIL BIT(1)
>> +#define I2C_TRIG BIT(0)
>> +#define I2C_MST_CTRL2 0x4
>> +#define RD_MODE BIT(15)
>> +#define DEV_ADDR_OFS 8
>> +#define DEV_ADDR_MASK 0x7f
>> +#define DATA_WIDTH_OFS 4
>> +#define DATA_WIDTH_MASK 0xf
>> +#define MEM_ADDR_WIDTH_OFS 2
>> +#define MEM_ADDR_WIDTH_MASK 0x3
> can we have these masked already shifted? You could use
> GENMASK().
I'll take a look.
>> +#define SCL_FREQ_OFS 0
>> +#define SCL_FREQ_MASK 0x3
>> +#define I2C_MST_DATA_WORD0 0x8
>> +#define I2C_MST_DATA_WORD1 0xc
>> +#define I2C_MST_DATA_WORD2 0x10
>> +#define I2C_MST_DATA_WORD3 0x14
> Can we use a prefix for all these defines?
Yes will add "RTL9300_".
I assume for the bit values too? So something like "MEM_ADDR_OFS"
becomes "RTL9300_I2C_MST_CTRL1_MEM_ADDR_OFS" is that OK or too verbose?
>> +
>> +#define RTL9300_I2C_STD_FREQ 0
>> +#define RTL9300_I2C_FAST_FREQ 1
> This can also be an enum.
Ack
>
>> +
>> +DEFINE_MUTEX(i2c_lock);
> ...
>
>> +static int rtl9300_i2c_write(struct rtl9300_i2c *i2c, u8 *buf, int len)
>> +{
>> + u32 vals[4] = {};
>> + int i, ret;
>> +
>> + if (len > 16)
>> + return -EIO;
>> +
>> + for (i = 0; i < len; i++) {
>> + if (i % 4 == 0)
>> + vals[i/4] = 0;
>> + vals[i/4] <<= 8;
>> + vals[i/4] |= buf[i];
>> + }
>> +
>> + ret = regmap_bulk_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0,
>> + vals, ARRAY_SIZE(vals));
>> + if (ret)
>> + return ret;
>> +
>> + return len;
> why returning "len"? And in any case this is ignored.
I copied that behaviour from the openwrt driver. I think making it the
same as the other functions would make more sense.
>
>> +}
>> +
>> +static int rtl9300_i2c_writel(struct rtl9300_i2c *i2c, u32 data)
>> +{
>> + int ret;
>> +
>> + ret = regmap_write(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_DATA_WORD0, data);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
> return regmap_write(...) ?
>
> In any case, the returned value of these functions is completely
> ignored, not even printed. Should we either:
>
> - condier the return value in the _xfer() functions
> or
> - make all these functions void?
I suppose it's a bit academic. Under the hood it's mmio so it's not as
if it can really fail (famous last words). That said, this switch chip
can be run in a core disabled mode and you could then in theory be doing
I2C over SPI from an external SoC. If someone were just naively updating
a hardware design to add the external SoC they might neglect to move the
I2C connections. It's also just good practice so I'll propagate the
returns up to the _xfer().
>> +}
>> +
>> +static int rtl9300_i2c_execute_xfer(struct rtl9300_i2c *i2c, char read_write,
>> + int size, union i2c_smbus_data *data, int len)
>> +{
>> + u32 val, mask;
>> + int ret;
>> +
>> + if (read_write == I2C_SMBUS_READ)
>> + val = 0;
>> + else
>> + val = RWOP;
>> + mask = RWOP;
>> +
>> + val |= I2C_TRIG;
>> + mask |= I2C_TRIG;
> how about "mask = RWOP | I2C_TRIG" to make it in one line?
>
> Also val can be simplified as:
>
> val = I2C_TRIG;
> if (read_write == I2C_SMBUS_WRITE)
> val |= RWOP;
>
> Not a binding commeent, as you wish.
I'll take a look. I kind of did like the pairing of val and mask for
each thing being set.
>> +
>> + ret = regmap_update_bits(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1, mask, val);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_read_poll_timeout(i2c->regmap, i2c->i2c_mst_ofs + I2C_MST_CTRL1,
>> + val, !(val & I2C_TRIG), 100, 2000);
>> + if (ret)
>> + return ret;
>> +
>> + if (val & I2C_FAIL)
> where is val taking taking this bit?
In the regmap_read_poll_timeout().
>
>> + return -EIO;
>> +
> ...
>
>> + switch (size) {
>> + case I2C_SMBUS_QUICK:
> ...
>> + case I2C_SMBUS_BYTE:
> ...
>> + case I2C_SMBUS_BYTE_DATA:
> ...
>> + case I2C_SMBUS_WORD_DATA:
> ...
>> + case I2C_SMBUS_BLOCK_DATA:
> ...
>> + default:
>> + dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> dev_err() ?
Ack.
>> + ret = -EOPNOTSUPP;
>> + goto out_unlock;
>> + }
> ...
>
>> + switch (clock_freq) {
>> + case I2C_MAX_STANDARD_MODE_FREQ:
> ...
>> + case I2C_MAX_FAST_MODE_FREQ:
> ...
>> + default:
>> + dev_warn(i2c->dev, "clock-frequency %d not supported\n", clock_freq);
>> + return -EINVAL;
> If we are returning an error we should print an error, let's make
> it a "return dev_err_probe()"
>
> But, I was thinking that by default we can assign
> I2C_MAX_STANDARD_MODE_FREQ and if the DTS defines a different
> frequency we could just print an error and stick to the default
> value. Makes sense?
I don't have a strong opinion. Failing the probe just because something
in the dts is wrong where we can have a sane default does seem overly
harsh. On the other hand I've had hardware QA folks complain when the
I2C bus is running at 98khz instead of 100khz.
>
>> + }
> ...
>
>> + return i2c_add_adapter(adap);
> return devm_i2c_add_adapter(adap);
>
> and the remove function is not needed.
OK thanks. I did look for a devm variant but obviously not hard enough.
>> +}
>> +
>> +static void rtl9300_i2c_remove(struct platform_device *pdev)
>> +{
>> + struct rtl9300_i2c *i2c = platform_get_drvdata(pdev);
>> +
>> + i2c_del_adapter(&i2c->adap);
>> +}
>> +
>> +static const struct of_device_id i2c_rtl9300_dt_ids[] = {
>> + { .compatible = "realtek,rtl9300-i2c" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, i2c_rtl9300_dt_ids);
>> +
>> +static struct platform_driver rtl9300_i2c_driver = {
>> + .probe = rtl9300_i2c_probe,
>> + .remove = rtl9300_i2c_remove,
>> + .driver = {
>> + .name = "i2c-rtl9300",
>> + .of_match_table = i2c_rtl9300_dt_ids,
>> + },
>> +};
>> +
>> +module_platform_driver(rtl9300_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>> +MODULE_LICENSE("GPL");
>> +
> Just a trailing blank line here.
Ack.
>
> Thanks,
> Andi
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/5] mips: dts: realtek: Add I2C controllers
2024-09-17 23:29 [PATCH 0/5] i2c: RTL9300 support Chris Packham
2024-09-17 23:29 ` [PATCH 1/5] dt-bindings: i2c: Add RTL9300 I2C controller Chris Packham
2024-09-17 23:29 ` [PATCH 2/5] i2c: Add driver for the " Chris Packham
@ 2024-09-17 23:29 ` Chris Packham
2024-09-17 23:29 ` [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer Chris Packham
2024-09-17 23:29 ` [PATCH 5/5] i2c: rtl9300: Add multiplexing support Chris Packham
4 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-17 23:29 UTC (permalink / raw)
To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham
Add the I2C controllers that are part of the RTL9300 SoC.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
arch/mips/boot/dts/realtek/rtl930x.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/mips/boot/dts/realtek/rtl930x.dtsi b/arch/mips/boot/dts/realtek/rtl930x.dtsi
index cf1b38b6c353..c579665df700 100644
--- a/arch/mips/boot/dts/realtek/rtl930x.dtsi
+++ b/arch/mips/boot/dts/realtek/rtl930x.dtsi
@@ -39,6 +39,24 @@ reboot {
offset = <0x0c>;
value = <0x01>;
};
+
+ i2c0: i2c@36c {
+ compatible = "realtek,rtl9300-i2c";
+ realtek,control-offset = <0x36c>;
+ realtek,global-control-offset = <0x384>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+
+ i2c1: i2c@388 {
+ compatible = "realtek,rtl9300-i2c";
+ realtek,control-offset = <0x388>;
+ realtek,global-control-offset = <0x384>;
+ status = "disabled";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
};
};
--
2.46.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer
2024-09-17 23:29 [PATCH 0/5] i2c: RTL9300 support Chris Packham
` (2 preceding siblings ...)
2024-09-17 23:29 ` [PATCH 3/5] mips: dts: realtek: Add I2C controllers Chris Packham
@ 2024-09-17 23:29 ` Chris Packham
2024-09-18 0:32 ` Rob Herring (Arm)
2024-09-18 14:25 ` Rob Herring
2024-09-17 23:29 ` [PATCH 5/5] i2c: rtl9300: Add multiplexing support Chris Packham
4 siblings, 2 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-17 23:29 UTC (permalink / raw)
To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham
An extension of the RTL9300 SoC is to support multiplexing by selecting
the SDA pins that are being used dynamically. Add a binding that allows
us to describe hardware that makes use of this.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
.../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 83 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
new file mode 100644
index 000000000000..a64879d0fda7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL I2C Multiplexer
+
+maintainers:
+ - Chris Packham <chris.packham@alliedtelesis.co.nz>
+
+description: |
+ The I2C controllers on the RTL9300 support a level of multiplexing. In the
+ simple case the rtl9300-i2c binding can provide a single SDA pin per
+ controller. This binding allows a more than one SDA line to be used per
+ controller providing a level of multiplexing.
+
+properties:
+ compatible:
+ const: realtek,rtl9300-i2c-mux
+
+ i2c-parent:
+ description: phandle of the I2C bus controller that this multiplexer
+ operates on.
+ $ref: /schemas/types.yaml#/definitions/phandle
+
+allOf:
+ - $ref: i2c-mux.yaml
+
+unevaluatedProperties: false
+
+required:
+ - compatible
+ - i2c-parent
+
+examples:
+ - |
+ switch@1b000000 {
+ compatible = "realtek,rtl9302c-switch", "syscon", "simple-mfd";
+ reg = <0x1b000000 0x10000>;
+
+ i2c0: i2c@36c {
+ compatible = "realtek,rtl9300-i2c";
+ realtek,control-offset = <0x36c>;
+ realtek,global-control-offset = <0x384>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
+
+ base {
+ i2c-mux {
+ compatible = "realtek,rtl9300-i2c-mux";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ i2c-parent = <&i2c0>;
+
+ i2c@0 {
+ reg = <0>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ gpio@20 {
+ compatible = "nxp,pca9555";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x20>;
+ };
+ };
+
+ i2c@2 {
+ reg = <2>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ gpio@20 {
+ compatible = "nxp,pca9555";
+ gpio-controller;
+ #gpio-cells = <2>;
+ reg = <0x20>;
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9e123e9839a5..178ac8a7e843 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19891,6 +19891,7 @@ 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-mux.yaml
F: Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c.yaml
F: drivers/i2c/busses/i2c-rtl9300.c
--
2.46.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer
2024-09-17 23:29 ` [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer Chris Packham
@ 2024-09-18 0:32 ` Rob Herring (Arm)
2024-09-18 14:25 ` Rob Herring
1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring (Arm) @ 2024-09-18 0:32 UTC (permalink / raw)
To: Chris Packham
Cc: krzk+dt, linux-i2c, linux-mips, devicetree, linux-kernel,
tsbogend, andi.shyti, conor+dt
On Wed, 18 Sep 2024 11:29:31 +1200, Chris Packham wrote:
> An extension of the RTL9300 SoC is to support multiplexing by selecting
> the SDA pins that are being used dynamically. Add a binding that allows
> us to describe hardware that makes use of this.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 83 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.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-mux.example.dts:22.25-28.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-mux.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-5-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.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer
2024-09-17 23:29 ` [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer Chris Packham
2024-09-18 0:32 ` Rob Herring (Arm)
@ 2024-09-18 14:25 ` Rob Herring
1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring @ 2024-09-18 14:25 UTC (permalink / raw)
To: Chris Packham
Cc: andi.shyti, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
On Wed, Sep 18, 2024 at 11:29:31AM +1200, Chris Packham wrote:
> An extension of the RTL9300 SoC is to support multiplexing by selecting
> the SDA pins that are being used dynamically. Add a binding that allows
> us to describe hardware that makes use of this.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> .../bindings/i2c/realtek,rtl9300-i2c-mux.yaml | 82 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 83 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
>
> diff --git a/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> new file mode 100644
> index 000000000000..a64879d0fda7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/realtek,rtl9300-i2c-mux.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/realtek,rtl9300-i2c-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL I2C Multiplexer
> +
> +maintainers:
> + - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +description: |
> + The I2C controllers on the RTL9300 support a level of multiplexing. In the
> + simple case the rtl9300-i2c binding can provide a single SDA pin per
> + controller. This binding allows a more than one SDA line to be used per
> + controller providing a level of multiplexing.
> +
> +properties:
> + compatible:
> + const: realtek,rtl9300-i2c-mux
> +
> + i2c-parent:
> + description: phandle of the I2C bus controller that this multiplexer
> + operates on.
> + $ref: /schemas/types.yaml#/definitions/phandle
The mux isn't a separate device, so I think this should just be part of
the i2c parent:
i2c-mux@36c {
i2c@0 {
...
};
i2c@1 {
...
};
...
};
And then you can get rid of the SDA pin property. If you only use 1 pin,
then there is just 1 'i2c' child node with an address matching the SDA
pin.
Rob
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 5/5] i2c: rtl9300: Add multiplexing support
2024-09-17 23:29 [PATCH 0/5] i2c: RTL9300 support Chris Packham
` (3 preceding siblings ...)
2024-09-17 23:29 ` [PATCH 4/5] dt-bindings: i2c: Add RTL9300 I2C multiplexer Chris Packham
@ 2024-09-17 23:29 ` Chris Packham
2024-09-18 20:36 ` Andi Shyti
4 siblings, 1 reply; 16+ messages in thread
From: Chris Packham @ 2024-09-17 23:29 UTC (permalink / raw)
To: andi.shyti, robh, krzk+dt, conor+dt, tsbogend
Cc: linux-i2c, devicetree, linux-kernel, linux-mips, Chris Packham
The RTL9300 I2C controller can support multiplexing by choosing the SDA
pin to be used dynamically. Add mutliplexing support to the rtl9300
driver.
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
drivers/i2c/busses/i2c-rtl9300.c | 168 ++++++++++++++++++++++++++++++-
1 file changed, 167 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-rtl9300.c b/drivers/i2c/busses/i2c-rtl9300.c
index f16e9b6343bf..a934a2526c92 100644
--- a/drivers/i2c/busses/i2c-rtl9300.c
+++ b/drivers/i2c/busses/i2c-rtl9300.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#include <linux/i2c.h>
+#include <linux/i2c-mux.h>
#include <linux/mod_devicetable.h>
#include <linux/mfd/syscon.h>
#include <linux/mutex.h>
@@ -44,6 +45,14 @@ struct rtl9300_i2c {
#define RTL9300_I2C_STD_FREQ 0
#define RTL9300_I2C_FAST_FREQ 1
+struct rtl9300_i2c_chan {
+ int parent;
+ const unsigned int *chans;
+ int n_chan;
+};
+
+#define RTL9300_I2C_MUX_NCHAN 8
+
DEFINE_MUTEX(i2c_lock);
static int rtl9300_i2c_reg_addr_set(struct rtl9300_i2c *i2c, u32 reg, u16 len)
@@ -370,7 +379,164 @@ static struct platform_driver rtl9300_i2c_driver = {
},
};
-module_platform_driver(rtl9300_i2c_driver);
+static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct i2c_adapter *adap = muxc->parent;
+ struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+ int ret;
+
+ ret = rtl9300_i2c_config_io(i2c, chan);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rtl9300_i2c_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
+{
+ struct i2c_adapter *adap = muxc->parent;
+ struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
+ int ret;
+
+ ret = rtl9300_i2c_config_io(i2c, i2c->sda_pin);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct fwnode_handle *fwnode = dev_fwnode(dev);
+ struct device_node *np = dev->of_node;
+ struct device_node *adap_np;
+ struct i2c_adapter *adap = NULL;
+ struct fwnode_handle *child;
+ unsigned int *chans;
+ int i = 0;
+
+ if (!is_of_node(fwnode))
+ return -EOPNOTSUPP;
+
+ if (!np)
+ return -ENODEV;
+
+ adap_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adap_np) {
+ dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adap = of_find_i2c_adapter_by_node(adap_np);
+ of_node_put(adap_np);
+
+ if (!adap)
+ return -EPROBE_DEFER;
+
+ mux->parent = i2c_adapter_id(adap);
+ put_device(&adap->dev);
+
+ mux->n_chan = device_get_child_node_count(dev);
+ if (mux->n_chan >= RTL9300_I2C_MUX_NCHAN)
+ return -EINVAL;
+
+ chans = devm_kcalloc(dev, mux->n_chan, sizeof(*mux->chans), GFP_KERNEL);
+ if (!chans)
+ return -ENOMEM;
+
+ device_for_each_child_node(dev, child) {
+ fwnode_property_read_u32(child, "reg", chans + i);
+ i++;
+ }
+ mux->chans = chans;
+
+ return 0;
+}
+
+static int rtl9300_i2c_mux_probe(struct platform_device *pdev)
+{
+ struct i2c_mux_core *muxc;
+ struct i2c_adapter *adap;
+ struct rtl9300_i2c_chan *mux;
+ int ret, i;
+
+ mux = devm_kzalloc(&pdev->dev, sizeof(*mux), GFP_KERNEL);
+ if (!mux)
+ return -ENOMEM;
+
+ ret = rtl9300_i2c_mux_probe_fw(mux, pdev);
+ if (ret)
+ return ret;
+
+ adap = i2c_get_adapter(mux->parent);
+ if (!adap)
+ return -EPROBE_DEFER;
+
+ muxc = i2c_mux_alloc(adap, &pdev->dev, mux->n_chan, 0, 0,
+ rtl9300_i2c_select_chan, rtl9300_i2c_deselect_mux);
+ if (!muxc) {
+ ret = -ENOMEM;
+ goto err_alloc;
+ }
+ muxc->priv = mux;
+
+ platform_set_drvdata(pdev, muxc);
+
+ for (i = 0; i < mux->n_chan; i++) {
+ ret = i2c_mux_add_adapter(muxc, 0, mux->chans[i]);
+ if (ret)
+ goto err_del_adapters;
+ }
+
+ return 0;
+
+err_del_adapters:
+ i2c_mux_del_adapters(muxc);
+err_alloc:
+ i2c_put_adapter(adap);
+
+ return ret;
+}
+
+static void rtl9300_i2c_mux_remove(struct platform_device *pdev)
+{
+ struct i2c_mux_core *muxc = platform_get_drvdata(pdev);
+
+ i2c_mux_del_adapters(muxc);
+ i2c_put_adapter(muxc->parent);
+}
+
+static const struct of_device_id i2c_rtl9300_mux_dt_ids[] = {
+ { .compatible = "realtek,rtl9300-i2c-mux" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, i2c_rtl9300_mux_dt_ids);
+
+static struct platform_driver rtl9300_i2c_mux_driver = {
+ .probe = rtl9300_i2c_mux_probe,
+ .remove = rtl9300_i2c_mux_remove,
+ .driver = {
+ .name = "i2c-mux-rtl9300",
+ .of_match_table = i2c_rtl9300_mux_dt_ids,
+ },
+};
+
+static struct platform_driver * const drivers[] = {
+ &rtl9300_i2c_driver,
+ &rtl9300_i2c_mux_driver,
+};
+
+static int __init rtl9300_i2c_init(void)
+{
+ return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_init(rtl9300_i2c_init);
+
+static void __exit rtl9300_i2c_exit(void)
+{
+ platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_exit(rtl9300_i2c_exit);
MODULE_DESCRIPTION("RTL9300 I2C controller driver");
MODULE_LICENSE("GPL");
--
2.46.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support
2024-09-17 23:29 ` [PATCH 5/5] i2c: rtl9300: Add multiplexing support Chris Packham
@ 2024-09-18 20:36 ` Andi Shyti
2024-09-18 21:44 ` Chris Packham
0 siblings, 1 reply; 16+ messages in thread
From: Andi Shyti @ 2024-09-18 20:36 UTC (permalink / raw)
To: Chris Packham
Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
Hi Chris,
...
> -module_platform_driver(rtl9300_i2c_driver);
> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> + struct i2c_adapter *adap = muxc->parent;
> + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
> + int ret;
> +
> + ret = rtl9300_i2c_config_io(i2c, chan);
> + if (ret)
> + return ret;
> +
> + return 0;
return "rtl9300_i2c_config_io()"?
> +}
...
> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct fwnode_handle *fwnode = dev_fwnode(dev);
> + struct device_node *np = dev->of_node;
> + struct device_node *adap_np;
> + struct i2c_adapter *adap = NULL;
> + struct fwnode_handle *child;
> + unsigned int *chans;
> + int i = 0;
> +
> + if (!is_of_node(fwnode))
> + return -EOPNOTSUPP;
> +
> + if (!np)
> + return -ENODEV;
> +
> + adap_np = of_parse_phandle(np, "i2c-parent", 0);
> + if (!adap_np) {
> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
> + return -ENODEV;
return dev_err_probe(...)?
> + }
> + adap = of_find_i2c_adapter_by_node(adap_np);
> + of_node_put(adap_np);
...
> +static int __init rtl9300_i2c_init(void)
> +{
> + return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
> +}
> +module_init(rtl9300_i2c_init);
> +
> +static void __exit rtl9300_i2c_exit(void)
> +{
> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
> +}
> +module_exit(rtl9300_i2c_exit);
You could use module_platform_driver()
Thanks,
Andi
>
> MODULE_DESCRIPTION("RTL9300 I2C controller driver");
> MODULE_LICENSE("GPL");
> --
> 2.46.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support
2024-09-18 20:36 ` Andi Shyti
@ 2024-09-18 21:44 ` Chris Packham
2024-09-19 5:06 ` Chris Packham
0 siblings, 1 reply; 16+ messages in thread
From: Chris Packham @ 2024-09-18 21:44 UTC (permalink / raw)
To: Andi Shyti
Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
Hi Andi, Rob,
On 19/09/24 08:36, Andi Shyti wrote:
> Hi Chris,
>
> ...
>
>> -module_platform_driver(rtl9300_i2c_driver);
>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32 chan)
>> +{
>> + struct i2c_adapter *adap = muxc->parent;
>> + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
>> + int ret;
>> +
>> + ret = rtl9300_i2c_config_io(i2c, chan);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
> return "rtl9300_i2c_config_io()"?
Ack.
>> +}
> ...
>
>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux, struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct fwnode_handle *fwnode = dev_fwnode(dev);
>> + struct device_node *np = dev->of_node;
>> + struct device_node *adap_np;
>> + struct i2c_adapter *adap = NULL;
>> + struct fwnode_handle *child;
>> + unsigned int *chans;
>> + int i = 0;
>> +
>> + if (!is_of_node(fwnode))
>> + return -EOPNOTSUPP;
>> +
>> + if (!np)
>> + return -ENODEV;
>> +
>> + adap_np = of_parse_phandle(np, "i2c-parent", 0);
>> + if (!adap_np) {
>> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>> + return -ENODEV;
> return dev_err_probe(...)?
Ack.
>> + }
>> + adap = of_find_i2c_adapter_by_node(adap_np);
>> + of_node_put(adap_np);
> ...
>
>> +static int __init rtl9300_i2c_init(void)
>> +{
>> + return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
>> +}
>> +module_init(rtl9300_i2c_init);
>> +
>> +static void __exit rtl9300_i2c_exit(void)
>> +{
>> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
>> +}
>> +module_exit(rtl9300_i2c_exit);
> You could use module_platform_driver()
Can I though? I want to support both the simple I2C controller and the
MUX mode with the same driver. Which is why I've ended up with two
drivers to register.
On the binding patch, Rob made the suggestion that I just make the
i2c-mux part of the parent. I did consider that but quickly got tied in
knots because I couldn't figure out how to have a device that is both an
adapter and a mux. The main problem was that any child nodes of an i2c
adapter in the device tree are presumed to be I2C devices and get probed
automatically by of_i2c_register_devices(). Equally I can't register a
mux without having an adapter that the mux operates over.
>
> Thanks,
> Andi
>
>>
>> MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>> MODULE_LICENSE("GPL");
>> --
>> 2.46.1
>>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 5/5] i2c: rtl9300: Add multiplexing support
2024-09-18 21:44 ` Chris Packham
@ 2024-09-19 5:06 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-09-19 5:06 UTC (permalink / raw)
To: Andi Shyti
Cc: robh, krzk+dt, conor+dt, tsbogend, linux-i2c, devicetree,
linux-kernel, linux-mips
On 19/09/24 09:44, Chris Packham wrote:
> Hi Andi, Rob,
>
> On 19/09/24 08:36, Andi Shyti wrote:
>> Hi Chris,
>>
>> ...
>>
>>> -module_platform_driver(rtl9300_i2c_driver);
>>> +static int rtl9300_i2c_select_chan(struct i2c_mux_core *muxc, u32
>>> chan)
>>> +{
>>> + struct i2c_adapter *adap = muxc->parent;
>>> + struct rtl9300_i2c *i2c = i2c_get_adapdata(adap);
>>> + int ret;
>>> +
>>> + ret = rtl9300_i2c_config_io(i2c, chan);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>> return "rtl9300_i2c_config_io()"?
>
> Ack.
>
>>> +}
>> ...
>>
>>> +static int rtl9300_i2c_mux_probe_fw(struct rtl9300_i2c_chan *mux,
>>> struct platform_device *pdev)
>>> +{
>>> + struct device *dev = &pdev->dev;
>>> + struct fwnode_handle *fwnode = dev_fwnode(dev);
>>> + struct device_node *np = dev->of_node;
>>> + struct device_node *adap_np;
>>> + struct i2c_adapter *adap = NULL;
>>> + struct fwnode_handle *child;
>>> + unsigned int *chans;
>>> + int i = 0;
>>> +
>>> + if (!is_of_node(fwnode))
>>> + return -EOPNOTSUPP;
>>> +
>>> + if (!np)
>>> + return -ENODEV;
>>> +
>>> + adap_np = of_parse_phandle(np, "i2c-parent", 0);
>>> + if (!adap_np) {
>>> + dev_err(&pdev->dev, "Cannot parse i2c-parent\n");
>>> + return -ENODEV;
>> return dev_err_probe(...)?
>
> Ack.
>
>>> + }
>>> + adap = of_find_i2c_adapter_by_node(adap_np);
>>> + of_node_put(adap_np);
>> ...
>>
>>> +static int __init rtl9300_i2c_init(void)
>>> +{
>>> + return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_init(rtl9300_i2c_init);
>>> +
>>> +static void __exit rtl9300_i2c_exit(void)
>>> +{
>>> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
>>> +}
>>> +module_exit(rtl9300_i2c_exit);
>> You could use module_platform_driver()
>
> Can I though? I want to support both the simple I2C controller and the
> MUX mode with the same driver. Which is why I've ended up with two
> drivers to register.
>
> On the binding patch, Rob made the suggestion that I just make the
> i2c-mux part of the parent. I did consider that but quickly got tied
> in knots because I couldn't figure out how to have a device that is
> both an adapter and a mux. The main problem was that any child nodes
> of an i2c adapter in the device tree are presumed to be I2C devices
> and get probed automatically by of_i2c_register_devices(). Equally I
> can't register a mux without having an adapter that the mux operates
> over.
OK I think I've got something working that has a dt binding like
i2c@36c {
compatible = "realtek,rtl9300-i2c";
reg = <0x36c 0x14>;
status = "okay";
#address-cells = <0x01>;
#size-cells = <0x00>;
i2c@0 {
reg = <0x00>;
#address-cells = <1>;
#size-cells = <1>;
gpio@20 {
...
};
};
i2c@2 {
reg = <0x02>;
};
};
In the probe() I can iterate over the child nodes and create an adapter
for each. The code is a bit fiddly but I think it's a net win if I can
do away with the rtl9300-i2c-mux part. It also happily means that I
don't have an extra I2C bus that is the same as the first mux channel.
I'll try an tidy things up and get another iteration out before my weekend.
>
>>
>> Thanks,
>> Andi
>>
>>> MODULE_DESCRIPTION("RTL9300 I2C controller driver");
>>> MODULE_LICENSE("GPL");
>>> --
>>> 2.46.1
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread