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

[zychen <zychennvt@gmail.com>]

Hi Krzysztof,
	Thank you for the review.

Krzysztof Kozlowski 於 2026/3/2 下午 03:24 寫道:
> On Mon, Mar 02, 2026 at 02:08:21AM +0000, Zi-Yu Chen wrote:
>> Add I2C support for Nuvoton MA35D1 SoC.
>> The controller supports standard, fast and fast-plus modes,
>> and provides master/slave functionality.
>>
>> Signed-off-by: Zi-Yu Chen <zychennvt@gmail.com>
>> ---
>>  drivers/i2c/busses/Kconfig      |  13 +
>>  drivers/i2c/busses/Makefile     |   1 +
>>  drivers/i2c/busses/i2c-ma35d1.c | 819 ++++++++++++++++++++++++++++++++
>>  3 files changed, 833 insertions(+)
>>  create mode 100644 drivers/i2c/busses/i2c-ma35d1.c
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index e11d50750e63..6bf8be1d2575 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1589,4 +1589,17 @@ config I2C_VIRTIO
>>            This driver can also be built as a module. If so, the module
>>            will be called i2c-virtio.
>>  
>> +config I2C_MA35D1
>> +	tristate "Nuvoton MA35D1 I2C driver"
>> +	depends on ARCH_MA35
> 
> 
> Missing COMPILE_TEST
Will add || COMPILE_TEST to the dependency in v2.
> 
> ...
> 
>> +	/* Setup info block for the I2C core */
>> +	strscpy(i2c->adap.name, "ma35d1-i2c", sizeof(i2c->adap.name));
>> +	i2c->adap.owner = THIS_MODULE;
>> +	i2c->adap.algo = &ma35d1_i2c_algorithm;
>> +	i2c->adap.retries = 2;
>> +	i2c->adap.algo_data = i2c;
>> +	i2c->adap.dev.parent = &pdev->dev;
>> +	i2c->adap.dev.of_node = pdev->dev.of_node;
>> +	i2c_set_adapdata(&i2c->adap, i2c);
>> +
>> +	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>> +				   &busfreq);
>> +	if (ret) {
>> +		dev_err(i2c->dev, "clock-frequency not specified in DT\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Calculate divider based on the current peripheral clock rate */
>> +	clkdiv = DIV_ROUND_CLOSEST(clk_get_rate(i2c->clk), busfreq * 4) - 1;
>> +	if (clkdiv < 0 || clkdiv > 0xffff) {
>> +		dev_err(dev, "invalid clkdiv value: %d\n", clkdiv);
>> +		return -EINVAL;
>> +	}
>> +
>> +	i2c->irq = platform_get_irq(pdev, 0);
>> +	if (i2c->irq < 0)
>> +		return i2c->irq;
>> +
>> +	platform_set_drvdata(pdev, i2c);
>> +
>> +	pm_runtime_set_autosuspend_delay(dev, I2C_PM_TIMEOUT);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = pm_runtime_get_sync(dev);
>> +	if (ret < 0)
>> +		goto rpm_disable;
>> +
>> +	writel(clkdiv & 0xffff, i2c->regs + MA35_CLKDIV);
>> +
>> +	ret = devm_request_irq(dev, i2c->irq, ma35d1_i2c_irq, IRQF_SHARED,
>> +			       dev_name(dev), i2c);
>> +
> 
> No blank line ever between call and if()
Acknowledged. Will fix in v2.
> 
>> +	if (ret != 0) {
> 
> Write simple and obvious code.
> 
> if (ret)
> 
Acknowledged. I will simplify the error check to if (ret) in v2.
>> +		dev_err(dev, "cannot claim IRQ %d\n", i2c->irq);
>> +		goto rpm_disable;
>> +	}
>> +
>> +	/* Give it another chance if pinctrl used is not ready yet */
>> +	if (ret == -EPROBE_DEFER)
> 
> Pointless and dead code.
Acknowledged. I will remove the redundant -EPROBE_DEFER check and its associated comment in v2
> 
>> +		goto rpm_disable;
>> +
>> +	ret = i2c_add_adapter(&i2c->adap);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add bus to i2c core: %d\n", ret);
>> +		goto rpm_disable;
>> +	}
>> +
>> +	pm_runtime_put_autosuspend(dev);
>> +
>> +	return 0;
>> +
>> +rpm_disable:
>> +	pm_runtime_put_noidle(dev);
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_suspended(dev);
>> +	pm_runtime_dont_use_autosuspend(dev);
>> +	return ret;
>> +}
>> +
>> +static void ma35d1_i2c_remove(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_i2c *i2c = platform_get_drvdata(pdev);
>> +
>> +	i2c_del_adapter(&i2c->adap);
>> +	pm_runtime_disable(&pdev->dev);
>> +}
>> +
>> +static int ma35d1_i2c_suspend(struct device *dev)
>> +{
>> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
>> +	unsigned int val;
>> +
>> +	spin_lock_irq(&i2c->lock);
>> +
>> +	/* Prepare for wake-up from I2C events if slave mode is active */
>> +	if (i2c->slave) {
>> +		val = readl(i2c->regs + MA35_CTL0);
>> +		val |= (MA35_CTL_SI | MA35_CTL_AA);
>> +		writel(val, i2c->regs + MA35_CTL0);
>> +		ma35d1_i2c_enable_irq(i2c);
>> +	}
>> +
>> +	spin_unlock_irq(&i2c->lock);
>> +
>> +	/* 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);
>> +
>> +	enable_irq_wake(i2c->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_i2c_resume(struct device *dev)
>> +{
>> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
>> +	unsigned int val;
>> +
>> +	/* Disable wake-up */
>> +	writel(0x0, i2c->regs + MA35_WKCTL);
>> +
>> +	/* Clear pending wake-up flags */
>> +	val = readl(i2c->regs + MA35_WKSTS);
>> +	writel(val, i2c->regs + MA35_WKSTS);
>> +
>> +	disable_irq_wake(i2c->irq);
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_i2c_runtime_suspend(struct device *dev)
>> +{
>> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
>> +	unsigned int val;
>> +
>> +	/* Disable I2C controller */
>> +	val = readl(i2c->regs + MA35_CTL0);
>> +	val &= ~MA35_CTL_I2CEN;
>> +	writel(val, i2c->regs + MA35_CTL0);
>> +
>> +	clk_disable_unprepare(i2c->clk);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_i2c_runtime_resume(struct device *dev)
>> +{
>> +	struct ma35d1_i2c *i2c = dev_get_drvdata(dev);
>> +	unsigned int val;
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(i2c->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clock in resume\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Enable I2C controller */
>> +	val = readl(i2c->regs + MA35_CTL0);
>> +	val |= MA35_CTL_I2CEN;
>> +	writel(val, i2c->regs + MA35_CTL0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dev_pm_ops ma35d1_i2c_pmops = {
>> +	SYSTEM_SLEEP_PM_OPS(ma35d1_i2c_suspend, ma35d1_i2c_resume)
>> +		RUNTIME_PM_OPS(ma35d1_i2c_runtime_suspend,
>> +			       ma35d1_i2c_runtime_resume, NULL)
>> +};
>> +
>> +static const struct of_device_id ma35d1_i2c_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-i2c" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_i2c_of_match);
>> +
>> +static struct platform_driver ma35d1_i2c_driver = {
>> +	.probe      = ma35d1_i2c_probe,
>> +	.remove     = ma35d1_i2c_remove,
>> +	.driver     = {
>> +		.name   = "ma35d1-i2c",
>> +		.owner  = THIS_MODULE,
> 
> Do not upstream 12-year-old code. We fixed all these issues long time.
> Please write your driver from scratch, so you will not
> repeat/reintroduce all the issues which we already fixed.
> 
I will address these issues in V2 by:

Removing the redundant .owner = THIS_MODULE.

Reviewing the entire driver to ensure all APIs and patterns align with current upstream standards.

I am performing a "from-scratch" review to eliminate legacy patterns.
>> +		.of_match_table = ma35d1_i2c_of_match,
>> +		.pm = pm_ptr(&ma35d1_i2c_pmops),
>> +	},
> 
> Best regards,
> Krzysztof
>