[PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Paul Cercueil]

Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
to handle the .suspend/.resume callbacks.

These macros allow the suspend and resume functions to be automatically
dropped by the compiler when CONFIG_SUSPEND is disabled, without having
to use #ifdef guards.

The advantage is then that these functions are now always compiled
independently of any Kconfig option, and thanks to that bugs and
regressions are easier to catch.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org
---
 drivers/mfd/sec-core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index 1fb29c45f5cf..a467de2b2fea 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -455,7 +455,6 @@ static void sec_pmic_shutdown(struct i2c_client *i2c)
 	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int sec_pmic_suspend(struct device *dev)
 {
 	struct i2c_client *i2c = to_i2c_client(dev);
@@ -488,14 +487,14 @@ static int sec_pmic_resume(struct device *dev)
 
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
 
-static SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops, sec_pmic_suspend, sec_pmic_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops,
+				sec_pmic_suspend, sec_pmic_resume);
 
 static struct i2c_driver sec_pmic_driver = {
 	.driver = {
 		   .name = "sec_pmic",
-		   .pm = &sec_pmic_pm_ops,
+		   .pm = pm_sleep_ptr(&sec_pmic_pm_ops),
 		   .of_match_table = sec_dt_match,
 	},
 	.probe = sec_pmic_probe,
-- 
2.35.1

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Krzysztof Kozlowski]

On 07/08/2022 17:52, Paul Cercueil wrote:
> Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
> to handle the .suspend/.resume callbacks.
> 
> These macros allow the suspend and resume functions to be automatically
> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> to use #ifdef guards.
> 
> The advantage is then that these functions are now always compiled
> independently of any Kconfig option, and thanks to that bugs and
> regressions are easier to catch.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>

The address does not work. Please don't add it to commit log.

> Cc: linux-samsung-soc@vger.kernel.org

This is also not really needed in commit log... it's just a mailing list...

I actually never understood why people want to add to commit log, so to
something which will last 10 years, Cc-ing other folks, instead of
adding such tags after '---'. Imagine 10 years from now:

1. What's the point to be cced on this patch after 10 years instead of
using maintainers file (the one in 10 years)? Why Cc-ing me in 10 years?
If I am a maintainer of this driver in that time, I will be C-ced based
on maintainers file. If I am not a maintainer in 10 years, why the heck
cc-ing me based on some 10-year old commit? Just because I was a
maintainer once, like 10 years ago?

2. Or why cc-ing such people when backporting to stable?

It's quite a lot of unnecessary emails which many of us won't actually
handle later...

I sincerely admit I was once also adding such Cc-tags. But that time my
employer was counting lines-of-patch (including commit log)... crazy, right?


> ---
>  drivers/mfd/sec-core.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> index 1fb29c45f5cf..a467de2b2fea 100644
> --- a/drivers/mfd/sec-core.c
> +++ b/drivers/mfd/sec-core.c
> @@ -455,7 +455,6 @@ static void sec_pmic_shutdown(struct i2c_client *i2c)
>  	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int sec_pmic_suspend(struct device *dev)

Did you test W=1 with !CONFIG_PM_SLEEP? No warnings?


Best regards,
Krzysztof

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Paul Cercueil]

Hi Krzysztof,

Le lun., août 8 2022 at 12:11:02 +0300, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> a écrit :
> On 07/08/2022 17:52, Paul Cercueil wrote:
>>  Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
>>  to handle the .suspend/.resume callbacks.
>> 
>>  These macros allow the suspend and resume functions to be 
>> automatically
>>  dropped by the compiler when CONFIG_SUSPEND is disabled, without 
>> having
>>  to use #ifdef guards.
>> 
>>  The advantage is then that these functions are now always compiled
>>  independently of any Kconfig option, and thanks to that bugs and
>>  regressions are easier to catch.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>  Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> 
> The address does not work. Please don't add it to commit log.

That's what get-maintainers gave me, and I didn't get any error sending 
at that address. But I'll take your word.

>>  Cc: linux-samsung-soc@vger.kernel.org
> 
> This is also not really needed in commit log... it's just a mailing 
> list...
> 
> I actually never understood why people want to add to commit log, so 
> to
> something which will last 10 years, Cc-ing other folks, instead of
> adding such tags after '---'. Imagine 10 years from now:
> 
> 1. What's the point to be cced on this patch after 10 years instead of
> using maintainers file (the one in 10 years)? Why Cc-ing me in 10 
> years?
> If I am a maintainer of this driver in that time, I will be C-ced 
> based
> on maintainers file. If I am not a maintainer in 10 years, why the 
> heck
> cc-ing me based on some 10-year old commit? Just because I was a
> maintainer once, like 10 years ago?
> 
> 2. Or why cc-ing such people when backporting to stable?
> 
> It's quite a lot of unnecessary emails which many of us won't actually
> handle later...
> 
> I sincerely admit I was once also adding such Cc-tags. But that time 
> my
> employer was counting lines-of-patch (including commit log)... crazy, 
> right?

