[PATCH v2 0/3] i2c: spacemit: add reset support

[PATCH v2 0/3] i2c: spacemit: add reset support

[Encrow Thorne]

Add reset support for the K1 I2C driver. A reset ensures that the
controller starts in a clean and known state.

Reset ensures that the I2C hardware is in a clean state. We cannot assume
that no program used I2C before the kernel booted.

Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
---
Changes in v2:
- Replace reset property in dt-bindings.
- Use devm_reset_control_get_optional_exclusive_deasserted() instead.
- Rebase to v6.19-rc1.
- Link to v1: https://lore.kernel.org/r/20251119-i2c-k1_reset-support-v1-0-0e9e82bf9b65@gmail.com

---
Encrow Thorne (3):
      dt-bindings: i2c: spacemit: add optional resets
      i2c: k1: add reset support
      riscv: dts: spacemit: add reset property

 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 3 +++
 arch/riscv/boot/dts/spacemit/k1.dtsi                       | 8 ++++++++
 drivers/i2c/busses/i2c-k1.c                                | 7 +++++++
 3 files changed, 18 insertions(+)
---
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
change-id: 20251218-i2c-reset-a7be139213de

Best regards,
-- 
Encrow Thorne <jyc0019@gmail.com>

[PATCH v2 1/3] dt-bindings: i2c: spacemit: add optional resets

[Encrow Thorne]

The I2C controller requires a reset to ensure it starts from a clean state.

Add the 'resets' property to support this hardware requirement.

Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
---
 Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
index b7220fff2235..1290106e28e6 100644
--- a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
@@ -32,6 +32,9 @@ properties:
       - const: func
       - const: bus
 
+  resets:
+    maxItems: 1
+
   clock-frequency:
     description: |
       K1 support three different modes which running different frequencies

-- 
2.25.1

[PATCH v2 2/3] i2c: k1: add reset support

[Encrow Thorne]

The K1 I2C controller provides a reset line that needs to be deasserted
before the controller can be accessed.

Add reset support to the driver to ensure the controller starts in the
required state.

Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
---
 drivers/i2c/busses/i2c-k1.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/busses/i2c-k1.c b/drivers/i2c/busses/i2c-k1.c
index d42c03ef5db5..23661c7ddb67 100644
--- a/drivers/i2c/busses/i2c-k1.c
+++ b/drivers/i2c/busses/i2c-k1.c
@@ -10,6 +10,7 @@
  #include <linux/module.h>
  #include <linux/of_address.h>
  #include <linux/platform_device.h>
+ #include <linux/reset.h>
 
 /* spacemit i2c registers */
 #define SPACEMIT_ICR		 0x0		/* Control register */
@@ -534,6 +535,7 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *of_node = pdev->dev.of_node;
 	struct spacemit_i2c_dev *i2c;
+	struct reset_control *rst;
 	int ret;
 
 	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
@@ -578,6 +580,11 @@ static int spacemit_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(clk))
 		return dev_err_probe(dev, PTR_ERR(clk), "failed to enable bus clock");
 
+	rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst),
+				     "failed to acquire deasserted reset\n");
+
 	spacemit_i2c_reset(i2c);
 
 	i2c_set_adapdata(&i2c->adapt, i2c);

-- 
2.25.1

[PATCH v2 3/3] riscv: dts: spacemit: add reset property

[Encrow Thorne]

Add resets property to K1 I2C node.
---
 arch/riscv/boot/dts/spacemit/k1.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 7818ca4979b6..085987643638 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -367,6 +367,7 @@ i2c0: i2c@d4010800 {
 				 <&syscon_apbc CLK_TWSI0_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI0>;
 			interrupts = <36>;
 			status = "disabled";
 		};
@@ -380,6 +381,7 @@ i2c1: i2c@d4011000 {
 				 <&syscon_apbc CLK_TWSI1_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI1>;
 			interrupts = <37>;
 			status = "disabled";
 		};
@@ -393,6 +395,7 @@ i2c2: i2c@d4012000 {
 				 <&syscon_apbc CLK_TWSI2_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI2>;
 			interrupts = <38>;
 			status = "disabled";
 		};
@@ -406,6 +409,7 @@ i2c4: i2c@d4012800 {
 				 <&syscon_apbc CLK_TWSI4_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI4>;
 			interrupts = <40>;
 			status = "disabled";
 		};
@@ -419,6 +423,7 @@ i2c5: i2c@d4013800 {
 				 <&syscon_apbc CLK_TWSI5_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI5>;
 			interrupts = <41>;
 			status = "disabled";
 		};
