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

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(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

* 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

* 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

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