[PATCH 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support
zychen
zychennvt at gmail.com
Mon Mar 2 01:39:54 PST 2026
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 at 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
>
More information about the linux-arm-kernel
mailing list