[PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[Uwe Kleine-König]

One of the first things that the driver's pm_runtime_resume callback
does is to write zero to the OMAP_I2C_CON_REG register.
So there is no need to have the device resumed just to write to the
OMAP_I2C_CON_REG register and the call to pm_runtime_resume_and_get()
can be dropped.

The intended side effect of this commit is to remove an error path of
the function resulting in the remove callback returning a mostly ignored
error code. This prepares changing the prototype of struct
platform_driver's remove callback to return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I'm not completely sure that the reasing in the commit log is sound.
There might at least theoretical side effects of the register write
that are different if the device is fully resumed.

Best regards
Uwe

 drivers/i2c/busses/i2c-omap.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f9ae520aed22..a572f6d994ca 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1522,16 +1522,12 @@ omap_i2c_probe(struct platform_device *pdev)
 static int omap_i2c_remove(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*omap = platform_get_drvdata(pdev);
-	int ret;
 
 	i2c_del_adapter(&omap->adapter);
-	ret = pm_runtime_resume_and_get(&pdev->dev);
-	if (ret < 0)
-		return ret;
 
 	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
+
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
-	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	return 0;
 }

base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.2

Re: [PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[Andreas Kemnade]

Hi,

On Sun,  2 Apr 2023 12:55:18 +0200
Uwe Kleine-König         <u.kleine-koenig@pengutronix.de> wrote:

> One of the first things that the driver's pm_runtime_resume callback
> does is to write zero to the OMAP_I2C_CON_REG register.
> So there is no need to have the device resumed just to write to the
> OMAP_I2C_CON_REG register and the call to pm_runtime_resume_and_get()
> can be dropped.
> 
> The intended side effect of this commit is to remove an error path of
> the function resulting in the remove callback returning a mostly ignored
> error code. This prepares changing the prototype of struct
> platform_driver's remove callback to return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

As far as I have understand the code that runtime resume is needed
for enabling clocks to access the device

Regards,
Andreas

Re: [PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

Hello Andreas,

On Sun, Apr 02, 2023 at 10:50:01PM +0200, Andreas Kemnade wrote:
> On Sun,  2 Apr 2023 12:55:18 +0200
> Uwe Kleine-König         <u.kleine-koenig@pengutronix.de> wrote:
> 
> > One of the first things that the driver's pm_runtime_resume callback
> > does is to write zero to the OMAP_I2C_CON_REG register.
> > So there is no need to have the device resumed just to write to the
> > OMAP_I2C_CON_REG register and the call to pm_runtime_resume_and_get()
> > can be dropped.
> > 
> > The intended side effect of this commit is to remove an error path of
> > the function resulting in the remove callback returning a mostly ignored
> > error code. This prepares changing the prototype of struct
> > platform_driver's remove callback to return void.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> As far as I have understand the code that runtime resume is needed
> for enabling clocks to access the device

The only HW access the .remove() callback does is

	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);

. The driver's runtime resume callback looks as follows:

        pinctrl_pm_select_default_state(dev);

        if (!omap->regs)
                return 0;

        __omap_i2c_init(omap);

and the first thing in in __omap_i2c_init is (also) writing a zero to
OMAP_I2C_CON_REG.

I wouldn't expect that the pinctrl call is a precondition for the
register access?! The check for omap->regs is somehow strange, because
other code locations that do register access just assume that ->regs is
non-NULL.

So if there is some clk handling necessary before the register access,
I'm not aware where it's hidden. Is there some bus or omap specific code
that ensures clk handling?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[Tony Lindgren]

Hi,

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230403 05:48]:
> So if there is some clk handling necessary before the register access,
> I'm not aware where it's hidden. Is there some bus or omap specific code
> that ensures clk handling?

I think the missing part is that the runtime PM calls in the i2c driver
cause the parent ti-sysc interconnect target module device to get enabled
and clocked before accessing the i2c registers.

Regards,

Tony

Re: [PATCH] i2c: omap: Don't do pm_runtime_resume in .remove()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]


> > So if there is some clk handling necessary before the register access,
> > I'm not aware where it's hidden. Is there some bus or omap specific code
> > that ensures clk handling?
> 
> I think the missing part is that the runtime PM calls in the i2c driver
> cause the parent ti-sysc interconnect target module device to get enabled
> and clocked before accessing the i2c registers.

So, this patch is not needed?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2243 bytes --]

