The driver currently acquires reset control during probe but does not
manage it during system suspend/resume. This can leave the I2C controller
in an undefined state after resume.

Add reset control assertion during system suspend and deassertion during
resume to ensure proper hardware initialization across power management
transitions. Skip runtime suspend if the device is already suspended to
avoid redundant operations.

Signed-off-by: Artem Shimko <a.shimko.dev@gmail.com>
---
Hello maintainers and reviewers,

This patch adds proper reset control handling to the suspend/resume
callbacks. The reset line is asserted during system suspend to ensure
the controller is properly quiesced, and deasserted during resume to
reinitialize the hardware. Additionally, the suspend path now checks
the runtime PM status to avoid redundant operations when the device is
already suspended.

Thank you!
--
Regards,
Artem

 drivers/i2c/busses/i2c-designware-common.c | 40 ++++++++++++++++++----
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 4dc57fd56170..fbe817df9b2e 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -29,6 +29,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 #include <linux/swab.h>
 #include <linux/types.h>
 #include <linux/units.h>
@@ -967,6 +968,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_probe);
 
+#ifdef CONFIG_ACPI
 static int i2c_dw_prepare(struct device *device)
 {
 	/*
@@ -977,18 +979,23 @@ static int i2c_dw_prepare(struct device *device)
 	 */
 	return !has_acpi_companion(device);
 }
+#endif
 
 static int i2c_dw_runtime_suspend(struct device *device)
 {
 	struct dw_i2c_dev *dev = dev_get_drvdata(device);
+	int ret;
 
 	if (dev->shared_with_punit)
 		return 0;
 
 	i2c_dw_disable(dev);
-	i2c_dw_prepare_clk(dev, false);
 
-	return 0;
+	ret = reset_control_assert(dev->rst);
+	if (ret)
+		return ret;
+
+	return i2c_dw_prepare_clk(dev, false);
 }
 
 static int i2c_dw_suspend(struct device *device)
@@ -997,15 +1004,28 @@ static int i2c_dw_suspend(struct device *device)
 
 	i2c_mark_adapter_suspended(&dev->adapter);
 
-	return i2c_dw_runtime_suspend(device);
+	if (!pm_runtime_status_suspended(device))
+		return i2c_dw_runtime_suspend(device);
+
+	return 0;
 }
 
 static int i2c_dw_runtime_resume(struct device *device)
 {
 	struct dw_i2c_dev *dev = dev_get_drvdata(device);
+	int ret;
+
+	if (!dev->shared_with_punit) {
+		ret = i2c_dw_prepare_clk(dev, true);
+		if (ret)
+			return ret;
 
-	if (!dev->shared_with_punit)
-		i2c_dw_prepare_clk(dev, true);
+		ret = reset_control_deassert(dev->rst);
+		if (ret) {
+			i2c_dw_prepare_clk(dev, false);
+			return ret;
+		}
+	}
 
 	i2c_dw_init(dev);
 
@@ -1015,16 +1035,24 @@ static int i2c_dw_runtime_resume(struct device *device)
 static int i2c_dw_resume(struct device *device)
 {
 	struct dw_i2c_dev *dev = dev_get_drvdata(device);
+	int ret;
+
+	ret = i2c_dw_runtime_resume(device);
+	if (ret)
+		return ret;
 
-	i2c_dw_runtime_resume(device);
 	i2c_mark_adapter_resumed(&dev->adapter);
 
 	return 0;
 }
 
 EXPORT_GPL_DEV_PM_OPS(i2c_dw_dev_pm_ops) = {
+#ifdef CONFIG_ACPI
 	.prepare = pm_sleep_ptr(i2c_dw_prepare),
 	LATE_SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
+#else
+	SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
+#endif
 	RUNTIME_PM_OPS(i2c_dw_runtime_suspend, i2c_dw_runtime_resume, NULL)
 };
 
-- 
2.43.0

On Tue, Mar 10, 2026 at 09:40:45PM +0300, Artem Shimko wrote:
> The driver currently acquires reset control during probe but does not
> manage it during system suspend/resume. This can leave the I2C controller
> in an undefined state after resume.
> 
> Add reset control assertion during system suspend and deassertion during
> resume to ensure proper hardware initialization across power management
> transitions. Skip runtime suspend if the device is already suspended to
> avoid redundant operations.

...

> +#ifdef CONFIG_ACPI

Why?

>  static int i2c_dw_prepare(struct device *device)
>  {
>  	/*

>  	 */
>  	return !has_acpi_companion(device);
>  }
> +#endif

...

>  EXPORT_GPL_DEV_PM_OPS(i2c_dw_dev_pm_ops) = {
> +#ifdef CONFIG_ACPI
>  	.prepare = pm_sleep_ptr(i2c_dw_prepare),
>  	LATE_SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
> +#else
> +	SYSTEM_SLEEP_PM_OPS(i2c_dw_suspend, i2c_dw_resume)
> +#endif

Why?

Usually we try to avoid _adding_ ugly ifdeffery. Can you elaborate why the
change is needed in the first place and why there is no better way to handle
it?

>  	RUNTIME_PM_OPS(i2c_dw_runtime_suspend, i2c_dw_runtime_resume, NULL)
>  };

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

Thank you for your review!

On Tue, Mar 10, 2026 at 10:22 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Usually we try to avoid _adding_ ugly ifdeffery. Can you elaborate why the
> change is needed in the first place and why there is no better way to handle
> it?

The conditional compilation is needed because ACPI and DT platforms have
different requirements for system suspend/resume ordering: ACPI
platforms require LATE_SUSPEND/EARLY_RESUME
to support I2C operation regions, which may be accessed during
suspend/resume of other devices. DT platforms don't have
this requirement, but using LATE/EARLY callbacks causes problems
because the I2C controller's dependencies (reset controller,
clock controller) are not yet available at that stage, leading to hangs.