Yeah, well, I can add these tags after the '---' line. Nobody ever told 
me that I was doing it wrong, and I see Cc: tags quite often in commit 
messages, so I thought it was common practice.

>>  ---
>>   drivers/mfd/sec-core.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>>  diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
>>  index 1fb29c45f5cf..a467de2b2fea 100644
>>  --- a/drivers/mfd/sec-core.c
>>  +++ b/drivers/mfd/sec-core.c
>>  @@ -455,7 +455,6 @@ static void sec_pmic_shutdown(struct i2c_client 
>> *i2c)
>>   	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
>>   }
>> 
>>  -#ifdef CONFIG_PM_SLEEP
>>   static int sec_pmic_suspend(struct device *dev)
> 
> Did you test W=1 with !CONFIG_PM_SLEEP? No warnings?

I tested the PR with !CONFIG_PM_SLEEP, correct. sec-core.o compiles 
fine. No warnings with W=1.
> 
Cheers,
-Paul

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Krzysztof Kozlowski]

On 08/08/2022 12:28, Paul Cercueil wrote:
> Hi Krzysztof,
> 
> Le lun., août 8 2022 at 12:11:02 +0300, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> a écrit :
>> On 07/08/2022 17:52, Paul Cercueil wrote:
>>>  Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
>>>  to handle the .suspend/.resume callbacks.
>>>
>>>  These macros allow the suspend and resume functions to be 
>>> automatically
>>>  dropped by the compiler when CONFIG_SUSPEND is disabled, without 
>>> having
>>>  to use #ifdef guards.
>>>
>>>  The advantage is then that these functions are now always compiled
>>>  independently of any Kconfig option, and thanks to that bugs and
>>>  regressions are easier to catch.
>>>
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>  Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>
>> The address does not work. Please don't add it to commit log.
> 
> That's what get-maintainers gave me, and I didn't get any error sending 
> at that address. 

I know, I was bugging Bartlomiej and other Samsung folks to fix it and
we reached some kind of conclusion but it never resulted in a patch.

> But I'll take your word.
> 
>>>  Cc: linux-samsung-soc@vger.kernel.org
>>
>> This is also not really needed in commit log... it's just a mailing 
>> list...
>>
>> I actually never understood why people want to add to commit log, so 
>> to
>> something which will last 10 years, Cc-ing other folks, instead of
>> adding such tags after '---'. Imagine 10 years from now:
>>
>> 1. What's the point to be cced on this patch after 10 years instead of
>> using maintainers file (the one in 10 years)? Why Cc-ing me in 10 
>> years?
>> If I am a maintainer of this driver in that time, I will be C-ced 
>> based
>> on maintainers file. If I am not a maintainer in 10 years, why the 
>> heck
>> cc-ing me based on some 10-year old commit? Just because I was a
>> maintainer once, like 10 years ago?
>>
>> 2. Or why cc-ing such people when backporting to stable?
>>
>> It's quite a lot of unnecessary emails which many of us won't actually
>> handle later...
>>
>> I sincerely admit I was once also adding such Cc-tags. But that time 
>> my
>> employer was counting lines-of-patch (including commit log)... crazy, 
>> right?
> 
> Yeah, well, I can add these tags after the '---' line. Nobody ever told 
> me that I was doing it wrong, and I see Cc: tags quite often in commit 
> messages, so I thought it was common practice.

It indeed is a practice, which I do not understand. :) My complaining
about it was just complaining, not as a feedback required to change.

> 
>>>  ---
>>>   drivers/mfd/sec-core.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>>  diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
>>>  index 1fb29c45f5cf..a467de2b2fea 100644
>>>  --- a/drivers/mfd/sec-core.c
>>>  +++ b/drivers/mfd/sec-core.c
>>>  @@ -455,7 +455,6 @@ static void sec_pmic_shutdown(struct i2c_client 
>>> *i2c)
>>>   	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
>>>   }
>>>
>>>  -#ifdef CONFIG_PM_SLEEP
>>>   static int sec_pmic_suspend(struct device *dev)
>>
>> Did you test W=1 with !CONFIG_PM_SLEEP? No warnings?
> 
> I tested the PR with !CONFIG_PM_SLEEP, correct. sec-core.o compiles 
> fine. No warnings with W=1.

Ah, I see now. _DEFINE_DEV_PM_OPS uses __maybe_unused for such case.

Looks fine then. With dropped Bartlomiej Cc:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Paul Cercueil]

[...]

