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

Commit Message

Uwe Kleine-König April 2, 2023, 10:55 a.m. UTC

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 <[e-mail]>
---
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(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Andreas Kemnade April 2, 2023, 8:50 p.m. UTC | #1

Hi,

On Sun,  2 Apr 2023 12:55:18 +0200
Uwe Kleine-König         <[e-mail]> 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 <[e-mail]>

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

Regards,
Andreas

Uwe Kleine-König April 3, 2023, 5:48 a.m. UTC | #2

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         <[e-mail]> 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 <[e-mail]>
> 
> 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

Tony Lindgren April 3, 2023, 6:04 a.m. UTC | #3

Hi,

* Uwe Kleine-König <[e-mail]> [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

Wolfram Sang April 6, 2023, 6:44 a.m. UTC | #4

> > 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?

Patch

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;
 }