2023-02-07 16:13:34

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] media: i2c: imx290: fix conditional function defintions

From: Arnd Bergmann <[email protected]>

The runtime suspend/resume functions are only referenced from the
dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
that requires a __maybe_unused annotation to avoid a warning:

drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
static int imx290_runtime_resume(struct device *dev)
^
drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
static int imx290_runtime_suspend(struct device *dev)
^

Convert this to the new RUNTIME_PM_OPS() helper that so this
is not required. To improve this further, also use the pm_ptr()
helper that lets the dev_pm_ops get dropped entirely when
CONFIG_PM is disabled.

A related mistake happened in the of_match_ptr() macro here, which
like SET_RUNTIME_PM_OPS() requires the match table to be marked
as __maybe_unused, though I could not reproduce building this without
CONFIG_OF. Remove the of_match_ptr() here as there is no point in
dropping the match table in configurations without CONFIG_OF.

Fixes: 02852c01f654 ("media: i2c: imx290: Initialize runtime PM before subdev")
Signed-off-by: Arnd Bergmann <[email protected]>
---
drivers/media/i2c/imx290.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 49d6c8bdec41..48ae2e0adf9e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -1098,7 +1098,7 @@ static int imx290_runtime_suspend(struct device *dev)
}

static const struct dev_pm_ops imx290_pm_ops = {
- SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
+ RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
};