>>>>   ---
>>>>    drivers/mfd/sec-core.c | 7 +++----
>>>>    1 file changed, 3 insertions(+), 4 deletions(-)
>>>> 
>>>>   diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
>>>>   index 1fb29c45f5cf..a467de2b2fea 100644
>>>>   --- a/drivers/mfd/sec-core.c
>>>>   +++ b/drivers/mfd/sec-core.c
>>>>   @@ -455,7 +455,6 @@ static void sec_pmic_shutdown(struct 
>>>> i2c_client
>>>>  *i2c)
>>>>    	regmap_update_bits(sec_pmic->regmap_pmic, reg, mask, 0);
>>>>    }
>>>> 
>>>>   -#ifdef CONFIG_PM_SLEEP
>>>>    static int sec_pmic_suspend(struct device *dev)
>>> 
>>>  Did you test W=1 with !CONFIG_PM_SLEEP? No warnings?
>> 
>>  I tested the PR with !CONFIG_PM_SLEEP, correct. sec-core.o compiles
>>  fine. No warnings with W=1.
> 
> Ah, I see now. _DEFINE_DEV_PM_OPS uses __maybe_unused for such case.

Actually it doesn't :) Since the (dev_pm_ops) structure is always (and 
should always be) referenced through pm_sleep_ptr() or pm_ptr(), the 
symbols are never seen as unused by the compiler, but are automatically 
dropped by the compiler when the related config option is turned off.

Cheers,
-Paul

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Lee Jones]

On Mon, 08 Aug 2022, Krzysztof Kozlowski wrote:

> On 07/08/2022 17:52, Paul Cercueil wrote:
> > Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
> > to handle the .suspend/.resume callbacks.
> > 
> > These macros allow the suspend and resume functions to be automatically
> > dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> > to use #ifdef guards.
> > 
> > The advantage is then that these functions are now always compiled
> > independently of any Kconfig option, and thanks to that bugs and
> > regressions are easier to catch.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> 
> The address does not work. Please don't add it to commit log.
> 
> > Cc: linux-samsung-soc@vger.kernel.org
> 
> This is also not really needed in commit log... it's just a mailing list...
> 
> I actually never understood why people want to add to commit log, so to
> something which will last 10 years, Cc-ing other folks, instead of
> adding such tags after '---'. Imagine 10 years from now:
> 
> 1. What's the point to be cced on this patch after 10 years instead of
> using maintainers file (the one in 10 years)? Why Cc-ing me in 10 years?
> If I am a maintainer of this driver in that time, I will be C-ced based
> on maintainers file. If I am not a maintainer in 10 years, why the heck
> cc-ing me based on some 10-year old commit? Just because I was a
> maintainer once, like 10 years ago?

Why would that happen?

These tags are only used during initial submission.

> 2. Or why cc-ing such people when backporting to stable?

That doesn't happen either.

> It's quite a lot of unnecessary emails which many of us won't actually
> handle later...
> 
> I sincerely admit I was once also adding such Cc-tags. But that time my
> employer was counting lines-of-patch (including commit log)... crazy, right?

Nothing wrong with adding these tags IMHO.  It's what they're for.

I use them when I'm maintaining a large amount of out-of-tree, but
to-be-upstreamed patches over several versions.  Re-applying the
recipients list can become pretty labour-some after several
iterations.

Adding them under the '---' doesn't work when the purpose of them is
to keep the recipients list in Git history.

-- 
DEPRECATED: Please use lee@kernel.org

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Krzysztof Kozlowski]

On 09/08/2022 18:33, Lee Jones wrote:
> On Mon, 08 Aug 2022, Krzysztof Kozlowski wrote:
> 
>> On 07/08/2022 17:52, Paul Cercueil wrote:
>>> Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
>>> to handle the .suspend/.resume callbacks.
>>>
>>> These macros allow the suspend and resume functions to be automatically
>>> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
>>> to use #ifdef guards.
>>>
>>> The advantage is then that these functions are now always compiled
>>> independently of any Kconfig option, and thanks to that bugs and
>>> regressions are easier to catch.
>>>
>>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>
>> The address does not work. Please don't add it to commit log.
>>
>>> Cc: linux-samsung-soc@vger.kernel.org
>>
>> This is also not really needed in commit log... it's just a mailing list...
>>
>> I actually never understood why people want to add to commit log, so to
>> something which will last 10 years, Cc-ing other folks, instead of
>> adding such tags after '---'. Imagine 10 years from now:
>>
>> 1. What's the point to be cced on this patch after 10 years instead of
>> using maintainers file (the one in 10 years)? Why Cc-ing me in 10 years?
>> If I am a maintainer of this driver in that time, I will be C-ced based
>> on maintainers file. If I am not a maintainer in 10 years, why the heck
>> cc-ing me based on some 10-year old commit? Just because I was a
>> maintainer once, like 10 years ago?
> 
> Why would that happen?
> 
> These tags are only used during initial submission.

