Hi Angelo,
On Thu, 2024-11-07 at 11:13 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Il 06/11/24 13:52, Zoie Lin ha scritto:
> > This commit introduces support for runtime PM operations in
> > the I2C driver, enabling runtime suspend and resume functionality.
> >
> > Although in the most platforms, the bus power of i2c are always
> > on, some platforms disable the i2c bus power in order to meet
> > low power request.
> >
> > This implementation includes bus regulator control to facilitate
> > proper handling of the bus power based on platform requirements.
> >
> > Signed-off-by: Zoie Lin <zoie.lin@xxxxxxxxxxxx>
> > ---
> > drivers/i2c/busses/i2c-mt65xx.c | 77
> > ++++++++++++++++++++++++++++-----
> > 1 file changed, 65 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > b/drivers/i2c/busses/i2c-mt65xx.c
> > index 5bd342047d59..4209daec1efa 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
>
> ..snip..
>
> > @@ -1370,6 +1373,40 @@ static int mtk_i2c_parse_dt(struct
> > device_node *np, struct mtk_i2c *i2c)
> > return 0;
> > }
> >
> > +static int mtk_i2c_runtime_suspend(struct device *dev)
> > +{
> > + struct mtk_i2c *i2c = dev_get_drvdata(dev);
> > +
> > + clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > + if (i2c->adap.bus_regulator)
> > + regulator_disable(i2c->adap.bus_regulator);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_i2c_runtime_resume(struct device *dev)
> > +{
> > + int ret = 0;
> > + struct mtk_i2c *i2c = dev_get_drvdata(dev);
>
> struct mtk_i2c *i2c = dev_get_drvdata(dev);
> int ret;
>
> > +
> > + if (i2c->adap.bus_regulator) {
> > + ret = regulator_enable(i2c->adap.bus_regulator);
> > + if (ret) {
> > + dev_err(dev, "enable regulator failed!\n");
> > + return ret;
> > + }
> > + }
> > +
> > + ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > + if (ret) {
> > + if (i2c->adap.bus_regulator)
> > + regulator_disable(i2c->adap.bus_regulator);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int mtk_i2c_probe(struct platform_device *pdev)
> > {
> > int ret = 0;
> > @@ -1472,13 +1509,18 @@ static int mtk_i2c_probe(struct
> > platform_device *pdev)
> > }
> > }
> >
> > - ret = clk_bulk_prepare_enable(I2C_MT65XX_CLK_MAX, i2c-
> > >clocks);
> > + ret = clk_bulk_prepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > if (ret) {
> > - dev_err(&pdev->dev, "clock enable failed!\n");
> > return ret;
> > }
> > +
> > + platform_set_drvdata(pdev, i2c);
> > +
> > + ret = mtk_i2c_runtime_resume(i2c->dev);
> > + if (ret < 0)
> > + goto err_clk_bulk_unprepare;
> > mtk_i2c_init_hw(i2c);
> > - clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > + mtk_i2c_runtime_suspend(i2c->dev);
> >
> > ret = devm_request_irq(&pdev->dev, irq, mtk_i2c_irq,
> > IRQF_NO_SUSPEND | IRQF_TRIGGER_NONE,
> > @@ -1486,19 +1528,22 @@ static int mtk_i2c_probe(struct
> > platform_device *pdev)
> > if (ret < 0) {
> > dev_err(&pdev->dev,
> > "Request I2C IRQ %d fail\n", irq);
> > - goto err_bulk_unprepare;
> > + goto err_clk_bulk_unprepare;
> > }
> > + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>
> You had comments from me and from Andi on this delay, and you
> completely ignored
> both of us.
>
> We're still waiting for an answer to our question.
I am sorry for any confusion caused by my previous response.
The response to your question was included in the cover letter, which
might not have been very noticeable.
The delay before runtime_put_autosuspend() actually executes
mtk_i2c_runtime_suspend() depends on the frequency of I2C usage by the
devices attached to this bus. A 1000ms delay is a balanced value for
latency and power metrics based on the MTK platform.
Thank you.
Best regards,
Zoie
>
>
> > + pm_runtime_use_autosuspend(&pdev->dev);
> > + pm_runtime_enable(&pdev->dev);
>
> devm_pm_runtime_enable() please.
>
> >
> > i2c_set_adapdata(&i2c->adap, i2c);
> > ret = i2c_add_adapter(&i2c->adap);
> > if (ret)
> > - goto err_bulk_unprepare;
> > -
> > - platform_set_drvdata(pdev, i2c);
> > + goto err_pm_runtime_disable;
> >
> > return 0;
> >
> > -err_bulk_unprepare:
> > +err_pm_runtime_disable:
> > + pm_runtime_disable(&pdev->dev);
> > +err_clk_bulk_unprepare:
> > clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
> >
> > return ret;
> > @@ -1510,6 +1555,7 @@ static void mtk_i2c_remove(struct
> > platform_device *pdev)
> >
> > i2c_del_adapter(&i2c->adap);
> >
> > + pm_runtime_disable(&pdev->dev);
> > clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > }
> >
> > @@ -1518,6 +1564,10 @@ static int mtk_i2c_suspend_noirq(struct
> > device *dev)
> > struct mtk_i2c *i2c = dev_get_drvdata(dev);
> >
> > i2c_mark_adapter_suspended(&i2c->adap);
> > +
> > + if (!pm_runtime_status_suspended(dev))
> > + mtk_i2c_runtime_suspend(dev);
> > +
> > clk_bulk_unprepare(I2C_MT65XX_CLK_MAX, i2c->clocks);
> >
> > return 0;
> > @@ -1536,7 +1586,8 @@ static int mtk_i2c_resume_noirq(struct device
> > *dev)
> >
> > mtk_i2c_init_hw(i2c);
> >
> > - clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
> > + if (pm_runtime_status_suspended(dev))
> > + mtk_i2c_runtime_suspend(dev);
>
> You want to resume, not to suspend, in a resume handler.
>
> >
> > i2c_mark_adapter_resumed(&i2c->adap);
> >
> > @@ -1546,6 +1597,8 @@ static int mtk_i2c_resume_noirq(struct device
> > *dev)
> > static const struct dev_pm_ops mtk_i2c_pm = {
> > NOIRQ_SYSTEM_SLEEP_PM_OPS(mtk_i2c_suspend_noirq,
> > mtk_i2c_resume_noirq)
> > + SET_RUNTIME_PM_OPS(mtk_i2c_runtime_suspend,
> > mtk_i2c_runtime_resume,
> > + NULL)
> > };
> >
> > static struct platform_driver mtk_i2c_driver = {
>
>
>
