media: i2c: imx290: fix conditional function defintions

[PATCH] media: i2c: imx290: fix conditional function defintions

Posted by Arnd Bergmann 2 years, 12 months ago

From: Arnd Bergmann <arnd@arndb.de>

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 <arnd@arndb.de>
---
 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

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

Posted by Manivannan Sadhasivam 2 years, 10 months ago

On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>

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
> 

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

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

Posted by Laurent Pinchart 2 years, 12 months ago

Hi Arnd,

Thank you for the patch.

On Tue, Feb 07, 2023 at 05:13:12PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <arnd@arndb.de>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  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

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

Posted by Linux regression tracking (Thorsten Leemhuis) 2 years, 10 months ago

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 <arnd@arndb.de>
>>
>> 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 <arnd@arndb.de>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> ---
>>  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,
>>  	},
>>  };
>>  
>

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

Posted by Sakari Ailus 2 years, 10 months ago

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 <arnd@arndb.de>
> >>
> >> 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

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

Posted by Linux regression tracking (Thorsten Leemhuis) 2 years, 10 months ago

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 <arnd@arndb.de>
>>>>
>>>> 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

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

Posted by Thorsten Leemhuis 2 years, 10 months ago


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 <arnd@arndb.de>
>>>>>
>>>>> 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

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

Posted by Sakari Ailus 2 years, 10 months ago

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