@@ -443,6 +448,7 @@ i2c6: i2c@d4018800 {
 				 <&syscon_apbc CLK_TWSI6_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI6>;
 			interrupts = <70>;
 			status = "disabled";
 		};
@@ -546,6 +552,7 @@ i2c7: i2c@d401d000 {
 				 <&syscon_apbc CLK_TWSI7_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI7>;
 			interrupts = <18>;
 			status = "disabled";
 		};
@@ -559,6 +566,7 @@ i2c8: i2c@d401d800 {
 				 <&syscon_apbc CLK_TWSI8_BUS>;
 			clock-names = "func", "bus";
 			clock-frequency = <400000>;
+			resets = <&syscon_apbc RESET_TWSI8>;
 			interrupts = <19>;
 			status = "disabled";
 		};

-- 
2.25.1

Re: [PATCH v2 1/3] dt-bindings: i2c: spacemit: add optional resets

[Krzysztof Kozlowski]

On 19/12/2025 08:42, Encrow Thorne wrote:
> The I2C controller requires a reset to ensure it starts from a clean state.
> 
> Add the 'resets' property to support this hardware requirement.
> 
> Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
> ---
>  Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> index b7220fff2235..1290106e28e6 100644
> --- a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> @@ -32,6 +32,9 @@ properties:
>        - const: func
>        - const: bus
>  
> +  resets:
> +    maxItems: 1
> +

Still between other clock-related properties. You already got such
comment...

>    clock-frequency:
>      description: |
>        K1 support three different modes which running different frequencies
> 


Best regards,
Krzysztof

Re: [PATCH v2 0/3] i2c: spacemit: add reset support

[Krzysztof Kozlowski]

On 19/12/2025 08:42, Encrow Thorne wrote:
> Add reset support for the K1 I2C driver. A reset ensures that the
> controller starts in a clean and known state.
> 
> Reset ensures that the I2C hardware is in a clean state. We cannot assume
> that no program used I2C before the kernel booted.
> 
> Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
> ---
> Changes in v2:
> - Replace reset property in dt-bindings.

Replace with what? I don't see anything else there - you still have
reset property.


Best regards,
Krzysztof

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Andi Shyti]

Hi Encrow,

...

> +	rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst),
> +				     "failed to acquire deasserted reset\n");

If this is optional, why are we returning with error?

Thanks,
Andi

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Guodong Xu]

Hi, Andi

On Fri, Dec 26, 2025 at 5:01 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Encrow,
>
> ...
>
> > +     rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> > +     if (IS_ERR(rst))
> > +             return dev_err_probe(dev, PTR_ERR(rst),
> > +                                  "failed to acquire deasserted reset\n");
>
> If this is optional, why are we returning with error?
>

According to include/linux/reset.h, if the requested reset is not
specified in the device tree, this function returns NULL instead of
an error. Therefore, IS_ERR(rst) will only be true for actual
errors (e.g probe deferral).

BR,
Guodong

> Thanks,
> Andi

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Troy Mitchell]

On Fri, Dec 19, 2025 at 03:42:21PM +0800, Encrow Thorne wrote:
> The K1 I2C controller provides a reset line that needs to be deasserted
> before the controller can be accessed.
> 
> Add reset support to the driver to ensure the controller starts in the
> required state.
> 
> Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
Reviewed-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>

Re: [PATCH v2 1/3] dt-bindings: i2c: spacemit: add optional resets

[Troy Mitchell]

On Fri, Dec 19, 2025 at 09:03:14AM +0100, Krzysztof Kozlowski wrote:
> On 19/12/2025 08:42, Encrow Thorne wrote:
> > The I2C controller requires a reset to ensure it starts from a clean state.
> > 
> > Add the 'resets' property to support this hardware requirement.
> > 
> > Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > index b7220fff2235..1290106e28e6 100644
> > --- a/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/spacemit,k1-i2c.yaml
> > @@ -32,6 +32,9 @@ properties:
> >        - const: func
> >        - const: bus
> >  
> > +  resets:
> > +    maxItems: 1
> > +
> 
> Still between other clock-related properties. You already got such
> comment...
Please replace it below clock-frequency.
With this fix, you can add my tag:

Reviewed-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>
> 
> >    clock-frequency:
> >      description: |
> >        K1 support three different modes which running different frequencies
> > 
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Andi Shyti]

Hi Guodong,

