diff mbox

hwmon: tmp103: use SIMPLE_DEV_PM_OPS helper macro

Message ID 1492956651-15321-1-git-send-email-rahulbedarkar89@gmail.com (mailing list archive)
State Accepted
Headers show

Commit Message

Rahul Bedarkar April 23, 2017, 2:10 p.m. UTC 
Replace ifdefs with SIMPLE_DEV_PM_OPS helper macro.

Signed-off-by: Rahul Bedarkar <rahulbedarkar89@gmail.com>
---
 drivers/hwmon/tmp103.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

Comments

Guenter Roeck April 23, 2017, 3:43 p.m. UTC | #1 
Hi Rahul,

On 04/23/2017 07:10 AM, Rahul Bedarkar wrote:
> Replace ifdefs with SIMPLE_DEV_PM_OPS helper macro.
>
> Signed-off-by: Rahul Bedarkar <rahulbedarkar89@gmail.com>

Thanks a lot for your patch.

While I in general prefer code that avoids #ifdef, I have seen patches
which do the opposite when handling PM code. It appears that it is
unsettled if __maybe_unused or #ifdef should be used in such situations.
Until that is the case, I won't accept patches changing one into another
unless they are from the driver author or Acked by the driver author.

Thanks,
Guenter

> ---
>  drivers/hwmon/tmp103.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c
> index d0bb28b..7f85b14 100644
> --- a/drivers/hwmon/tmp103.c
> +++ b/drivers/hwmon/tmp103.c
> @@ -150,8 +150,7 @@ static int tmp103_probe(struct i2c_client *client,
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>
> -#ifdef CONFIG_PM
> -static int tmp103_suspend(struct device *dev)
> +static int __maybe_unused tmp103_suspend(struct device *dev)
>  {
>  	struct regmap *regmap = dev_get_drvdata(dev);
>
> @@ -159,7 +158,7 @@ static int tmp103_suspend(struct device *dev)
>  				  TMP103_CONF_SD_MASK, 0);
>  }
>
> -static int tmp103_resume(struct device *dev)
> +static int __maybe_unused tmp103_resume(struct device *dev)
>  {
>  	struct regmap *regmap = dev_get_drvdata(dev);
>
> @@ -167,15 +166,7 @@ static int tmp103_resume(struct device *dev)
>  				  TMP103_CONF_SD_MASK, TMP103_CONF_SD);
>  }
>
> -static const struct dev_pm_ops tmp103_dev_pm_ops = {
> -	.suspend	= tmp103_suspend,
> -	.resume		= tmp103_resume,
> -};
> -
> -#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops)
> -#else
> -#define	TMP103_DEV_PM_OPS NULL
> -#endif /* CONFIG_PM */
> +static SIMPLE_DEV_PM_OPS(tmp103_dev_pm_ops, tmp103_suspend, tmp103_resume);
>
>  static const struct i2c_device_id tmp103_id[] = {
>  	{ "tmp103", 0 },
> @@ -193,7 +184,7 @@ static struct i2c_driver tmp103_driver = {
>  	.driver = {
>  		.name	= "tmp103",
>  		.of_match_table = of_match_ptr(tmp103_of_match),
> -		.pm	= TMP103_DEV_PM_OPS,
> +		.pm	= &tmp103_dev_pm_ops,
>  	},
>  	.probe		= tmp103_probe,
>  	.id_table	= tmp103_id,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heiko Schocher April 24, 2017, 3:58 a.m. UTC | #2 
Hello Guenter, Rahul,

Am 23.04.2017 um 17:43 schrieb Guenter Roeck:
> Hi Rahul,
>
> On 04/23/2017 07:10 AM, Rahul Bedarkar wrote:
>> Replace ifdefs with SIMPLE_DEV_PM_OPS helper macro.
>>
>> Signed-off-by: Rahul Bedarkar <rahulbedarkar89@gmail.com>
>
> Thanks a lot for your patch.
>
> While I in general prefer code that avoids #ifdef, I have seen patches
> which do the opposite when handling PM code. It appears that it is
> unsettled if __maybe_unused or #ifdef should be used in such situations.
> Until that is the case, I won't accept patches changing one into another
> unless they are from the driver author or Acked by the driver author.

Hmm.. I like this patch too, but have also no idea, what is preffered.

Looking into drivers/hwmon

pollux:linux hs [master] $ grep -lr __maybe_unused drivers/hwmon/
drivers/hwmon/tmp108.c
drivers/hwmon/nct6775.c
drivers/hwmon/hwmon-vid.c
drivers/hwmon/max31722.c

Ok, there are hwmon drivers, which use this version already ...

pollux:linux hs [master] $ grep -lr CONFIG_PM drivers/hwmon/
drivers/hwmon/max6639.c
drivers/hwmon/jc42.c
drivers/hwmon/fam15h_power.c
drivers/hwmon/tmp102.c
drivers/hwmon/gpio-fan.c
drivers/hwmon/pwm-fan.c
drivers/hwmon/tmp103.c
drivers/hwmon/pmbus/Makefile
drivers/hwmon/lm75.c
drivers/hwmon/nct6683.c
drivers/hwmon/adt7x10.h
drivers/hwmon/w83627hf.c
drivers/hwmon/abituguru3.c
drivers/hwmon/Makefile
drivers/hwmon/acpi_power_meter.c
drivers/hwmon/adt7x10.c
drivers/hwmon/abituguru.c
drivers/hwmon/w83627ehf.c

May this conversion should be done in a patch, which touches more
devices?

Nevertheless, I like this approach, so:

Acked-by: Heiko Schocher <hs@denx.de>

bye,
Heiko

>
> Thanks,
> Guenter
>
>> ---
>>  drivers/hwmon/tmp103.c | 17 ++++-------------
>>  1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c
>> index d0bb28b..7f85b14 100644
>> --- a/drivers/hwmon/tmp103.c
>> +++ b/drivers/hwmon/tmp103.c
>> @@ -150,8 +150,7 @@ static int tmp103_probe(struct i2c_client *client,
>>      return PTR_ERR_OR_ZERO(hwmon_dev);
>>  }
>>
>> -#ifdef CONFIG_PM
>> -static int tmp103_suspend(struct device *dev)
>> +static int __maybe_unused tmp103_suspend(struct device *dev)
>>  {
>>      struct regmap *regmap = dev_get_drvdata(dev);
>>
>> @@ -159,7 +158,7 @@ static int tmp103_suspend(struct device *dev)
>>                    TMP103_CONF_SD_MASK, 0);
>>  }
>>
>> -static int tmp103_resume(struct device *dev)
>> +static int __maybe_unused tmp103_resume(struct device *dev)
>>  {
>>      struct regmap *regmap = dev_get_drvdata(dev);
>>
>> @@ -167,15 +166,7 @@ static int tmp103_resume(struct device *dev)
>>                    TMP103_CONF_SD_MASK, TMP103_CONF_SD);
>>  }
>>
>> -static const struct dev_pm_ops tmp103_dev_pm_ops = {
>> -    .suspend    = tmp103_suspend,
>> -    .resume        = tmp103_resume,
>> -};
>> -
>> -#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops)
>> -#else
>> -#define    TMP103_DEV_PM_OPS NULL
>> -#endif /* CONFIG_PM */
>> +static SIMPLE_DEV_PM_OPS(tmp103_dev_pm_ops, tmp103_suspend, tmp103_resume);
>>
>>  static const struct i2c_device_id tmp103_id[] = {
>>      { "tmp103", 0 },
>> @@ -193,7 +184,7 @@ static struct i2c_driver tmp103_driver = {
>>      .driver = {
>>          .name    = "tmp103",
>>          .of_match_table = of_match_ptr(tmp103_of_match),
>> -        .pm    = TMP103_DEV_PM_OPS,
>> +        .pm    = &tmp103_dev_pm_ops,
>>      },
>>      .probe        = tmp103_probe,
>>      .id_table    = tmp103_id,
>>
>
Rahul Bedarkar April 24, 2017, 6:49 a.m. UTC | #3 
Hello Guenter, Heiko,

On Mon, Apr 24, 2017 at 9:28 AM, Heiko Schocher <hs@denx.de> wrote:
> Hello Guenter, Rahul,
>
> Hmm.. I like this patch too, but have also no idea, what is preffered.
>
> Looking into drivers/hwmon
>
> pollux:linux hs [master] $ grep -lr __maybe_unused drivers/hwmon/
> drivers/hwmon/tmp108.c
> drivers/hwmon/nct6775.c
> drivers/hwmon/hwmon-vid.c
> drivers/hwmon/max31722.c
>
> Ok, there are hwmon drivers, which use this version already ...

Yes, some hwmon drivers already use this approach. Some drivers in
other sub systems also using it from start or moving towards this
approach.

>
> pollux:linux hs [master] $ grep -lr CONFIG_PM drivers/hwmon/
> drivers/hwmon/max6639.c
> drivers/hwmon/jc42.c
> drivers/hwmon/fam15h_power.c
> drivers/hwmon/tmp102.c
> drivers/hwmon/gpio-fan.c
> drivers/hwmon/pwm-fan.c
> drivers/hwmon/tmp103.c
> drivers/hwmon/pmbus/Makefile
> drivers/hwmon/lm75.c
> drivers/hwmon/nct6683.c
> drivers/hwmon/adt7x10.h
> drivers/hwmon/w83627hf.c
> drivers/hwmon/abituguru3.c
> drivers/hwmon/Makefile
> drivers/hwmon/acpi_power_meter.c
> drivers/hwmon/adt7x10.c
> drivers/hwmon/abituguru.c
> drivers/hwmon/w83627ehf.c
>
> May this conversion should be done in a patch, which touches more
> devices?

I'm happy send patches converting remaining drivers once this is
settled or accepted.

Thanks for your reviews.

Regards,
Rahul
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck April 24, 2017, 3:44 p.m. UTC | #4 
On Mon, Apr 24, 2017 at 12:19:18PM +0530, Rahul Bedarkar wrote:
> Hello Guenter, Heiko,
> 
> On Mon, Apr 24, 2017 at 9:28 AM, Heiko Schocher <hs@denx.de> wrote:
> > Hello Guenter, Rahul,
> >
> > Hmm.. I like this patch too, but have also no idea, what is preffered.
> >
> > Looking into drivers/hwmon
> >
> > pollux:linux hs [master] $ grep -lr __maybe_unused drivers/hwmon/
> > drivers/hwmon/tmp108.c
> > drivers/hwmon/nct6775.c
> > drivers/hwmon/hwmon-vid.c
> > drivers/hwmon/max31722.c
> >
> > Ok, there are hwmon drivers, which use this version already ...
> 
> Yes, some hwmon drivers already use this approach. Some drivers in
> other sub systems also using it from start or moving towards this
> approach.
> 
Yes, but as I mentioned it is unsettled if one or the other approach
is preferred, which makes me a bit wary. I'll be open to accepting
patches for jc42 and nct6883 since I am the author of those drivers.

Thanks,
Guenter

> >
> > pollux:linux hs [master] $ grep -lr CONFIG_PM drivers/hwmon/
> > drivers/hwmon/max6639.c
> > drivers/hwmon/jc42.c
> > drivers/hwmon/fam15h_power.c
> > drivers/hwmon/tmp102.c
> > drivers/hwmon/gpio-fan.c
> > drivers/hwmon/pwm-fan.c
> > drivers/hwmon/tmp103.c
> > drivers/hwmon/pmbus/Makefile
> > drivers/hwmon/lm75.c
> > drivers/hwmon/nct6683.c
> > drivers/hwmon/adt7x10.h
> > drivers/hwmon/w83627hf.c
> > drivers/hwmon/abituguru3.c
> > drivers/hwmon/Makefile
> > drivers/hwmon/acpi_power_meter.c
> > drivers/hwmon/adt7x10.c
> > drivers/hwmon/abituguru.c
> > drivers/hwmon/w83627ehf.c
> >
> > May this conversion should be done in a patch, which touches more
> > devices?
> 
> I'm happy send patches converting remaining drivers once this is
> settled or accepted.
> 
> Thanks for your reviews.
> 
> Regards,
> Rahul
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck April 24, 2017, 3:45 p.m. UTC | #5 
On Mon, Apr 24, 2017 at 05:58:18AM +0200, Heiko Schocher wrote:
> Hello Guenter, Rahul,
> 
> Am 23.04.2017 um 17:43 schrieb Guenter Roeck:
> >Hi Rahul,
> >
> >On 04/23/2017 07:10 AM, Rahul Bedarkar wrote:
> >>Replace ifdefs with SIMPLE_DEV_PM_OPS helper macro.
> >>
> >>Signed-off-by: Rahul Bedarkar <rahulbedarkar89@gmail.com>
> >
> >Thanks a lot for your patch.
> >
> >While I in general prefer code that avoids #ifdef, I have seen patches
> >which do the opposite when handling PM code. It appears that it is
> >unsettled if __maybe_unused or #ifdef should be used in such situations.
> >Until that is the case, I won't accept patches changing one into another
> >unless they are from the driver author or Acked by the driver author.
> 
> Hmm.. I like this patch too, but have also no idea, what is preffered.
> 
> Looking into drivers/hwmon
> 
> pollux:linux hs [master] $ grep -lr __maybe_unused drivers/hwmon/
> drivers/hwmon/tmp108.c
> drivers/hwmon/nct6775.c
> drivers/hwmon/hwmon-vid.c
> drivers/hwmon/max31722.c
> 
> Ok, there are hwmon drivers, which use this version already ...
> 
> pollux:linux hs [master] $ grep -lr CONFIG_PM drivers/hwmon/
> drivers/hwmon/max6639.c
> drivers/hwmon/jc42.c
> drivers/hwmon/fam15h_power.c
> drivers/hwmon/tmp102.c
> drivers/hwmon/gpio-fan.c
> drivers/hwmon/pwm-fan.c
> drivers/hwmon/tmp103.c
> drivers/hwmon/pmbus/Makefile
> drivers/hwmon/lm75.c
> drivers/hwmon/nct6683.c
> drivers/hwmon/adt7x10.h
> drivers/hwmon/w83627hf.c
> drivers/hwmon/abituguru3.c
> drivers/hwmon/Makefile
> drivers/hwmon/acpi_power_meter.c
> drivers/hwmon/adt7x10.c
> drivers/hwmon/abituguru.c
> drivers/hwmon/w83627ehf.c
> 
> May this conversion should be done in a patch, which touches more
> devices?
> 
> Nevertheless, I like this approach, so:
> 
> Acked-by: Heiko Schocher <hs@denx.de>
> 
Thanks, applied to hwmon-next.

Guenter

> bye,
> Heiko
> 
> >
> >Thanks,
> >Guenter
> >
> >>---
> >> drivers/hwmon/tmp103.c | 17 ++++-------------
> >> 1 file changed, 4 insertions(+), 13 deletions(-)
> >>
> >>diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c
> >>index d0bb28b..7f85b14 100644
> >>--- a/drivers/hwmon/tmp103.c
> >>+++ b/drivers/hwmon/tmp103.c
> >>@@ -150,8 +150,7 @@ static int tmp103_probe(struct i2c_client *client,
> >>     return PTR_ERR_OR_ZERO(hwmon_dev);
> >> }
> >>
> >>-#ifdef CONFIG_PM
> >>-static int tmp103_suspend(struct device *dev)
> >>+static int __maybe_unused tmp103_suspend(struct device *dev)
> >> {
> >>     struct regmap *regmap = dev_get_drvdata(dev);
> >>
> >>@@ -159,7 +158,7 @@ static int tmp103_suspend(struct device *dev)
> >>                   TMP103_CONF_SD_MASK, 0);
> >> }
> >>
> >>-static int tmp103_resume(struct device *dev)
> >>+static int __maybe_unused tmp103_resume(struct device *dev)
> >> {
> >>     struct regmap *regmap = dev_get_drvdata(dev);
> >>
> >>@@ -167,15 +166,7 @@ static int tmp103_resume(struct device *dev)
> >>                   TMP103_CONF_SD_MASK, TMP103_CONF_SD);
> >> }
> >>
> >>-static const struct dev_pm_ops tmp103_dev_pm_ops = {
> >>-    .suspend    = tmp103_suspend,
> >>-    .resume        = tmp103_resume,
> >>-};
> >>-
> >>-#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops)
> >>-#else
> >>-#define    TMP103_DEV_PM_OPS NULL
> >>-#endif /* CONFIG_PM */
> >>+static SIMPLE_DEV_PM_OPS(tmp103_dev_pm_ops, tmp103_suspend, tmp103_resume);
> >>
> >> static const struct i2c_device_id tmp103_id[] = {
> >>     { "tmp103", 0 },
> >>@@ -193,7 +184,7 @@ static struct i2c_driver tmp103_driver = {
> >>     .driver = {
> >>         .name    = "tmp103",
> >>         .of_match_table = of_match_ptr(tmp103_of_match),
> >>-        .pm    = TMP103_DEV_PM_OPS,
> >>+        .pm    = &tmp103_dev_pm_ops,
> >>     },
> >>     .probe        = tmp103_probe,
> >>     .id_table    = tmp103_id,
> >>
> >
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/hwmon/tmp103.c b/drivers/hwmon/tmp103.c
index d0bb28b..7f85b14 100644
--- a/drivers/hwmon/tmp103.c
+++ b/drivers/hwmon/tmp103.c
@@ -150,8 +150,7 @@  static int tmp103_probe(struct i2c_client *client,
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-#ifdef CONFIG_PM
-static int tmp103_suspend(struct device *dev)
+static int __maybe_unused tmp103_suspend(struct device *dev)
 {
 	struct regmap *regmap = dev_get_drvdata(dev);
 
@@ -159,7 +158,7 @@  static int tmp103_suspend(struct device *dev)
 				  TMP103_CONF_SD_MASK, 0);
 }
 
-static int tmp103_resume(struct device *dev)
+static int __maybe_unused tmp103_resume(struct device *dev)
 {
 	struct regmap *regmap = dev_get_drvdata(dev);
 
@@ -167,15 +166,7 @@  static int tmp103_resume(struct device *dev)
 				  TMP103_CONF_SD_MASK, TMP103_CONF_SD);
 }
 
-static const struct dev_pm_ops tmp103_dev_pm_ops = {
-	.suspend	= tmp103_suspend,
-	.resume		= tmp103_resume,
-};
-
-#define TMP103_DEV_PM_OPS (&tmp103_dev_pm_ops)
-#else
-#define	TMP103_DEV_PM_OPS NULL
-#endif /* CONFIG_PM */
+static SIMPLE_DEV_PM_OPS(tmp103_dev_pm_ops, tmp103_suspend, tmp103_resume);
 
 static const struct i2c_device_id tmp103_id[] = {
 	{ "tmp103", 0 },
@@ -193,7 +184,7 @@  static struct i2c_driver tmp103_driver = {
 	.driver = {
 		.name	= "tmp103",
 		.of_match_table = of_match_ptr(tmp103_of_match),
-		.pm	= TMP103_DEV_PM_OPS,
+		.pm	= &tmp103_dev_pm_ops,
 	},
 	.probe		= tmp103_probe,
 	.id_table	= tmp103_id,