linux-kernel - Re: [PATCH 2/2] i2c: Add Spreadtrum I2C controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20170621032529.GA1142@spreadtrum.com>
Date:   Wed, 21 Jun 2017 11:25:30 +0800
From:   Baolin Wang <baolin.wang@...eadtrum.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Wolfram Sang <wsa@...-dreams.de>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Baolin Wang <baolin.wang@...aro.org>
Subject: Re: [PATCH 2/2] i2c: Add Spreadtrum I2C controller driver

Hi Andy,

Sorry for late reply due to my business trip.

On 六,  6月 17, 2017 at 08:18:49下午 +0300, Andy Shevchenko wrote:
> On Thu, Jun 15, 2017 at 2:30 PM, Baolin Wang <baolin.wang@...eadtrum.com> wrote:
> > This patch adds the I2C controller driver for Spreadtrum platform.
> 
> 
> > +       i2c_dev->irq = platform_get_irq(pdev, 0);
> > +       if (i2c_dev->irq < 0) {
> > +               dev_err(&pdev->dev, "failed to get irq resource\n");
> 
> > +               return -ENXIO;
> 
> Why shadow actual error?

Sorry for missing this and will fix in next version.

> 
> > +       }
> 
> 
> > +       if (!of_property_read_u32(dev->of_node, "clock-frequency", &prop))
> > +               i2c_dev->bus_freq = prop;
> > +
> > +       sprd_i2c_clk_init(i2c_dev);
> > +       platform_set_drvdata(pdev, i2c_dev);
> > +
> > +       pm_runtime_set_autosuspend_delay(i2c_dev->dev, SPRD_I2C_PM_TIMEOUT);
> > +       pm_runtime_use_autosuspend(i2c_dev->dev);
> > +       pm_runtime_set_active(i2c_dev->dev);
> > +       pm_runtime_enable(i2c_dev->dev);
> > +
> > +       ret = pm_runtime_get_sync(i2c_dev->dev);
> > +       if (ret < 0) {
> > +               dev_err(&pdev->dev, "i2c%d pm runtime resume failed!\n",
> > +                       pdev->id);
> 
> > +               return ret;
> 
> goto error;

Yes, will fix it.

> 
> > +       }
> > +
> > +       ret = devm_request_threaded_irq(dev, i2c_dev->irq,
> > +               sprd_i2c_isr, sprd_i2c_isr_thread,
> > +               IRQF_NO_SUSPEND | IRQF_ONESHOT,
> > +               pdev->name, i2c_dev);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "failed to request irq %d\n", i2c_dev->irq);
> > +               goto error;
> > +       }
> > +
> > +       ret = i2c_add_numbered_adapter(&i2c_dev->adap);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "add adapter failed\n");
> > +               goto error;
> > +       }
> > +
> > +       pm_runtime_mark_last_busy(i2c_dev->dev);
> > +       pm_runtime_put_autosuspend(i2c_dev->dev);
> > +       return 0;
> > +
> > +error:
> > +       pm_runtime_put_noidle(i2c_dev->dev);
> > +       pm_runtime_disable(i2c_dev->dev);
> > +       return ret;
> > +}
> > +
> > +static int sprd_i2c_remove(struct platform_device *pdev)
> > +{
> > +       struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev);
> > +       int ret;
> > +
> 
> > +       ret = pm_runtime_get_sync(i2c_dev->dev);
> > +       if (ret < 0)
> > +               return ret;
> 
> Does it make any sense? Doesn't device core power on the device before
> calling ->remove() ?

Yes, you are right. We need power on device in probe() function.

> 
> > +
> > +       i2c_del_adapter(&i2c_dev->adap);
> > +
> > +       if (!IS_ERR_OR_NULL(i2c_dev->clk))
> 
> _OR_NULL looks suspicious.

Since our ->clk can be NULL as one optional connfig in case we test I2C
driver on FPGA platform which does not support clock operation.

> 
> > +               clk_unprepare(i2c_dev->clk);
> > +
> > +       pm_runtime_put_noidle(i2c_dev->dev);
> > +       pm_runtime_disable(i2c_dev->dev);
> > +
> > +       return 0;
> > +}
> > +
> 
> > +#ifdef CONFIG_PM_SLEEP
> 
> __maybe_unused instead?

OK.

> 
> > +static int sprd_i2c_suspend_noirq(struct device *pdev)
> 
> > +static int sprd_i2c_resume_noirq(struct device *pdev)
> 
> > +#endif  /* CONFIG_PM_SLEEP */
> 
> > +#ifdef CONFIG_PM
> 
> Ditto.

OK.

> 
> > +static int sprd_i2c_runtime_suspend(struct device *pdev)
> 
> > +}
> 
> > +static int sprd_i2c_runtime_resume(struct device *pdev)
> > +{
> 
> > +       clk_prepare_enable(i2c_dev->clk);
> 
> This might fail.

Yes, will check return value.

> 
> 
> > +}
> > +#endif  /* CONFIG_PM */
> 
> 
> > +static struct platform_driver sprd_i2c_driver = {
> > +       .probe = sprd_i2c_probe,
> > +       .remove = sprd_i2c_remove,
> > +       .driver = {
> > +                  .name = "sprd-i2c",
> 
> > +                  .of_match_table = of_match_ptr(sprd_i2c_of_match),
> 
> of_match_ptr seems redundant.

OK.

> 
> > +                  .pm = &sprd_i2c_pm_ops,
> > +       },
> > +};
> > +
> 
> > +static int __init sprd_i2c_init(void)
> > +{
> > +       return platform_driver_register(&sprd_i2c_driver);
> > +}
> 
> > +arch_initcall_sync(sprd_i2c_init);
> 
> Why?

IN our Spreadtrum platform, our regulator driver will depend on I2C driver
and the regulator driver uses subsys_initcall() level to initialize. Moreover
some other drivers like GPU, they will depend on regulator to set voltage and
they also need initialization much earlier. Thanks for your comments.

> 
> -- 
> With Best Regards,
> Andy Shevchenko