[PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Elgin Perumbilly]

Replace runtime_suspend/resume wrappers by using power helpers
directly with DEFINE_RUNTIME_DEV_PM_OPS().

Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
---
 drivers/media/i2c/imx415.c | 44 +++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
index 0b424c17e880..b7d44f3e165b 100644
--- a/drivers/media/i2c/imx415.c
+++ b/drivers/media/i2c/imx415.c
@@ -1129,8 +1129,12 @@ static void imx415_subdev_cleanup(struct imx415 *sensor)
 	v4l2_ctrl_handler_free(&sensor->ctrls);
 }

-static int imx415_power_on(struct imx415 *sensor)
+static int imx415_power_on(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct imx415 *sensor = to_imx415(subdev);
+
 	int ret;

 	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
@@ -1161,11 +1165,17 @@ static int imx415_power_on(struct imx415 *sensor)
 	return ret;
 }

-static void imx415_power_off(struct imx415 *sensor)
+static int imx415_power_off(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+	struct imx415 *sensor = to_imx415(subdev);
+
 	clk_disable_unprepare(sensor->clk);
 	gpiod_set_value_cansleep(sensor->reset, 1);
 	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
+
+	return 0;
 }

 static int imx415_identify_model(struct imx415 *sensor)
@@ -1371,7 +1381,7 @@ static int imx415_probe(struct i2c_client *client)
 	 * work when runtime PM is disabled in the kernel. To that end, power
 	 * the sensor on manually here, identify it, and fully initialize it.
 	 */
-	ret = imx415_power_on(sensor);
+	ret = imx415_power_on(sensor->dev);
 	if (ret)
 		return ret;

@@ -1411,7 +1421,7 @@ static int imx415_probe(struct i2c_client *client)
 	pm_runtime_put_noidle(sensor->dev);
 	imx415_subdev_cleanup(sensor);
 err_power:
-	imx415_power_off(sensor);
+	imx415_power_off(sensor->dev);
 	return ret;
 }

@@ -1430,32 +1440,12 @@ static void imx415_remove(struct i2c_client *client)
 	 */
 	pm_runtime_disable(sensor->dev);
 	if (!pm_runtime_status_suspended(sensor->dev))
-		imx415_power_off(sensor);
+		imx415_power_off(sensor->dev);
 	pm_runtime_set_suspended(sensor->dev);
 }

-static int imx415_runtime_resume(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
-	struct imx415 *sensor = to_imx415(subdev);
-
-	return imx415_power_on(sensor);
-}
-
-static int imx415_runtime_suspend(struct device *dev)
-{
-	struct i2c_client *client = to_i2c_client(dev);
-	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
-	struct imx415 *sensor = to_imx415(subdev);
-
-	imx415_power_off(sensor);
-
-	return 0;
-}
-
-static DEFINE_RUNTIME_DEV_PM_OPS(imx415_pm_ops, imx415_runtime_suspend,
-				 imx415_runtime_resume, NULL);
+static DEFINE_RUNTIME_DEV_PM_OPS(imx415_pm_ops, imx415_power_off,
+				 imx415_power_on, NULL);

 static const struct of_device_id imx415_of_match[] = {
 	{ .compatible = "sony,imx415" },
--
2.34.1

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Michael Riesch]

Hi Elgin,

Thanks for the patch but...

On 3/24/26 13:45, Elgin Perumbilly wrote:
> Replace runtime_suspend/resume wrappers by using power helpers
> directly with DEFINE_RUNTIME_DEV_PM_OPS().

...why? What advantage does this refactoring bring?

Best regards,
Michael