However, I agree that adding ifdefs is not ideal.

I have a couple ideas for v2, could you please advise which one is
better from your point of view?
1. Splitting PM ops into separate structures for ACPI and DT
2. Using has_acpi_companion() checks in the resume callbacks

--
Regards,
Artem

On Wed, Mar 11, 2026 at 09:20:25AM +0300, Artem Shimko wrote:
> Hi Andy,
> On Tue, Mar 10, 2026 at 10:22 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Usually we try to avoid _adding_ ugly ifdeffery. Can you elaborate why the
> > change is needed in the first place and why there is no better way to handle
> > it?
> 
> The conditional compilation is needed because ACPI and DT platforms have
> different requirements for system suspend/resume ordering: ACPI
> platforms require LATE_SUSPEND/EARLY_RESUME
> to support I2C operation regions, which may be accessed during
> suspend/resume of other devices. DT platforms don't have
> this requirement, but using LATE/EARLY callbacks causes problems
> because the I2C controller's dependencies (reset controller,
> clock controller) are not yet available at that stage, leading to hangs.
> 
> However, I agree that adding ifdefs is not ideal.
> 
> I have a couple ideas for v2, could you please advise which one is
> better from your point of view?
> 1. Splitting PM ops into separate structures for ACPI and DT
> 2. Using has_acpi_companion() checks in the resume callbacks

The second option is inline with the current approach in .prepare(),
so at least it would be easier to sell to the maintainers.

The first one will still require some ifdeffery if I am not mistaken.
(Or at least the similar has_acpi_companion() check.)

-- 
With Best Regards,
Andy Shevchenko

Hi Andy,

On Wed, Mar 11, 2026 at 3:11 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> The second option is inline with the current approach in .prepare(),
> so at least it would be easier to sell to the maintainers.
>
> The first one will still require some ifdeffery if I am not mistaken.
> (Or at least the similar has_acpi_companion() check.)

Thank you very much for the review and for the suggestion.

I spent some time trying to implement the has_acpi_companion() approach.
has_acpi_companion() can be used inside the callbacks, but it cannot
change which callbacks (suspend vs. late_suspend)
are registered in the PM ops structure. For ACPI platforms, we must
keep LATE_SYSTEM_SLEEP_PM_OPS to support I2C operation
regions that may be accessed during suspend/resume of other devices
(as established by commit [1], found by my colleague). For DT
platforms,
LATE_SYSTEM_SLEEP_PM_OPS causes hangs because the reset controller is
not yet available at the late/early stage.

I agree that adding ifdefs is not ideal. I will prepare v2 that:
1. Moves reset control handling into runtime suspend/resume callbacks
(this makes sense and aligns better with the existing PM structure).
2. Keeps the conditional PM ops selection.

Let's look at the maintainers' opinion, maybe after that I will be
able to send v2.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20260317&id=541527728341b
--
Regards,
Artem