Re: [PATCH v10 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] i2c: ma35d1: missing register restore after system resume
- [High] i2c: ma35d1: system suspend aborted by enable_irq_wake failure
--

commit b259b8f6191b7d3a9bf1070672a2bb0522d35137
Author: Zi-Yu Chen <zychennvt@gmail.com>

i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

This commit introduces the Nuvoton MA35D1 I2C driver, which supports standard,
fast, and fast-plus modes. The driver provides both controller and target
functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..9b1872b8033f1
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
> @@ -0,0 +1,781 @@
[ ... ]
> +static void ma35d1_i2c_reset(struct ma35d1_i2c *i2c)
> +{
> +	unsigned int clkdiv, slvaddr;
> +
> +	clkdiv = readl(i2c->regs + MA35_CLKDIV);
> +	slvaddr = readl(i2c->regs + MA35_ADDR0);

[Severity: High]
Does this code read a potentially corrupted CLKDIV value from hardware
instead of using a saved configuration?

If the system was suspended and power was dropped, this register might be
lost. The recovery function would then read a default CLKDIV of 0, which is
later written back, permanently corrupting the bus frequency.

[ ... ]
> +static int ma35d1_i2c_suspend(struct device *dev)
> +{
> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	/* Prepare for wake-up from I2C events if target mode is active */
> +	if (i2c->target) {
> +		ma35d1_i2c_write_ctl(i2c, MA35_CTL_AA);
> +
> +		/* Setup wake-up control */
> +		writel(0x1, i2c->regs + MA35_WKCTL);
> +
> +		/* Clear pending wake-up flags */
> +		val = readl(i2c->regs + MA35_WKSTS);
> +		writel(val, i2c->regs + MA35_WKSTS);
> +
> +		ret = enable_irq_wake(i2c->irq);
> +		if (ret)
> +			return ret;

[Severity: High]
Could this fail the entire system suspend sequence if the I2C interrupt
cannot be configured as a wakeup source?

If the underlying IRQ controller does not support irq_set_wake() for this
interrupt line (which is common on many ARM platforms without dedicated
wake routing), enable_irq_wake() will return -ENXIO or -EINVAL.

Propagating this error to the PM core violently aborts the sleep transition
whenever target mode is enabled.

[ ... ]
> +static int ma35d1_i2c_runtime_resume(struct device *dev)
> +{
> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_enable(i2c->clk);

[Severity: High]
Does this function need to restore the CLKDIV and CTL0 registers?

These registers are lost during a system suspend (S3) if the power domain
drops power.

If the registers aren't restored, the first transfer attempted after system
resume writes STA to an otherwise zeroed CTL0, leaving the controller and
interrupts disabled. This results in a 5-second timeout, after which the
error path resets the controller but writes back a default CLKDIV.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921083753.1109826-1-zychennvt@gmail.com?part=2