If pm_runtime_get() fails in .remove() the driver used to return the
error to the driver core. The only effect of this (compared to returning
zero) is a generic warning that the error value is ignored.

So emit a better warning and return zero to suppress the generic (and
little helpful) message. Also disable runtime PM in the error case.

This prepares changing platform device remove callbacks to return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Thu, Apr 06, 2023 at 08:44:33AM +0200, Wolfram Sang wrote:
> 
> > > So if there is some clk handling necessary before the register access,
> > > I'm not aware where it's hidden. Is there some bus or omap specific code
> > > that ensures clk handling?
> > 
> > I think the missing part is that the runtime PM calls in the i2c driver
> > cause the parent ti-sysc interconnect target module device to get enabled
> > and clocked before accessing the i2c registers.
> 
> So, this patch is not needed?

The patch as is is wrong. For my quest to drop the return value of
platform driver's remove callbacks, I need this patch instead:

 drivers/i2c/busses/i2c-omap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f9ae520aed22..2b4e2be51318 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
 	int ret;
 
 	i2c_del_adapter(&omap->adapter);
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
-		return ret;
+		dev_err(omap->dev, "Failed to resume hardware, skip disable\n");
+	else
+		omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
 
-	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+
 	return 0;
 }
 

base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
-- 
2.39.2


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Tony Lindgren]

Hi,

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
>  	int ret;
>  
>  	i2c_del_adapter(&omap->adapter);
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +
> +	ret = pm_runtime_get_sync(&pdev->dev);

It's better to use pm_runtime_resume_and_get() nowadays in general as
it does pm_runtime_put_noidle() on errors.

Not sure if there are changes needed here, maybe warn and return early
if needed?

Regards,

Tony

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1381 bytes --]

Hello Tony,

On Thu, Apr 13, 2023 at 08:12:22AM +0300, Tony Lindgren wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
> >  	int ret;
> >  
> >  	i2c_del_adapter(&omap->adapter);
> > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +
> > +	ret = pm_runtime_get_sync(&pdev->dev);
> 
> It's better to use pm_runtime_resume_and_get() nowadays in general as
> it does pm_runtime_put_noidle() on errors.

Sticking to pm_runtime_resume_and_get() complicates the change however,
because the function calls pm_runtime_put_sync() already. So with
pm_runtime_resume_and_get() I'd need

	if (ret >= 0)
		pm_runtime_put_sync(&pdev->dev);

instead of a plain

	pm_runtime_put_sync(&pdev->dev);

.

> Not sure if there are changes needed here, maybe warn and return early
> if needed?

The idea of "return early" in a remove callback is exactly what I want
to get rid of.

See
https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de
for an example.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Tony Lindgren]

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 06:24]:
> Hello Tony,
> 
> On Thu, Apr 13, 2023 at 08:12:22AM +0300, Tony Lindgren wrote:
> > * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> > > --- a/drivers/i2c/busses/i2c-omap.c
> > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
> > >  	int ret;
> > >  
> > >  	i2c_del_adapter(&omap->adapter);
> > > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > > +
> > > +	ret = pm_runtime_get_sync(&pdev->dev);
> > 
> > It's better to use pm_runtime_resume_and_get() nowadays in general as
> > it does pm_runtime_put_noidle() on errors.
> 
> Sticking to pm_runtime_resume_and_get() complicates the change however,
> because the function calls pm_runtime_put_sync() already. So with
> pm_runtime_resume_and_get() I'd need
> 
> 	if (ret >= 0)
> 		pm_runtime_put_sync(&pdev->dev);
> 
> instead of a plain
> 
> 	pm_runtime_put_sync(&pdev->dev);

In that case you still need to do pm_runtime_put_noidle()
on errors, so not sure what's the best way here.

> > Not sure if there are changes needed here, maybe warn and return early
> > if needed?
> 
> The idea of "return early" in a remove callback is exactly what I want
> to get rid of.
> 
> See
> https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de
> for an example.

Oh OK. Care to clarify a bit why we are not allowed to return errors
on remove though? Are we getting rid of the return value for remove?
Sorry if I'm not following the cunning plan here :)

Regards,

Tony

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> Oh OK. Care to clarify a bit why we are not allowed to return errors
> on remove though? Are we getting rid of the return value for remove?
> Sorry if I'm not following the cunning plan here :)

