Re: [PATCH v33 4/5] i2c: ast2600: Add controller driver for AST2600 new register set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The AST2600_I2CM_BUS_RECOVER_FAIL interrupt is handled in the ISR but never enabled in the Interrupt Enable Register (IER), causing bus recovery failures to time out.
--

commit d7e183b301ba820c2bd844ca60f5af7e37834e10
Author: Ryan Chen <ryan_chen@aspeedtech.com>

i2c: ast2600: Add controller driver for AST2600 new register set

This commit adds a new I2C controller driver for the ASPEED AST2600,
supporting its new register layout. The new capabilities include packet-based
transfers, dual pool buffer mode, and hardware-assisted bus recovery.

> diff --git a/drivers/i2c/busses/i2c-ast2600.c b/drivers/i2c/busses/i2c-ast2600.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ast2600.c

[ ... ]

> +static int ast2600_i2c_controller_irq(struct ast2600_i2c_bus *i2c_bus)
> +{
> +	u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
> +	u32 ctrl;
> +
> +	sts &= ~AST2600_I2CM_SMBUS_ALERT;
> +
> +	if (sts & AST2600_I2CM_BUS_RECOVER_FAIL) {
> +		writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR);
> +		ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		WRITE_ONCE(i2c_bus->msgs, NULL);
> +		i2c_bus->cmd_err = -EPROTO;
> +		complete(&i2c_bus->cmd_complete);
> +		return 1;
> +	}

[Severity: Medium]
Since the AST2600_I2CM_BUS_RECOVER_FAIL bit is never enabled in the
Interrupt Enable Register (IER), will this error handling block ever execute?

[ ... ]

> +static int ast2600_i2c_controller_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{

[ ... ]

> +	timeout = wait_for_completion_timeout(&i2c_bus->cmd_complete, i2c_bus->adap.timeout);
> +	if (timeout == 0) {
> +		u32 ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> +		dev_dbg(i2c_bus->dev, "timeout isr[%x], sts[%x]\n",
> +			readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> +			readl(i2c_bus->reg_base + AST2600_I2CC_STS_AND_BUFF));
> +
> +		writel(0, i2c_bus->reg_base + AST2600_I2CM_IER);
> +		synchronize_irq(i2c_bus->irq);
> +		writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR),
> +		       i2c_bus->reg_base + AST2600_I2CM_ISR);
> +
> +		writel(ctrl & ~AST2600_I2CC_MASTER_EN, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +		writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
> +
> +		WRITE_ONCE(i2c_bus->msgs, NULL);
> +		writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +		       i2c_bus->reg_base + AST2600_I2CM_IER);

[Severity: Medium]
Should this mask include AST2600_I2CM_BUS_RECOVER_FAIL?

If hardware attempts bus recovery and fails, it sets the BUS_RECOVER_FAIL
bit in the ISR. However, because it isn't enabled in the IER, the CPU won't
get the interrupt and will instead wait out the full completion timeout
in ast2600_i2c_recover_bus() rather than failing immediately.

[ ... ]

> +static int ast2600_i2c_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = devm_request_irq(dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
> +			       dev_name(dev), i2c_bus);
> +	if (ret < 0) {
> +		ret = dev_err_probe(dev, ret, "Unable to request irq %d\n",
> +				    i2c_bus->irq);
> +		goto err;
> +	}
> +
> +	writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
> +	       i2c_bus->reg_base + AST2600_I2CM_IER);

[Severity: Medium]
Similarly here in ast2600_i2c_probe(), does the initial IER configuration
need to enable AST2600_I2CM_BUS_RECOVER_FAIL to ensure instant recovery
failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-upstream_i2c-v33-0-6d5338fc56ed@aspeedtech.com?part=4