| Message ID | 1312394960-21689-3-git-send-email-khilman@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote: > Current usage of runtime PM is not quite correct. The actual > idle/unidle of the I2C hardware should not happen until the runtime PM > callbacks are called. Therefore, change omap_i2c_[un]idle() functions > to only be called from the runtime PM callbacks (when usage count > transitions to/from zero.) > > Also, the runtime PM core does usage counting and replaces > functionality currently managed by the dev->idle flag. Remove usage > of dev->idle in favor of using runtime PM, and checking status using > pm_runtime_suspended(). > > Signed-off-by: Kevin Hilman <khilman@ti.com> > --- > drivers/i2c/busses/i2c-omap.c | 58 ++++++++++++++++++++++++++-------------- > 1 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index 12d0cbc..1b5325b 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -194,7 +194,6 @@ struct omap_i2c_dev { > */ > u8 rev; > unsigned b_hw:1; /* bad h/w fixes */ > - unsigned idle:1; > u16 iestate; /* Saved interrupt register */ > u16 pscstate; > u16 scllstate; > @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) > { > struct omap_i2c_bus_platform_data *pdata; > > - WARN_ON(!dev->idle); > - > pdata = dev->dev->platform_data; > > - pm_runtime_get_sync(dev->dev); > - > if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); > @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) > omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); > omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > } > - dev->idle = 0; > > /* > * Don't write to this register if the IE state is 0 as it can > @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) > struct omap_i2c_bus_platform_data *pdata; > u16 iv; > > - WARN_ON(dev->idle); > - > pdata = dev->dev->platform_data; > > dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); > @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) > } else { > omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate); > > - /* Flush posted write before the dev->idle store occurs */ > + /* Flush posted write */ > omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); > } > - dev->idle = 1; > - > - pm_runtime_put_sync(dev->dev); > } > > static int omap_i2c_init(struct omap_i2c_dev *dev) > @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > int i; > int r; > > - omap_i2c_unidle(dev); > + pm_runtime_get_sync(dev->dev); > > r = omap_i2c_wait_for_bb(dev); > if (r < 0) > @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) > > omap_i2c_wait_for_bb(dev); > out: > - omap_i2c_idle(dev); > + pm_runtime_put_sync(dev->dev); I wonder if these pm_runtime_put need to be synchronous ? Could we just call pm_runtime_put() instead ? Ditto to all other. > @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_RUNTIME > +static int omap_i2c_runtime_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); what happened to dev_get_drvdata(dev) ?? > + omap_i2c_idle(_dev); > + > + return 0; > +} > + > +static int omap_i2c_runtime_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); ditto > + omap_i2c_unidle(_dev); > + > + return 0; > +} > + > +static struct dev_pm_ops omap_i2c_pm_ops = { > + .runtime_suspend = omap_i2c_runtime_suspend, > + .runtime_resume = omap_i2c_runtime_resume, > +}; > +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops) > +#else > +#define OMAP_I2C_PM_OPS NULL > +#endif OMAP_I2C_PM_OPS isn't used anywhere ??
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote: >> Current usage of runtime PM is not quite correct. The actual >> idle/unidle of the I2C hardware should not happen until the runtime PM >> callbacks are called. Therefore, change omap_i2c_[un]idle() functions >> to only be called from the runtime PM callbacks (when usage count >> transitions to/from zero.) >> >> Also, the runtime PM core does usage counting and replaces >> functionality currently managed by the dev->idle flag. Remove usage >> of dev->idle in favor of using runtime PM, and checking status using >> pm_runtime_suspended(). >> >> Signed-off-by: Kevin Hilman <khilman@ti.com> >> --- >> drivers/i2c/busses/i2c-omap.c | 58 ++++++++++++++++++++++++++-------------- >> 1 files changed, 38 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index 12d0cbc..1b5325b 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -194,7 +194,6 @@ struct omap_i2c_dev { >> */ >> u8 rev; >> unsigned b_hw:1; /* bad h/w fixes */ >> - unsigned idle:1; >> u16 iestate; /* Saved interrupt register */ >> u16 pscstate; >> u16 scllstate; >> @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) >> { >> struct omap_i2c_bus_platform_data *pdata; >> >> - WARN_ON(!dev->idle); >> - >> pdata = dev->dev->platform_data; >> >> - pm_runtime_get_sync(dev->dev); >> - >> if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { >> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); >> omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); >> @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) >> omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); >> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); >> } >> - dev->idle = 0; >> >> /* >> * Don't write to this register if the IE state is 0 as it can >> @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) >> struct omap_i2c_bus_platform_data *pdata; >> u16 iv; >> >> - WARN_ON(dev->idle); >> - >> pdata = dev->dev->platform_data; >> >> dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); >> @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) >> } else { >> omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate); >> >> - /* Flush posted write before the dev->idle store occurs */ >> + /* Flush posted write */ >> omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); >> } >> - dev->idle = 1; >> - >> - pm_runtime_put_sync(dev->dev); >> } >> >> static int omap_i2c_init(struct omap_i2c_dev *dev) >> @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> int i; >> int r; >> >> - omap_i2c_unidle(dev); >> + pm_runtime_get_sync(dev->dev); >> >> r = omap_i2c_wait_for_bb(dev); >> if (r < 0) >> @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) >> >> omap_i2c_wait_for_bb(dev); >> out: >> - omap_i2c_idle(dev); >> + pm_runtime_put_sync(dev->dev); > > I wonder if these pm_runtime_put need to be synchronous ? Could we just > call pm_runtime_put() instead ? Ditto to all other. Yes, will use asynchronous versions. >> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev) >> return 0; >> } >> >> +#ifdef CONFIG_PM_RUNTIME >> +static int omap_i2c_runtime_suspend(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > > what happened to dev_get_drvdata(dev) ?? > Yes, that would work too since: static inline void *platform_get_drvdata(const struct platform_device *pdev) { return dev_get_drvdata(&pdev->dev); } but IMO, readability is better if the driver does platform_set_drvdata() and then the corresponding platform_get_drvdata() >> + omap_i2c_idle(_dev); >> + >> + return 0; >> +} >> + >> +static int omap_i2c_runtime_resume(struct device *dev) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > > ditto > >> + omap_i2c_unidle(_dev); >> + >> + return 0; >> +} >> + >> +static struct dev_pm_ops omap_i2c_pm_ops = { >> + .runtime_suspend = omap_i2c_runtime_suspend, >> + .runtime_resume = omap_i2c_runtime_resume, >> +}; >> +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops) >> +#else >> +#define OMAP_I2C_PM_OPS NULL >> +#endif > > OMAP_I2C_PM_OPS isn't used anywhere ?? doh thanks for catching Kevin
Hi, On Thu, Aug 04, 2011 at 07:53:37AM -0700, Kevin Hilman wrote: > >> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> +#ifdef CONFIG_PM_RUNTIME > >> +static int omap_i2c_runtime_suspend(struct device *dev) > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > > > > what happened to dev_get_drvdata(dev) ?? > > > > Yes, that would work too since: > > static inline void *platform_get_drvdata(const struct platform_device *pdev) > { > return dev_get_drvdata(&pdev->dev); > } > > but IMO, readability is better if the driver does platform_set_drvdata() > and then the corresponding platform_get_drvdata() fair enough, but if you already have the dev pointer, what's the gain in doing a container_of() just to go back to the dev pointer again ? IMHO, there's really no need for that and just calling dev_get_drvdata() straight is enough. All in all, platform_get_[sg]et_drvdata() are just wrappers for dev_[sg]et_drvdata(). The whole point, was to avoid dev_[sg]et_drvdata(&pdev->dev). It's the same with <drivername>_writel() calls we see on all drivers. Just a wrapper, but you can use __raw_writel() directly if you wish.
Felipe Balbi <balbi@ti.com> writes: > Hi, > > On Thu, Aug 04, 2011 at 07:53:37AM -0700, Kevin Hilman wrote: >> >> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev) >> >> return 0; >> >> } >> >> >> >> +#ifdef CONFIG_PM_RUNTIME >> >> +static int omap_i2c_runtime_suspend(struct device *dev) >> >> +{ >> >> + struct platform_device *pdev = to_platform_device(dev); >> >> + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); >> > >> > what happened to dev_get_drvdata(dev) ?? >> > >> >> Yes, that would work too since: >> >> static inline void *platform_get_drvdata(const struct platform_device *pdev) >> { >> return dev_get_drvdata(&pdev->dev); >> } >> >> but IMO, readability is better if the driver does platform_set_drvdata() >> and then the corresponding platform_get_drvdata() > > fair enough, but if you already have the dev pointer, what's the gain in > doing a container_of() just to go back to the dev pointer again ? There is no gain, but there is no loss either. The compiler is smart enough that the resulting assembly is the same. > IMHO, there's really no need for that and just calling dev_get_drvdata() > straight is enough. All in all, platform_get_[sg]et_drvdata() are just > wrappers for dev_[sg]et_drvdata(). The whole point, was to avoid > dev_[sg]et_drvdata(&pdev->dev). Well, whether or not to use dev_[gs]et_* or platform_[gs]et_* is not relevant to $SUBJECT patch. The current driver does platform_set_drvdata(), so I used platform_get_drvdata() for consistency and readability. If I were to use dev_get* then I would want the correpsonding set changed to dev_set_* also for consistency. If someone wants to change both the sets and gets to the dev_ versions, that's fine with me, but it should be separate patch. Kevin
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 12d0cbc..1b5325b 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -194,7 +194,6 @@ struct omap_i2c_dev { */ u8 rev; unsigned b_hw:1; /* bad h/w fixes */ - unsigned idle:1; u16 iestate; /* Saved interrupt register */ u16 pscstate; u16 scllstate; @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) { struct omap_i2c_bus_platform_data *pdata; - WARN_ON(!dev->idle); - pdata = dev->dev->platform_data; - pm_runtime_get_sync(dev->dev); - if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev) omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate); omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); } - dev->idle = 0; /* * Don't write to this register if the IE state is 0 as it can @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) struct omap_i2c_bus_platform_data *pdata; u16 iv; - WARN_ON(dev->idle); - pdata = dev->dev->platform_data; dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) } else { omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate); - /* Flush posted write before the dev->idle store occurs */ + /* Flush posted write */ omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG); } - dev->idle = 1; - - pm_runtime_put_sync(dev->dev); } static int omap_i2c_init(struct omap_i2c_dev *dev) @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) int i; int r; - omap_i2c_unidle(dev); + pm_runtime_get_sync(dev->dev); r = omap_i2c_wait_for_bb(dev); if (r < 0) @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) omap_i2c_wait_for_bb(dev); out: - omap_i2c_idle(dev); + pm_runtime_put_sync(dev->dev); return r; } @@ -727,7 +716,7 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id) struct omap_i2c_dev *dev = dev_id; u16 iv, w; - if (dev->idle) + if (pm_runtime_suspended(dev->dev)) return IRQ_NONE; iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); @@ -819,7 +808,7 @@ omap_i2c_isr(int this_irq, void *dev_id) pdata = dev->dev->platform_data; - if (dev->idle) + if (pm_runtime_suspended(dev->dev)) return IRQ_NONE; bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); @@ -1021,7 +1010,6 @@ omap_i2c_probe(struct platform_device *pdev) } dev->speed = speed; - dev->idle = 1; dev->dev = &pdev->dev; dev->irq = irq->start; dev->base = ioremap(mem->start, resource_size(mem)); @@ -1040,7 +1028,7 @@ omap_i2c_probe(struct platform_device *pdev) dev->regs = (u8 *)reg_map_ip_v1; pm_runtime_enable(dev->dev); - omap_i2c_unidle(dev); + pm_runtime_get_sync(dev->dev); dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff; @@ -1087,7 +1075,7 @@ omap_i2c_probe(struct platform_device *pdev) dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id, pdata->rev, dev->rev >> 4, dev->rev & 0xf, dev->speed); - omap_i2c_idle(dev); + pm_runtime_put_sync(dev->dev); adap = &dev->adapter; i2c_set_adapdata(adap, dev); @@ -1111,7 +1099,7 @@ err_free_irq: free_irq(dev->irq, dev); err_unuse_clocks: omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); - omap_i2c_idle(dev); + pm_runtime_put_sync(dev->dev); iounmap(dev->base); err_free_mem: platform_set_drvdata(pdev, NULL); @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_RUNTIME +static int omap_i2c_runtime_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); + + omap_i2c_idle(_dev); + + return 0; +} + +static int omap_i2c_runtime_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); + + omap_i2c_unidle(_dev); + + return 0; +} + +static struct dev_pm_ops omap_i2c_pm_ops = { + .runtime_suspend = omap_i2c_runtime_suspend, + .runtime_resume = omap_i2c_runtime_resume, +}; +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops) +#else +#define OMAP_I2C_PM_OPS NULL +#endif + static struct platform_driver omap_i2c_driver = { .probe = omap_i2c_probe, .remove = omap_i2c_remove,
Current usage of runtime PM is not quite correct. The actual idle/unidle of the I2C hardware should not happen until the runtime PM callbacks are called. Therefore, change omap_i2c_[un]idle() functions to only be called from the runtime PM callbacks (when usage count transitions to/from zero.) Also, the runtime PM core does usage counting and replaces functionality currently managed by the dev->idle flag. Remove usage of dev->idle in favor of using runtime PM, and checking status using pm_runtime_suspended(). Signed-off-by: Kevin Hilman <khilman@ti.com> --- drivers/i2c/busses/i2c-omap.c | 58 ++++++++++++++++++++++++++-------------- 1 files changed, 38 insertions(+), 20 deletions(-)