Yes, that's the plan. If you look at the caller of the remove functions
(before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):

static void platform_remove(struct device *_dev)
{
        struct platform_driver *drv = to_platform_driver(_dev->driver);
        struct platform_device *dev = to_platform_device(_dev);

        if (drv->remove) {
                int ret = drv->remove(dev);

                if (ret)
                        dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
        }
        dev_pm_domain_detach(_dev, true);
}

you see it's pointless to return an error value. But the prototype
seduces driver authors to do it yielding to error that can easily
prevented if .remove returns void. See also
5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
of the quest.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Tony Lindgren]

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > on remove though? Are we getting rid of the return value for remove?
> > Sorry if I'm not following the cunning plan here :)
> 
> Yes, that's the plan. If you look at the caller of the remove functions
> (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> 
> static void platform_remove(struct device *_dev)
> {
>         struct platform_driver *drv = to_platform_driver(_dev->driver);
>         struct platform_device *dev = to_platform_device(_dev);
> 
>         if (drv->remove) {
>                 int ret = drv->remove(dev);
> 
>                 if (ret)
>                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
>         }
>         dev_pm_domain_detach(_dev, true);
> }
> 
> you see it's pointless to return an error value. But the prototype
> seduces driver authors to do it yielding to error that can easily
> prevented if .remove returns void. See also
> 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> of the quest.

OK thanks. So maybe check the pm_runtime_get_sync() and on error do
pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
are fine for me, maybe you already figured it out.

Regards,

Tony

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

Hello Tony,

On Thu, Apr 13, 2023 at 10:11:24AM +0300, Tony Lindgren wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> > On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > > on remove though? Are we getting rid of the return value for remove?
> > > Sorry if I'm not following the cunning plan here :)
> > 
> > Yes, that's the plan. If you look at the caller of the remove functions
> > (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> > 
> > static void platform_remove(struct device *_dev)
> > {
> >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> >         struct platform_device *dev = to_platform_device(_dev);
> > 
> >         if (drv->remove) {
> >                 int ret = drv->remove(dev);
> > 
> >                 if (ret)
> >                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> >         }
> >         dev_pm_domain_detach(_dev, true);
> > }
> > 
> > you see it's pointless to return an error value. But the prototype
> > seduces driver authors to do it yielding to error that can easily
> > prevented if .remove returns void. See also
> > 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> > of the quest.
> 
> OK thanks. So maybe check the pm_runtime_get_sync() and on error do
> pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
> are fine for me, maybe you already figured it out.

Is this an Ack for my patch?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Tony Lindgren]

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:37]:
> Hello Tony,
> 
> On Thu, Apr 13, 2023 at 10:11:24AM +0300, Tony Lindgren wrote:
> > * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> > > On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > > > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > > > on remove though? Are we getting rid of the return value for remove?
> > > > Sorry if I'm not following the cunning plan here :)
> > > 
> > > Yes, that's the plan. If you look at the caller of the remove functions
> > > (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> > > 
> > > static void platform_remove(struct device *_dev)
> > > {
> > >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> > >         struct platform_device *dev = to_platform_device(_dev);
> > > 
> > >         if (drv->remove) {
> > >                 int ret = drv->remove(dev);
> > > 
> > >                 if (ret)
> > >                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> > >         }
> > >         dev_pm_domain_detach(_dev, true);
> > > }
> > > 
> > > you see it's pointless to return an error value. But the prototype
> > > seduces driver authors to do it yielding to error that can easily
> > > prevented if .remove returns void. See also
> > > 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> > > of the quest.
> > 
> > OK thanks. So maybe check the pm_runtime_get_sync() and on error do
> > pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
> > are fine for me, maybe you already figured it out.
> 
> Is this an Ack for my patch?

Yes looking at it again:

Reviewed-by: Tony Lindgren <tony@atomide.com>

Thanks,

Tony

Re: [PATCH] i2c: omap: Improve error reporting for problems during .remove()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On Thu, Apr 06, 2023 at 10:23:54AM +0200, Uwe Kleine-König wrote:
> If pm_runtime_get() fails in .remove() the driver used to return the
> error to the driver core. The only effect of this (compared to returning
> zero) is a generic warning that the error value is ignored.
> 
> So emit a better warning and return zero to suppress the generic (and
> little helpful) message. Also disable runtime PM in the error case.
> 
> This prepares changing platform device remove callbacks to return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]