/* ----------------------------------------------------------------------------
@@ -1362,8 +1362,8 @@ static struct i2c_driver imx290_i2c_driver = {
.remove = imx290_remove,
.driver = {
.name = "imx290",
- .pm = &imx290_pm_ops,
- .of_match_table = of_match_ptr(imx290_of_match),
+ .pm = pm_ptr(&imx290_pm_ops),
+ .of_match_table = imx290_of_match,
},
};

--
2.39.1



2023-02-07 16:40:22

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

Hi Arnd,

Thank you for the patch.

On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> The runtime suspend/resume functions are only referenced from the
> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
> that requires a __maybe_unused annotation to avoid a warning:
>
> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
> static int imx290_runtime_resume(struct device *dev)
> ^
> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
> static int imx290_runtime_suspend(struct device *dev)
> ^
>
> Convert this to the new RUNTIME_PM_OPS() helper that so this
> is not required. To improve this further, also use the pm_ptr()
> helper that lets the dev_pm_ops get dropped entirely when
> CONFIG_PM is disabled.
>
> A related mistake happened in the of_match_ptr() macro here, which
> like SET_RUNTIME_PM_OPS() requires the match table to be marked
> as __maybe_unused, though I could not reproduce building this without
> CONFIG_OF. Remove the of_match_ptr() here as there is no point in
> dropping the match table in configurations without CONFIG_OF.
>
> Fixes: 02852c01f654 ("media: i2c: imx290: Initialize runtime PM before subdev")
> Signed-off-by: Arnd Bergmann <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> drivers/media/i2c/imx290.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 49d6c8bdec41..48ae2e0adf9e 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1098,7 +1098,7 @@ static int imx290_runtime_suspend(struct device *dev)
> }
>
> static const struct dev_pm_ops imx290_pm_ops = {
> - SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
> + RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
> };
>
> /* ----------------------------------------------------------------------------
> @@ -1362,8 +1362,8 @@ static struct i2c_driver imx290_i2c_driver = {
> .remove = imx290_remove,
> .driver = {
> .name = "imx290",
> - .pm = &imx290_pm_ops,
> - .of_match_table = of_match_ptr(imx290_of_match),
> + .pm = pm_ptr(&imx290_pm_ops),
> + .of_match_table = imx290_of_match,
> },
> };
>

--
Regards,

Laurent Pinchart

2023-03-20 09:18:34

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

On 07.02.23 17:40, Laurent Pinchart wrote:
> On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
>> From: Arnd Bergmann <[email protected]>
>>
>> The runtime suspend/resume functions are only referenced from the
>> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
>> that requires a __maybe_unused annotation to avoid a warning:
>>
>> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
>> static int imx290_runtime_resume(struct device *dev)
>> ^
>> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
>> static int imx290_runtime_suspend(struct device *dev)
>> ^
>>

I might be missing something (if so, please tell me), but to me it looks
this fix for a build issue in 6.3-rc (which shows up in Guenters weekly
reports to Linus) didn't make any progress in the past few weeks. Is
there a reason why? Who actually needs to pick it up and send it towards
mainline? Manivannan Sadhasivam? Sakari Ailus?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

>> Convert this to the new RUNTIME_PM_OPS() helper that so this
>> is not required. To improve this further, also use the pm_ptr()
>> helper that lets the dev_pm_ops get dropped entirely when
>> CONFIG_PM is disabled.
>>
>> A related mistake happened in the of_match_ptr() macro here, which
>> like SET_RUNTIME_PM_OPS() requires the match table to be marked
>> as __maybe_unused, though I could not reproduce building this without
>> CONFIG_OF. Remove the of_match_ptr() here as there is no point in
>> dropping the match table in configurations without CONFIG_OF.
>>
>> Fixes: 02852c01f654 ("media: i2c: imx290: Initialize runtime PM before subdev")
>> Signed-off-by: Arnd Bergmann <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
>> ---
>> drivers/media/i2c/imx290.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
>> index 49d6c8bdec41..48ae2e0adf9e 100644
>> --- a/drivers/media/i2c/imx290.c
>> +++ b/drivers/media/i2c/imx290.c
>> @@ -1098,7 +1098,7 @@ static int imx290_runtime_suspend(struct device *dev)
>> }
>>
>> static const struct dev_pm_ops imx290_pm_ops = {
>> - SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
>> + RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
>> };
>>
>> /* ----------------------------------------------------------------------------
>> @@ -1362,8 +1362,8 @@ static struct i2c_driver imx290_i2c_driver = {
>> .remove = imx290_remove,
>> .driver = {
>> .name = "imx290",
>> - .pm = &imx290_pm_ops,
>> - .of_match_table = of_match_ptr(imx290_of_match),
>> + .pm = pm_ptr(&imx290_pm_ops),
>> + .of_match_table = imx290_of_match,
>> },
>> };
>>
>

2023-03-20 09:23:49

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> The runtime suspend/resume functions are only referenced from the
> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
> that requires a __maybe_unused annotation to avoid a warning:
>
> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
> static int imx290_runtime_resume(struct device *dev)
> ^
> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
> static int imx290_runtime_suspend(struct device *dev)
> ^
>
> Convert this to the new RUNTIME_PM_OPS() helper that so this
> is not required. To improve this further, also use the pm_ptr()
> helper that lets the dev_pm_ops get dropped entirely when
> CONFIG_PM is disabled.
>
> A related mistake happened in the of_match_ptr() macro here, which
> like SET_RUNTIME_PM_OPS() requires the match table to be marked
> as __maybe_unused, though I could not reproduce building this without
> CONFIG_OF. Remove the of_match_ptr() here as there is no point in
> dropping the match table in configurations without CONFIG_OF.
>
> Fixes: 02852c01f654 ("media: i2c: imx290: Initialize runtime PM before subdev")
> Signed-off-by: Arnd Bergmann <[email protected]>

Reviewed-by: Manivannan Sadhasivam <[email protected]>

Thanks,
Mani

> ---
> drivers/media/i2c/imx290.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 49d6c8bdec41..48ae2e0adf9e 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -1098,7 +1098,7 @@ static int imx290_runtime_suspend(struct device *dev)
> }
>
> static const struct dev_pm_ops imx290_pm_ops = {
> - SET_RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
> + RUNTIME_PM_OPS(imx290_runtime_suspend, imx290_runtime_resume, NULL)
> };
>
> /* ----------------------------------------------------------------------------
> @@ -1362,8 +1362,8 @@ static struct i2c_driver imx290_i2c_driver = {
> .remove = imx290_remove,
> .driver = {
> .name = "imx290",
> - .pm = &imx290_pm_ops,
> - .of_match_table = of_match_ptr(imx290_of_match),
> + .pm = pm_ptr(&imx290_pm_ops),
> + .of_match_table = imx290_of_match,
> },
> };
>
> --
> 2.39.1
>

--
மணிவண்ணன் சதாசிவம்

2023-03-20 09:32:26

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

Hello,

On Mon, Mar 20, 2023 at 10:18:23AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 07.02.23 17:40, Laurent Pinchart wrote:
> > On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <[email protected]>
> >>
> >> The runtime suspend/resume functions are only referenced from the
> >> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
> >> that requires a __maybe_unused annotation to avoid a warning:
> >>
> >> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
> >> static int imx290_runtime_resume(struct device *dev)
> >> ^
> >> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
> >> static int imx290_runtime_suspend(struct device *dev)
> >> ^
> >>
>
> I might be missing something (if so, please tell me), but to me it looks
> this fix for a build issue in 6.3-rc (which shows up in Guenters weekly
> reports to Linus) didn't make any progress in the past few weeks. Is
> there a reason why? Who actually needs to pick it up and send it towards
> mainline? Manivannan Sadhasivam? Sakari Ailus?

Mauro has recently pulled my PR including this and I understand it's in
Mauro's tree now.

--
Kind regards,

Sakari Ailus

2023-03-20 09:42:36

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

On 20.03.23 10:32, Sakari Ailus wrote:
> On Mon, Mar 20, 2023 at 10:18:23AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 07.02.23 17:40, Laurent Pinchart wrote:
>>> On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
>>>> From: Arnd Bergmann <[email protected]>
>>>>
>>>> The runtime suspend/resume functions are only referenced from the
>>>> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
>>>> that requires a __maybe_unused annotation to avoid a warning:
>>>>
>>>> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
>>>> static int imx290_runtime_resume(struct device *dev)
>>>> ^
>>>> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
>>>> static int imx290_runtime_suspend(struct device *dev)
>>>> ^
>>>>
>>
>> I might be missing something (if so, please tell me), but to me it looks
>> this fix for a build issue in 6.3-rc (which shows up in Guenters weekly
>> reports to Linus) didn't make any progress in the past few weeks. Is
>> there a reason why? Who actually needs to pick it up and send it towards
>> mainline? Manivannan Sadhasivam? Sakari Ailus?
>
> Mauro has recently pulled my PR including this and I understand it's in
> Mauro's tree now.

Ahh, great, many thx.

I checked Linux-next before writing my mail, but didn't sport it there. :-/

Ciao, Thorsten

2023-03-20 09:47:00

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions



On 20.03.23 10:42, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 20.03.23 10:32, Sakari Ailus wrote:
>> On Mon, Mar 20, 2023 at 10:18:23AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>>> On 07.02.23 17:40, Laurent Pinchart wrote:
>>>> On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
>>>>> From: Arnd Bergmann <[email protected]>
>>>>>
>>>>> The runtime suspend/resume functions are only referenced from the
>>>>> dev_pm_ops, but they use the old SET_RUNTIME_PM_OPS() helper
>>>>> that requires a __maybe_unused annotation to avoid a warning:
>>>>>
>>>>> drivers/media/i2c/imx290.c:1082:12: error: unused function 'imx290_runtime_resume' [-Werror,-Wunused-function]
>>>>> static int imx290_runtime_resume(struct device *dev)
>>>>> ^
>>>>> drivers/media/i2c/imx290.c:1090:12: error: unused function 'imx290_runtime_suspend' [-Werror,-Wunused-function]
>>>>> static int imx290_runtime_suspend(struct device *dev)
>>>>> ^
>>>>>
>>>
>>> I might be missing something (if so, please tell me), but to me it looks
>>> this fix for a build issue in 6.3-rc (which shows up in Guenters weekly
>>> reports to Linus) didn't make any progress in the past few weeks. Is
>>> there a reason why? Who actually needs to pick it up and send it towards
>>> mainline? Manivannan Sadhasivam? Sakari Ailus?
>>
>> Mauro has recently pulled my PR including this and I understand it's in
>> Mauro's tree now.
>
> Ahh, great, many thx.
>
> I checked Linux-next before writing my mail, but didn't sport it there. :-/

Argh, ohh no, it's there. Sorry for the noise. Second time something
like this happens today. Seems today is not my best day. :-/

Ciao, Thorsten

2023-03-20 09:51:34

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH] media: i2c: imx290: fix conditional function defintions

On Mon, Mar 20, 2023 at 10:46:45AM +0100, Thorsten Leemhuis wrote:
> Argh, ohh no, it's there. Sorry for the noise. Second time something
> like this happens today. Seems today is not my best day. :-/

No worries!

--
Sakari Ailus