LinuxTV Patchwork [v3] media: video-i2c: check if chip struct has set_power function

login
register
mail settings
Submitter Matt Ranostay
Date Nov. 24, 2018, 10:03 p.m.
Message ID <20181124220323.13497-1-matt.ranostay@konsulko.com>
Download mbox | patch
Permalink /patch/53138/
State Accepted
Delegated to: Hans Verkuil
Headers show

Comments

Matt Ranostay - Nov. 24, 2018, 10:03 p.m.
Not all future supported video chips will always have power management
support, and so it is important to check before calling set_power() is
defined.

Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hans Verkuil <hansverk@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Akinobu Mita <akinobu.mita@gmail.com>
Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---

Changes from v2:
- split out from mlx90640 patch series
- added to Cc list

Changes from v1:
- none

 drivers/media/i2c/video-i2c.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)
Akinobu Mita - Nov. 25, 2018, 3:17 p.m.
2018?11?25?(?) 7:03 Matt Ranostay <matt.ranostay@konsulko.com>:
>
> Not all future supported video chips will always have power management
> support, and so it is important to check before calling set_power() is
> defined.
>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>

Looks good.

Reviewed-by: Akinobu Mita <akinobu.mita@gmail.com>
Sakari Ailus - Nov. 25, 2018, 4:55 p.m.
Hi Matt,