On Fri, Dec 26, 2025 at 07:38:22AM +0800, Guodong Xu wrote:
> On Fri, Dec 26, 2025 at 5:01 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > +     rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> > > +     if (IS_ERR(rst))
> > > +             return dev_err_probe(dev, PTR_ERR(rst),
> > > +                                  "failed to acquire deasserted reset\n");
> >
> > If this is optional, why are we returning with error?
> >
> 
> According to include/linux/reset.h, if the requested reset is not
> specified in the device tree, this function returns NULL instead of
> an error. Therefore, IS_ERR(rst) will only be true for actual
> errors (e.g probe deferral).

And this is quite obvious, but you haven't answered my qestion.

Why do we care of internal failures in reset? If reset fails on
an optional reset control function why should we kill our driver?
Just ignore the error and move forward as we have done until now.

If the kernel is suffering from internal failures (say ENOMEM),
it will fail anyway or, with some luck, recover.

Andi

Re: [PATCH v2 0/3] i2c: spacemit: add reset support

[Guodong Xu]

On Fri, Dec 19, 2025 at 4:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/12/2025 08:42, Encrow Thorne wrote:
> > Add reset support for the K1 I2C driver. A reset ensures that the
> > controller starts in a clean and known state.
> >
> > Reset ensures that the I2C hardware is in a clean state. We cannot assume
> > that no program used I2C before the kernel booted.
> >
> > Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
> > ---
> > Changes in v2:
> > - Replace reset property in dt-bindings.
>
> Replace with what? I don't see anything else there - you still have
> reset property.

It looks like a phrasing issue. By 'replace,' I guess, Encrow meant that
the resets property was moved (reordered, put into a different 'place')
within the file, not that it was swapped for a different property.

Encow,

Would you please correct your changelog description
in the next version with something like
'Reorder the placement of the resets property in the dt-binding file.'

BR,
Guodong

>
>
> Best regards,
> Krzysztof

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Guodong Xu]

Hi, Andi

On Sun, Dec 28, 2025 at 3:26 AM Andi Shyti <andi.shyti@kernel.org> wrote:
>
> Hi Guodong,
>
> On Fri, Dec 26, 2025 at 07:38:22AM +0800, Guodong Xu wrote:
> > On Fri, Dec 26, 2025 at 5:01 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > > +     rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> > > > +     if (IS_ERR(rst))
> > > > +             return dev_err_probe(dev, PTR_ERR(rst),
> > > > +                                  "failed to acquire deasserted reset\n");
> > >
> > > If this is optional, why are we returning with error?
> > >
> >
> > According to include/linux/reset.h, if the requested reset is not
> > specified in the device tree, this function returns NULL instead of
> > an error. Therefore, IS_ERR(rst) will only be true for actual
> > errors (e.g probe deferral).
>
> And this is quite obvious, but you haven't answered my qestion.
>
> Why do we care of internal failures in reset? If reset fails on
> an optional reset control function why should we kill our driver?

Thanks for the clarification. I see your point now.

My reasoning is that if the resets property is explicitly listed in the
Device Tree, the driver must respect it. If the property is present but
we encounter an error (like -EPROBE_DEFER), ignoring that failure could
put the hardware in an undefined or dirty state.

> Just ignore the error and move forward as we have done until now.

I want to double-check what you mean.

I checked other drivers in drivers/i2c/busses (e.g. i2c-riic.c, i2c-mv64xxx.c,
i2c-designware-platdrv.c), and they seem to follow this pattern of returning
errors even for optional resets.

BR,
Guodong

>
> If the kernel is suffering from internal failures (say ENOMEM),
> it will fail anyway or, with some luck, recover.
>
> Andi

Re: [PATCH v2 0/3] i2c: spacemit: add reset support

[Encrow Thorne]

On Sun, Dec 28, 2025 at 07:53:52AM +0800, Guodong Xu wrote:
> On Fri, Dec 19, 2025 at 4:03 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 19/12/2025 08:42, Encrow Thorne wrote:
> > > Add reset support for the K1 I2C driver. A reset ensures that the
> > > controller starts in a clean and known state.
> > >
> > > Reset ensures that the I2C hardware is in a clean state. We cannot assume
> > > that no program used I2C before the kernel booted.
> > >
> > > Signed-off-by: Encrow Thorne <jyc0019@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Replace reset property in dt-bindings.
> >
> > Replace with what? I don't see anything else there - you still have
> > reset property.
> 
> It looks like a phrasing issue. By 'replace,' I guess, Encrow meant that
> the resets property was moved (reordered, put into a different 'place')
> within the file, not that it was swapped for a different property.
> 
> Encow,
> 
> Would you please correct your changelog description
> in the next version with something like
> 'Reorder the placement of the resets property in the dt-binding file.'
> 
> BR,
> Guodong
> 
 Thank you for your suggestion, Guodong. I will modify it in the next version.

 			- Encrow