No, the tags are used in any other resends, backports etc while
traveling through different trees. I think only stable-backports do not
use them, but all other gfp+git-send will follow the tags.

> 
>> 2. Or why cc-ing such people when backporting to stable?
> 
> That doesn't happen either.

Indeed, stable does not use these Cc.

> 
>> It's quite a lot of unnecessary emails which many of us won't actually
>> handle later...
>>
>> I sincerely admit I was once also adding such Cc-tags. But that time my
>> employer was counting lines-of-patch (including commit log)... crazy, right?
> 
> Nothing wrong with adding these tags IMHO.  It's what they're for.
> 
> I use them when I'm maintaining a large amount of out-of-tree, but
> to-be-upstreamed patches over several versions.  Re-applying the
> recipients list can become pretty labour-some after several
> iterations.

You can do it still while keeping the tags after ---. Only patch-related
workflows strip such tags. If you cherry-pick, rebase, merge, you always
get the content of ---.

The same as typical changelog (not cover letter but one in the patch) -
you keep it after --- and it does not disappear.

> 
> Adding them under the '---' doesn't work when the purpose of them is
> to keep the recipients list in Git history.

This I understand, what I did not understand (and you did not explain)
is what would be the purpose to keep them in Git history. What is the
point to have them in commit log of 10 year old commit?

Best regards,
Krzysztof

Re: [PATCH 13/28] mfd: sec: Remove #ifdef guards for PM related functions

[Lee Jones]

On Tue, 09 Aug 2022, Krzysztof Kozlowski wrote:

> On 09/08/2022 18:33, Lee Jones wrote:
> > On Mon, 08 Aug 2022, Krzysztof Kozlowski wrote:
> > 
> >> On 07/08/2022 17:52, Paul Cercueil wrote:
> >>> Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros
> >>> to handle the .suspend/.resume callbacks.
> >>>
> >>> These macros allow the suspend and resume functions to be automatically
> >>> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> >>> to use #ifdef guards.
> >>>
> >>> The advantage is then that these functions are now always compiled
> >>> independently of any Kconfig option, and thanks to that bugs and
> >>> regressions are easier to catch.
> >>>
> >>> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>> Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >>
> >> The address does not work. Please don't add it to commit log.
> >>
> >>> Cc: linux-samsung-soc@vger.kernel.org
> >>
> >> This is also not really needed in commit log... it's just a mailing list...
> >>
> >> I actually never understood why people want to add to commit log, so to
> >> something which will last 10 years, Cc-ing other folks, instead of
> >> adding such tags after '---'. Imagine 10 years from now:
> >>
> >> 1. What's the point to be cced on this patch after 10 years instead of
> >> using maintainers file (the one in 10 years)? Why Cc-ing me in 10 years?
> >> If I am a maintainer of this driver in that time, I will be C-ced based
> >> on maintainers file. If I am not a maintainer in 10 years, why the heck
> >> cc-ing me based on some 10-year old commit? Just because I was a
> >> maintainer once, like 10 years ago?
> > 
> > Why would that happen?
> > 
> > These tags are only used during initial submission.
> 
> No, the tags are used in any other resends, backports etc while
> traveling through different trees. I think only stable-backports do not
> use them, but all other gfp+git-send will follow the tags.
> 
> > 
> >> 2. Or why cc-ing such people when backporting to stable?
> > 
> > That doesn't happen either.
> 
> Indeed, stable does not use these Cc.
> 
> >> It's quite a lot of unnecessary emails which many of us won't actually
> >> handle later...
> >>
> >> I sincerely admit I was once also adding such Cc-tags. But that time my
> >> employer was counting lines-of-patch (including commit log)... crazy, right?
> > 
> > Nothing wrong with adding these tags IMHO.  It's what they're for.
> > 
> > I use them when I'm maintaining a large amount of out-of-tree, but
> > to-be-upstreamed patches over several versions.  Re-applying the
> > recipients list can become pretty labour-some after several
> > iterations.
> 
> You can do it still while keeping the tags after ---. Only patch-related
> workflows strip such tags. If you cherry-pick, rebase, merge, you always
> get the content of ---.
> 
> The same as typical changelog (not cover letter but one in the patch) -
> you keep it after --- and it does not disappear.

I'll have to try this next time.

> > Adding them under the '---' doesn't work when the purpose of them is
> > to keep the recipients list in Git history.
> 
> This I understand, what I did not understand (and you did not explain)
> is what would be the purpose to keep them in Git history. What is the
> point to have them in commit log of 10 year old commit?

Here is a documented use for the tags:

 "If a person has had the opportunity to comment on a patch, but has not
  provided such comments, you may optionally add a ``Cc:`` tag to the patch."

Thus, when a recipient replies with a *-by tag, I strip out the
corresponding Cc: line.

Obviously this does not apply to mailing lists.

-- 
DEPRECATED: Please use lee@kernel.org