| Message ID | 20220807145247.46107-14-paul@crapouillou.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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
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
[...] >>>> --- >>>> 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
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.
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
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.
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,
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(-)