On Sat, Nov 24, 2018 at 02:03:23PM -0800, Matt Ranostay wrote:
> Not all future supported video chips will always have power management
> support, and so it is important to check before calling set_power() is
> defined.
> 
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hans Verkuil <hansverk@cisco.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
> 
> Changes from v2:
> - split out from mlx90640 patch series
> - added to Cc list
> 
> Changes from v1:
> - none
> 
>  drivers/media/i2c/video-i2c.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index b6ebb8d53e90..01dcf179f203 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -736,9 +736,11 @@ static int video_i2c_probe(struct i2c_client *client,
>  	video_set_drvdata(&data->vdev, data);
>  	i2c_set_clientdata(client, data);
>  
> -	ret = data->chip->set_power(data, true);
> -	if (ret)
> -		goto error_unregister_device;
> +	if (data->chip->set_power) {
> +		ret = data->chip->set_power(data, true);
> +		if (ret)
> +			goto error_unregister_device;
> +	}

How about adding a macro to call the op if it's set? It could be used to
call other ops when they're set, and ignore them when they're not. Just an
idea. See e.g. v4l2_subdev_call() in include/media/v4l2-subdev.h .

>  
>  	pm_runtime_get_noresume(&client->dev);
>  	pm_runtime_set_active(&client->dev);
> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
>  	pm_runtime_put_noidle(&client->dev);
> -	data->chip->set_power(data, false);
> +
> +	if (data->chip->set_power)
> +		data->chip->set_power(data, false);
>  
>  error_unregister_device:
>  	v4l2_device_unregister(v4l2_dev);
> @@ -791,7 +795,9 @@ static int video_i2c_remove(struct i2c_client *client)
>  	pm_runtime_disable(&client->dev);
>  	pm_runtime_set_suspended(&client->dev);
>  	pm_runtime_put_noidle(&client->dev);
> -	data->chip->set_power(data, false);
> +
> +	if (data->chip->set_power)
> +		data->chip->set_power(data, false);
>  
>  	video_unregister_device(&data->vdev);
>  
> @@ -804,6 +810,9 @@ static int video_i2c_pm_runtime_suspend(struct device *dev)
>  {
>  	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>  
> +	if (!data->chip->set_power)
> +		return 0;
> +
>  	return data->chip->set_power(data, false);
>  }
>  
> @@ -811,6 +820,9 @@ static int video_i2c_pm_runtime_resume(struct device *dev)
>  {
>  	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>  
> +	if (!data->chip->set_power)
> +		return 0;
> +
>  	return data->chip->set_power(data, true);
>  }
>  
> -- 
> 2.17.1
>
Hans Verkuil - Nov. 26, 2018, 1:50 p.m.
On 11/25/2018 05:55 PM, Sakari Ailus wrote:
> Hi Matt,
> 
> On Sat, Nov 24, 2018 at 02:03:23PM -0800, Matt Ranostay wrote:
>> Not all future supported video chips will always have power management
>> support, and so it is important to check before calling set_power() is
>> defined.
>>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Hans Verkuil <hansverk@cisco.com>
>> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
>> Cc: Akinobu Mita <akinobu.mita@gmail.com>
>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
>> ---
>>
>> Changes from v2:
>> - split out from mlx90640 patch series
>> - added to Cc list
>>
>> Changes from v1:
>> - none
>>
>>  drivers/media/i2c/video-i2c.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
>> index b6ebb8d53e90..01dcf179f203 100644
>> --- a/drivers/media/i2c/video-i2c.c
>> +++ b/drivers/media/i2c/video-i2c.c
>> @@ -736,9 +736,11 @@ static int video_i2c_probe(struct i2c_client *client,
>>  	video_set_drvdata(&data->vdev, data);
>>  	i2c_set_clientdata(client, data);
>>  
>> -	ret = data->chip->set_power(data, true);
>> -	if (ret)
>> -		goto error_unregister_device;
>> +	if (data->chip->set_power) {
>> +		ret = data->chip->set_power(data, true);
>> +		if (ret)
>> +			goto error_unregister_device;
>> +	}
> 
> How about adding a macro to call the op if it's set? It could be used to
> call other ops when they're set, and ignore them when they're not. Just an
> idea. See e.g. v4l2_subdev_call() in include/media/v4l2-subdev.h .

Matt, is this something you want to do? If so, then I'll wait for a v4.

Regards,

	Hans

> 
>>  
>>  	pm_runtime_get_noresume(&client->dev);
>>  	pm_runtime_set_active(&client->dev);
>> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
>>  	pm_runtime_disable(&client->dev);
>>  	pm_runtime_set_suspended(&client->dev);
>>  	pm_runtime_put_noidle(&client->dev);
>> -	data->chip->set_power(data, false);
>> +
>> +	if (data->chip->set_power)
>> +		data->chip->set_power(data, false);
>>  
>>  error_unregister_device:
>>  	v4l2_device_unregister(v4l2_dev);
>> @@ -791,7 +795,9 @@ static int video_i2c_remove(struct i2c_client *client)
>>  	pm_runtime_disable(&client->dev);
>>  	pm_runtime_set_suspended(&client->dev);
>>  	pm_runtime_put_noidle(&client->dev);
>> -	data->chip->set_power(data, false);
>> +
>> +	if (data->chip->set_power)
>> +		data->chip->set_power(data, false);
>>  
>>  	video_unregister_device(&data->vdev);
>>  
>> @@ -804,6 +810,9 @@ static int video_i2c_pm_runtime_suspend(struct device *dev)
>>  {
>>  	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>>  
>> +	if (!data->chip->set_power)
>> +		return 0;
>> +
>>  	return data->chip->set_power(data, false);
>>  }
>>  
>> @@ -811,6 +820,9 @@ static int video_i2c_pm_runtime_resume(struct device *dev)
>>  {
>>  	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
>>  
>> +	if (!data->chip->set_power)
>> +		return 0;
>> +
>>  	return data->chip->set_power(data, true);
>>  }
>>  
>> -- 
>> 2.17.1
>>
>
Matt Ranostay - Nov. 26, 2018, 3:01 p.m.
On Mon, Nov 26, 2018 at 2:50 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/25/2018 05:55 PM, Sakari Ailus wrote:
> > Hi Matt,
> >
> > On Sat, Nov 24, 2018 at 02:03:23PM -0800, Matt Ranostay wrote:
> >> Not all future supported video chips will always have power management
> >> support, and so it is important to check before calling set_power() is
> >> defined.
> >>
> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Cc: Hans Verkuil <hansverk@cisco.com>
> >> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> >> Cc: Akinobu Mita <akinobu.mita@gmail.com>
> >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> >> ---
> >>
> >> Changes from v2:
> >> - split out from mlx90640 patch series
> >> - added to Cc list
> >>
> >> Changes from v1:
> >> - none
> >>
> >>  drivers/media/i2c/video-i2c.c | 22 +++++++++++++++++-----
> >>  1 file changed, 17 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> >> index b6ebb8d53e90..01dcf179f203 100644
> >> --- a/drivers/media/i2c/video-i2c.c
> >> +++ b/drivers/media/i2c/video-i2c.c
> >> @@ -736,9 +736,11 @@ static int video_i2c_probe(struct i2c_client *client,
> >>      video_set_drvdata(&data->vdev, data);
> >>      i2c_set_clientdata(client, data);
> >>
> >> -    ret = data->chip->set_power(data, true);
> >> -    if (ret)
> >> -            goto error_unregister_device;
> >> +    if (data->chip->set_power) {
> >> +            ret = data->chip->set_power(data, true);
> >> +            if (ret)
> >> +                    goto error_unregister_device;
> >> +    }
> >
> > How about adding a macro to call the op if it's set? It could be used to
> > call other ops when they're set, and ignore them when they're not. Just an
> > idea. See e.g. v4l2_subdev_call() in include/media/v4l2-subdev.h .
>
> Matt, is this something you want to do? If so, then I'll wait for a v4.
>

Probably wouldn't get around to doing it for a bit (on travel for the
next few weeks). If you could review/accept v3 for now that would be
great.

- Matt

> Regards,
>
>         Hans
>
> >
> >>
> >>      pm_runtime_get_noresume(&client->dev);
> >>      pm_runtime_set_active(&client->dev);
> >> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
> >>      pm_runtime_disable(&client->dev);
> >>      pm_runtime_set_suspended(&client->dev);
> >>      pm_runtime_put_noidle(&client->dev);
> >> -    data->chip->set_power(data, false);
> >> +
> >> +    if (data->chip->set_power)
> >> +            data->chip->set_power(data, false);
> >>
> >>  error_unregister_device:
> >>      v4l2_device_unregister(v4l2_dev);
> >> @@ -791,7 +795,9 @@ static int video_i2c_remove(struct i2c_client *client)
> >>      pm_runtime_disable(&client->dev);
> >>      pm_runtime_set_suspended(&client->dev);
> >>      pm_runtime_put_noidle(&client->dev);
> >> -    data->chip->set_power(data, false);
> >> +
> >> +    if (data->chip->set_power)
> >> +            data->chip->set_power(data, false);
> >>
> >>      video_unregister_device(&data->vdev);
> >>
> >> @@ -804,6 +810,9 @@ static int video_i2c_pm_runtime_suspend(struct device *dev)
> >>  {
> >>      struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
> >>
> >> +    if (!data->chip->set_power)
> >> +            return 0;
> >> +
> >>      return data->chip->set_power(data, false);
> >>  }
> >>
> >> @@ -811,6 +820,9 @@ static int video_i2c_pm_runtime_resume(struct device *dev)
> >>  {
> >>      struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
> >>
> >> +    if (!data->chip->set_power)
> >> +            return 0;
> >> +
> >>      return data->chip->set_power(data, true);
> >>  }
> >>
> >> --
> >> 2.17.1
> >>
> >
>

Patch

diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
index b6ebb8d53e90..01dcf179f203 100644
--- a/drivers/media/i2c/video-i2c.c
+++ b/drivers/media/i2c/video-i2c.c
@@ -736,9 +736,11 @@  static int video_i2c_probe(struct i2c_client *client,
 	video_set_drvdata(&data->vdev, data);
 	i2c_set_clientdata(client, data);
 
-	ret = data->chip->set_power(data, true);
-	if (ret)
-		goto error_unregister_device;
+	if (data->chip->set_power) {
+		ret = data->chip->set_power(data, true);
+		if (ret)
+			goto error_unregister_device;
+	}
 
 	pm_runtime_get_noresume(&client->dev);
 	pm_runtime_set_active(&client->dev);
@@ -767,7 +769,9 @@  static int video_i2c_probe(struct i2c_client *client,
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
-	data->chip->set_power(data, false);
+
+	if (data->chip->set_power)
+		data->chip->set_power(data, false);
 
 error_unregister_device:
 	v4l2_device_unregister(v4l2_dev);
@@ -791,7 +795,9 @@  static int video_i2c_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_put_noidle(&client->dev);
-	data->chip->set_power(data, false);
+
+	if (data->chip->set_power)
+		data->chip->set_power(data, false);
 
 	video_unregister_device(&data->vdev);
 
@@ -804,6 +810,9 @@  static int video_i2c_pm_runtime_suspend(struct device *dev)
 {
 	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
 
+	if (!data->chip->set_power)
+		return 0;
+
 	return data->chip->set_power(data, false);
 }
 
@@ -811,6 +820,9 @@  static int video_i2c_pm_runtime_resume(struct device *dev)
 {
 	struct video_i2c_data *data = i2c_get_clientdata(to_i2c_client(dev));
 
+	if (!data->chip->set_power)
+		return 0;
+
 	return data->chip->set_power(data, true);
 }
 

Privacy Policy