[v2,0/1] i2c: mediatek: add runtime PM operations and bus regulator control

[v2,0/1] i2c: mediatek: add runtime PM operations and bus regulator control

[Zoie Lin]

This series is based on linux-next, tag: next-20241105

Changes in v2:
- author name modification
- replacement of pm runtime API
- removal of redundant error messages
- return value adjustment
- add runtime pm status check

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.

Zoie Lin (1):
  i2c: mediatek: add runtime PM operations and bus regulator control

 drivers/i2c/busses/i2c-mt65xx.c | 77 ++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 12 deletions(-)

-- 
2.45.2

[v2,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

[Zoie Lin]

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@mediatek.com>
---
 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
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -1245,8 +1246,8 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	int left_num = num;
 	struct mtk_i2c *i2c = i2c_get_adapdata(adap);
 
-	ret = clk_bulk_enable(I2C_MT65XX_CLK_MAX, i2c->clocks);
-	if (ret)
+	ret = pm_runtime_resume_and_get(i2c->dev);
+	if (ret < 0)
 		return ret;
 
 	i2c->auto_restart = i2c->dev_comp->auto_restart;
@@ -1299,7 +1300,9 @@ static int mtk_i2c_transfer(struct i2c_adapter *adap,
 	ret = num;
 
 err_exit:
-	clk_bulk_disable(I2C_MT65XX_CLK_MAX, i2c->clocks);
+	pm_runtime_mark_last_busy(i2c->dev);
+	pm_runtime_put_autosuspend(i2c->dev);
+
 	return ret;
 }
 
@@ -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);
+
+	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);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
 
 	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);
 
 	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 = {
-- 
2.45.2

Re: [v2,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

[AngeloGioacchino Del Regno]

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@mediatek.com>
> ---
>   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.


> +	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 = {

Re: [v2,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

[Zoie Lin (林禹妡)]

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@mediatek.com>
> > ---
> >   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 = {
> 
> 
> 

Re: [v2,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

[AngeloGioacchino Del Regno]

Il 07/11/24 16:19, Zoie Lin (林禹妡) ha scritto:
> 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@mediatek.com>
>>> ---
>>>    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.
>   

Can you please write down "the numbers" into the commit description?

As in, to justify why 1000ms is a balanced value for latency and power, so,
write down a small table containing both values for various delays, like:

Delay(ms)    Latency(us)   Power(uW)
200          999           9999
500          9999          99999
1000         99999         999999

Please also say what is the minimum acceptable latency.

Thanks,
Angelo

>   
> 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 = {
>>
>>
>>

Re: [v2,1/1] i2c: mediatek: add runtime PM operations and bus regulator control

[Andi Shyti]

Hi Zoie,

...

> > > @@ -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.

Still you need to make it clear in the code. Please add a good
comment to describe why this delay is needed and where this 1000
has been found.

This is a constant that is not written in the datasheet, please
report how you have achieved this result. I want to avoid
arbitrary numbers inherited from the development phase.

Thanks,
Andi