> >
> >
> > Best regards,
> > Krzysztof

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Troy Mitchell]

On Sun, Dec 28, 2025 at 08:42:47AM +0800, Guodong Xu wrote:
> Hi, Andi
> 
> On Sun, Dec 28, 2025 at 3:26 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> >
> > Hi Guodong,
> >
> > On Fri, Dec 26, 2025 at 07:38:22AM +0800, Guodong Xu wrote:
> > > On Fri, Dec 26, 2025 at 5:01 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > > > +     rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> > > > > +     if (IS_ERR(rst))
> > > > > +             return dev_err_probe(dev, PTR_ERR(rst),
> > > > > +                                  "failed to acquire deasserted reset\n");
> > > >
> > > > If this is optional, why are we returning with error?
> > > >
> > >
> > > According to include/linux/reset.h, if the requested reset is not
> > > specified in the device tree, this function returns NULL instead of
> > > an error. Therefore, IS_ERR(rst) will only be true for actual
> > > errors (e.g probe deferral).
> >
> > And this is quite obvious, but you haven't answered my qestion.
> >
> > Why do we care of internal failures in reset? If reset fails on
> > an optional reset control function why should we kill our driver?
> 
> Thanks for the clarification. I see your point now.
> 
> My reasoning is that if the resets property is explicitly listed in the
> Device Tree, the driver must respect it.
It's not required.

>
> If the property is present but
> we encounter an error (like -EPROBE_DEFER), ignoring that failure could
> put the hardware in an undefined or dirty state.
Then why it's optional?

The real reason:
"Optional" here means the reset line is allowed to be absent from the
Device Tree. It does not mean we can ignore the failure when it is
defined in the DT but fails to be acquired.

If devm_reset_control_get_optional_* returns an error (e.g.,
-EPROBE_DEFER), it indicates the hardware description expects a reset
control, but the system is not yet ready to provide it. Ignoring this
error would break the probe deferral mechanism and potentially cause the
driver to access hardware in an invalid state.

                          - Troy

Re: [PATCH v2 2/3] i2c: k1: add reset support

[Troy Mitchell]

On Tue, Dec 30, 2025 at 10:11:16PM +0800, Troy Mitchell wrote:
> On Sun, Dec 28, 2025 at 08:42:47AM +0800, Guodong Xu wrote:
> > Hi, Andi
> > 
> > On Sun, Dec 28, 2025 at 3:26 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > >
> > > Hi Guodong,
> > >
> > > On Fri, Dec 26, 2025 at 07:38:22AM +0800, Guodong Xu wrote:
> > > > On Fri, Dec 26, 2025 at 5:01 AM Andi Shyti <andi.shyti@kernel.org> wrote:
> > > > > > +     rst = devm_reset_control_get_optional_exclusive_deasserted(dev, NULL);
> > > > > > +     if (IS_ERR(rst))
> > > > > > +             return dev_err_probe(dev, PTR_ERR(rst),
> > > > > > +                                  "failed to acquire deasserted reset\n");
> > > > >
> > > > > If this is optional, why are we returning with error?
> > > > >
> > > >
> > > > According to include/linux/reset.h, if the requested reset is not
> > > > specified in the device tree, this function returns NULL instead of
> > > > an error. Therefore, IS_ERR(rst) will only be true for actual
> > > > errors (e.g probe deferral).
> > >
> > > And this is quite obvious, but you haven't answered my qestion.
> > >
> > > Why do we care of internal failures in reset? If reset fails on
> > > an optional reset control function why should we kill our driver?
> > 
> > Thanks for the clarification. I see your point now.
> > 
> > My reasoning is that if the resets property is explicitly listed in the
> > Device Tree, the driver must respect it.
Sorry, I misread your comment. I thought you were referring to the
bindings. In that case, we are on the same page.

                                    - Troy
> It's not required.
> 
> >
> > If the property is present but
> > we encounter an error (like -EPROBE_DEFER), ignoring that failure could
> > put the hardware in an undefined or dirty state.
> Then why it's optional?
> 
> The real reason:
> "Optional" here means the reset line is allowed to be absent from the
> Device Tree. It does not mean we can ignore the failure when it is
> defined in the DT but fails to be acquired.
> 
> If devm_reset_control_get_optional_* returns an error (e.g.,
> -EPROBE_DEFER), it indicates the hardware description expects a reset
> control, but the system is not yet ready to provide it. Ignoring this
> error would break the probe deferral mechanism and potentially cause the
> driver to access hardware in an invalid state.
> 
>                           - Troy