| Message ID | 1373429409-3944-1-git-send-email-shuah.kh@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi, thanks for your patch > static struct pnp_device_id tpm_pnp_tbl[] = { > {"PNP0C31", 0}, /* TPM */ > {"ATM1200", 0}, /* Atmel */ > @@ -835,9 +834,12 @@ static struct pnp_driver tis_pnp_driver = { > .name = "tpm_tis", > .id_table = tpm_pnp_tbl, > .probe = tpm_tis_pnp_init, > - .suspend = tpm_tis_pnp_suspend, > - .resume = tpm_tis_pnp_resume, > .remove = tpm_tis_pnp_remove, > +#ifdef CONFIG_PM_SLEEP > + .driver = { > + .pm = &tpm_tis_pm, > + }, > +#endif > }; I don't think the #if CONFIG_PM_SLEEP is required here. Thanks, Peter
On 07/10/2013 04:43 PM, Peter Hüwe wrote: > Hi, > > thanks for your patch >> static struct pnp_device_id tpm_pnp_tbl[] = { >> {"PNP0C31", 0}, /* TPM */ >> {"ATM1200", 0}, /* Atmel */ >> @@ -835,9 +834,12 @@ static struct pnp_driver tis_pnp_driver = { >> .name = "tpm_tis", >> .id_table = tpm_pnp_tbl, >> .probe = tpm_tis_pnp_init, >> - .suspend = tpm_tis_pnp_suspend, >> - .resume = tpm_tis_pnp_resume, >> .remove = tpm_tis_pnp_remove, >> +#ifdef CONFIG_PM_SLEEP >> + .driver = { >> + .pm = &tpm_tis_pm, >> + }, >> +#endif >> }; > > > I don't think the #if CONFIG_PM_SLEEP is required here. > > Thanks, > Peter > tpm_tis_resume() is defined originally in CONFIG_PM_SLEEP scope. I can make the change to have tpm_tis_resume() not be in CONFIG_PM_SLEEP scope and remove this CONFIG_PM_SLEEP when defining .pm. That does make sense looking at tpm_pm_suspend() and tpm_pm_resume() which are defined without CONFIG_PM_SLEEP scope. Sounds like the right approach? I will redo the patch and send v2. I find that the use of CONFIG_PM, CONFIG_PM_SLEEP, and CONFIG_PM_RUNTIME are not very consistent. :) -- Shuah Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658
Hi Shuah, thanks for your reply. > >> +#ifdef CONFIG_PM_SLEEP > >> + .driver = { > >> + .pm = &tpm_tis_pm, > >> + }, > >> +#endif > >> > >> }; > > > > I don't think the #if CONFIG_PM_SLEEP is required here. In this case, the SIMPLE_DEV_PM_OPS macro handles the case internally - i.e. no matter whether CONFIG_PM_SLEEP is set or not, the correct structure is set up and thus no ifdef needed. > > tpm_tis_resume() is defined originally in CONFIG_PM_SLEEP scope. I can > make the change to have tpm_tis_resume() not be in CONFIG_PM_SLEEP scope > and remove this CONFIG_PM_SLEEP when defining .pm. > That does make sense looking at tpm_pm_suspend() and tpm_pm_resume() which > are defined ithout CONFIG_PM_SLEEP scope. Sounds like the right approach? > I will redo the patch and send v2. Hmm, at first I thought that would be a good idea, however scrolling to the git history I found: commit 07368d32f1a67e797def08cf2ee3ea1647b204b6 Author: Rafael J. Wysocki <rjw@sisk.pl> Date: Thu Aug 9 23:00:35 2012 +0200 tpm_tis / PM: Fix unused function warning for CONFIG_PM_SLEEP According to a compiler warning, the tpm_tis_resume() function is not used for CONFIG_PM_SLEEP unset, so add a #ifdef to prevent it from being built in that case. Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> So removing it there would effectively revert the patch and re-enable the warning. > I find that the use of CONFIG_PM, CONFIG_PM_SLEEP, and CONFIG_PM_RUNTIME > are not very consistent. :) Yes. Maybe the better idea is to add the correct CONFIG_PM ifdefs for all code paths related to PM. Or leave the CONFIG_PM for tpm_tis_resume as it is. Thanks, Peter
Hi Peter, On 07/10/2013 05:30 PM, Peter Hüwe wrote: >> >> tpm_tis_resume() is defined originally in CONFIG_PM_SLEEP scope. I can >> make the change to have tpm_tis_resume() not be in CONFIG_PM_SLEEP scope >> and remove this CONFIG_PM_SLEEP when defining .pm. >> That does make sense looking at tpm_pm_suspend() and tpm_pm_resume() which >> are defined ithout CONFIG_PM_SLEEP scope. Sounds like the right approach? >> I will redo the patch and send v2. > > Hmm, > at first I thought that would be a good idea, however scrolling to the git > history I found: > > commit 07368d32f1a67e797def08cf2ee3ea1647b204b6 > Author: Rafael J. Wysocki <rjw@sisk.pl> > Date: Thu Aug 9 23:00:35 2012 +0200 > > tpm_tis / PM: Fix unused function warning for CONFIG_PM_SLEEP > > According to a compiler warning, the tpm_tis_resume() function is not > used for CONFIG_PM_SLEEP unset, so add a #ifdef to prevent it from > being built in that case. > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl> > > So removing it there would effectively revert the patch and re-enable the > warning. > > > >> I find that the use of CONFIG_PM, CONFIG_PM_SLEEP, and CONFIG_PM_RUNTIME >> are not very consistent. :) > Yes. > > Maybe the better idea is to add the correct CONFIG_PM ifdefs for all code > paths related to PM. > Or leave the CONFIG_PM for tpm_tis_resume as it is. > > For now, leaving tpm_tis_resume() is better to keep this change simpler. I am seeing this type of inconsistency in several drivers as I am going around making changes to convert from legacy pm_ops to dev_pm_ops. At some point, it might be worth while looking at the usage of these defines and set some clear guidelines. -- Shuah Shuah Khan, Linux Kernel Developer - Open Source Group Samsung Research America (Silicon Valley) shuah.kh@samsung.com | (970) 672-0658
Sounds good. Thanks. Peter
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 4519cb3..5796d01 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -766,6 +766,25 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) } #endif +#ifdef CONFIG_PM_SLEEP +static int tpm_tis_resume(struct device *dev) +{ + struct tpm_chip *chip = dev_get_drvdata(dev); + int ret; + + if (chip->vendor.irq) + tpm_tis_reenable_interrupts(chip); + + ret = tpm_pm_resume(dev); + if (!ret) + tpm_do_selftest(chip); + + return ret; +} +#endif + +static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); + #ifdef CONFIG_PNP static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, const struct pnp_device_id *pnp_id) @@ -787,26 +806,6 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, return tpm_tis_init(&pnp_dev->dev, start, len, irq); } -static int tpm_tis_pnp_suspend(struct pnp_dev *dev, pm_message_t msg) -{ - return tpm_pm_suspend(&dev->dev); -} - -static int tpm_tis_pnp_resume(struct pnp_dev *dev) -{ - struct tpm_chip *chip = pnp_get_drvdata(dev); - int ret; - - if (chip->vendor.irq) - tpm_tis_reenable_interrupts(chip); - - ret = tpm_pm_resume(&dev->dev); - if (!ret) - tpm_do_selftest(chip); - - return ret; -} - static struct pnp_device_id tpm_pnp_tbl[] = { {"PNP0C31", 0}, /* TPM */ {"ATM1200", 0}, /* Atmel */ @@ -835,9 +834,12 @@ static struct pnp_driver tis_pnp_driver = { .name = "tpm_tis", .id_table = tpm_pnp_tbl, .probe = tpm_tis_pnp_init, - .suspend = tpm_tis_pnp_suspend, - .resume = tpm_tis_pnp_resume, .remove = tpm_tis_pnp_remove, +#ifdef CONFIG_PM_SLEEP + .driver = { + .pm = &tpm_tis_pm, + }, +#endif }; #define TIS_HID_USR_IDX sizeof(tpm_pnp_tbl)/sizeof(struct pnp_device_id) -2 @@ -846,20 +848,6 @@ module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id, MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe"); #endif -#ifdef CONFIG_PM_SLEEP -static int tpm_tis_resume(struct device *dev) -{ - struct tpm_chip *chip = dev_get_drvdata(dev); - - if (chip->vendor.irq) - tpm_tis_reenable_interrupts(chip); - - return tpm_pm_resume(dev); -} -#endif - -static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume); - static struct platform_driver tis_drv = { .driver = { .name = "tpm_tis",
Convert drivers/char/tpm/tpm_tis.c to use dev_pm_ops instead of legacy pm_ops. This patch depends on pnp driver bus ops change to invoke pnp_driver dev_pm_ops. Signed-off-by: Shuah Khan <shuah.kh@samsung.com> --- drivers/char/tpm/tpm_tis.c | 60 ++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 36 deletions(-)