> 
> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@siliconsignals.io>
> ---
>  drivers/media/i2c/imx415.c | 44 +++++++++++++++-----------------------
>  1 file changed, 17 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx415.c b/drivers/media/i2c/imx415.c
> index 0b424c17e880..b7d44f3e165b 100644
> --- a/drivers/media/i2c/imx415.c
> +++ b/drivers/media/i2c/imx415.c
> @@ -1129,8 +1129,12 @@ static void imx415_subdev_cleanup(struct imx415 *sensor)
>  	v4l2_ctrl_handler_free(&sensor->ctrls);
>  }
> 
> -static int imx415_power_on(struct imx415 *sensor)
> +static int imx415_power_on(struct device *dev)
>  {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> +	struct imx415 *sensor = to_imx415(subdev);
> +
>  	int ret;
> 
>  	ret = regulator_bulk_enable(ARRAY_SIZE(sensor->supplies),
> @@ -1161,11 +1165,17 @@ static int imx415_power_on(struct imx415 *sensor)
>  	return ret;
>  }
> 
> -static void imx415_power_off(struct imx415 *sensor)
> +static int imx415_power_off(struct device *dev)
>  {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> +	struct imx415 *sensor = to_imx415(subdev);
> +
>  	clk_disable_unprepare(sensor->clk);
>  	gpiod_set_value_cansleep(sensor->reset, 1);
>  	regulator_bulk_disable(ARRAY_SIZE(sensor->supplies), sensor->supplies);
> +
> +	return 0;
>  }
> 
>  static int imx415_identify_model(struct imx415 *sensor)
> @@ -1371,7 +1381,7 @@ static int imx415_probe(struct i2c_client *client)
>  	 * work when runtime PM is disabled in the kernel. To that end, power
>  	 * the sensor on manually here, identify it, and fully initialize it.
>  	 */
> -	ret = imx415_power_on(sensor);
> +	ret = imx415_power_on(sensor->dev);
>  	if (ret)
>  		return ret;
> 
> @@ -1411,7 +1421,7 @@ static int imx415_probe(struct i2c_client *client)
>  	pm_runtime_put_noidle(sensor->dev);
>  	imx415_subdev_cleanup(sensor);
>  err_power:
> -	imx415_power_off(sensor);
> +	imx415_power_off(sensor->dev);
>  	return ret;
>  }
> 
> @@ -1430,32 +1440,12 @@ static void imx415_remove(struct i2c_client *client)
>  	 */
>  	pm_runtime_disable(sensor->dev);
>  	if (!pm_runtime_status_suspended(sensor->dev))
> -		imx415_power_off(sensor);
> +		imx415_power_off(sensor->dev);
>  	pm_runtime_set_suspended(sensor->dev);
>  }
> 
> -static int imx415_runtime_resume(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> -	struct imx415 *sensor = to_imx415(subdev);
> -
> -	return imx415_power_on(sensor);
> -}
> -
> -static int imx415_runtime_suspend(struct device *dev)
> -{
> -	struct i2c_client *client = to_i2c_client(dev);
> -	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> -	struct imx415 *sensor = to_imx415(subdev);
> -
> -	imx415_power_off(sensor);
> -
> -	return 0;
> -}
> -
> -static DEFINE_RUNTIME_DEV_PM_OPS(imx415_pm_ops, imx415_runtime_suspend,
> -				 imx415_runtime_resume, NULL);
> +static DEFINE_RUNTIME_DEV_PM_OPS(imx415_pm_ops, imx415_power_off,
> +				 imx415_power_on, NULL);
> 
>  static const struct of_device_id imx415_of_match[] = {
>  	{ .compatible = "sony,imx415" },
> --
> 2.34.1
> 

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Sakari Ailus]

Hi Michael,

On Tue, Mar 24, 2026 at 08:42:31PM +0100, Michael Riesch wrote:
> Hi Elgin,
> 
> Thanks for the patch but...
> 
> On 3/24/26 13:45, Elgin Perumbilly wrote:
> > Replace runtime_suspend/resume wrappers by using power helpers
> > directly with DEFINE_RUNTIME_DEV_PM_OPS().
> 
> ...why? What advantage does this refactoring bring?

It looks like patch removes two redundant functions, doesn't it? :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Michael Riesch]

Hi Elgin, Sakari,

On 3/25/26 13:28, Sakari Ailus wrote:
> Hi Michael,
> 
> On Tue, Mar 24, 2026 at 08:42:31PM +0100, Michael Riesch wrote:
>> Hi Elgin,
>>
>> Thanks for the patch but...
>>
>> On 3/24/26 13:45, Elgin Perumbilly wrote:
>>> Replace runtime_suspend/resume wrappers by using power helpers
>>> directly with DEFINE_RUNTIME_DEV_PM_OPS().
>>
>> ...why? What advantage does this refactoring bring?
> 
> It looks like patch removes two redundant functions, doesn't it? :-)

Well yes. 10 lines saved. Not sure whether this was the biggest issue
with this driver -- surely it was not a big issue for me.

Elgin, I was wondering whether this change is within a certain context
or whether there is something I fail to recognize.

Anyway, if you absolutely want this change: I feel it would be nicer if
you left the imx415_runtime_{resume,suspend} and moved the bit of code
from imx415_power_{on,off} to them. Reasons for that may be mostly
aesthetical -- I claim better readability that way.

Best regards,
Michael

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Elgin Perumbilly]

Hi Michael, Sakari,                                                             
Sorry for the late reply, and thanks for the feedback.                          
                                                                                
>Hi Elgin, Sakari,                                                              
>                                                                               
>On 3/25/26 13:28, Sakari Ailus wrote:                                          
>> Hi Michael,                                                                  
>>                                                                              
>> On Tue, Mar 24, 2026 at 08:42:31PM +0100, Michael Riesch wrote:              
>>> Hi Elgin,                                                                   
>>>                                                                             
>>> Thanks for the patch but...                                                 
>>>                                                                             
>>> On 3/24/26 13:45, Elgin Perumbilly wrote:                                   
>>>> Replace runtime_suspend/resume wrappers by using power helpers             
>>>> directly with DEFINE_RUNTIME_DEV_PM_OPS().                                 
>>>                                                                             
>>> ...why? What advantage does this refactoring bring?                         
>>                                                                              
>> It looks like patch removes two redundant functions, doesn't it? :-)         
>                                                                               
>Well yes. 10 lines saved. Not sure whether this was the biggest issue          
>with this driver -- surely it was not a big issue for me.                      
>                                                                               
>Elgin, I was wondering whether this change is within a certain context         
>or whether there is something I fail to recognize.                             
>                                                                               
>Anyway, if you absolutely want this change: I feel it would be nicer if        
>you left the imx415_runtime_{resume,suspend} and moved the bit of code         
>from imx415_power_{on,off} to them. Reasons for that may be mostly             
>aesthetical -- I claim better readability that way.                            
                                                                                
The intent of this change is to remove redundant runtime PM                     
wrappers and simplify the code, without altering behavior.                      
                                                                                
If you’re okay with this approach, we can proceed as is. If you                 
feel it’s not necessary at this point, that’s fine as well.                     
                                                                                
Best regards,                                                                   
Elgin 

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Sakari Ailus]

Hi Elgin,

On Sat, Mar 28, 2026 at 06:11:43AM +0000, Elgin Perumbilly wrote:
> Hi Michael, Sakari,                                                             
> Sorry for the late reply, and thanks for the feedback.                          
>                                                                                 
> >Hi Elgin, Sakari,                                                              
> >                                                                               
> >On 3/25/26 13:28, Sakari Ailus wrote:                                          
> >> Hi Michael,                                                                  
> >>                                                                              
> >> On Tue, Mar 24, 2026 at 08:42:31PM +0100, Michael Riesch wrote:              
> >>> Hi Elgin,                                                                   
> >>>                                                                             
> >>> Thanks for the patch but...                                                 
> >>>                                                                             
> >>> On 3/24/26 13:45, Elgin Perumbilly wrote:                                   
> >>>> Replace runtime_suspend/resume wrappers by using power helpers             
> >>>> directly with DEFINE_RUNTIME_DEV_PM_OPS().                                 
> >>>                                                                             
> >>> ...why? What advantage does this refactoring bring?                         
> >>                                                                              
> >> It looks like patch removes two redundant functions, doesn't it? :-)         
> >                                                                               
> >Well yes. 10 lines saved. Not sure whether this was the biggest issue          
> >with this driver -- surely it was not a big issue for me.                      
> >                                                                               
> >Elgin, I was wondering whether this change is within a certain context         
> >or whether there is something I fail to recognize.                             
> >                                                                               
> >Anyway, if you absolutely want this change: I feel it would be nicer if        
> >you left the imx415_runtime_{resume,suspend} and moved the bit of code         
> >from imx415_power_{on,off} to them. Reasons for that may be mostly             
> >aesthetical -- I claim better readability that way.                            
>                                                                                 
> The intent of this change is to remove redundant runtime PM                     
> wrappers and simplify the code, without altering behavior.                      
>                                                                                 
> If you’re okay with this approach, we can proceed as is. If you                 
> feel it’s not necessary at this point, that’s fine as well.                     

Can you address Michael's comments?

Code tends to be copied to other drivers so cleaning up existing drivers is
worthwhile.

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: i2c: imx415: Drop redundant runtime PM callbacks

[Elgin Perumbilly]

Hi Sakari,

> Hi Elgin,
>
> On Sat, Mar 28, 2026 at 06:11:43AM +0000, Elgin Perumbilly wrote:
> >Hi Michael, Sakari,
> >Sorry for the late reply, and thanks for the feedback.
> >
> >>Hi Elgin, Sakari,
> >>
> >>On 3/25/26 13:28, Sakari Ailus wrote:
> >>> Hi Michael,
> >>>
> >>> On Tue, Mar 24, 2026 at 08:42:31PM +0100, Michael Riesch wrote:
> >>>> Hi Elgin,
> >>>>
> >>>> Thanks for the patch but...
> >>>>
> >>>> On 3/24/26 13:45, Elgin Perumbilly wrote:
> >>>>> Replace runtime_suspend/resume wrappers by using power helpers
> >>>>> directly with DEFINE_RUNTIME_DEV_PM_OPS().
> >>>>
> >>>> ...why? What advantage does this refactoring bring?
> >>>
> >>> It looks like patch removes two redundant functions, doesn't it? :-)
> >>
> >>Well yes. 10 lines saved. Not sure whether this was the biggest issue
> >>with this driver -- surely it was not a big issue for me.
> >>
> >>Elgin, I was wondering whether this change is within a certain context
> >>or whether there is something I fail to recognize.
> >>
> >>Anyway, if you absolutely want this change: I feel it would be nicer if
> >>you left the imx415_runtime_{resume,suspend} and moved the bit of code
> >>from imx415_power_{on,off} to them. Reasons for that may be mostly
> >>aesthetical -- I claim better readability that way.
> >
> >The intent of this change is to remove redundant runtime PM
> >wrappers and simplify the code, without altering behavior.
> >
> >If you’re okay with this approach, we can proceed as is. If you
> >feel it’s not necessary at this point, that’s fine as well.
>
> Can you address Michael's comments?
>
> Code tends to be copied to other drivers so cleaning up existing drivers is
> worthwhile.

As far as I understand, would it make sense to follow an approach
similar to t4ka3.c?

i.e. keep runtime_suspend() / runtime_resume() and move the power
on/off logic there. This would address Michael’s readability concern
while still achieving the cleanup.

Regards,
Elgin