All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] media: video-i2c: check if chip struct has set_power function
@ 2018-11-24 22:03 Matt Ranostay
  2018-11-25 15:17 ` Akinobu Mita
  2018-11-25 16:55 ` Sakari Ailus
  0 siblings, 2 replies; 5+ messages in thread
From: Matt Ranostay @ 2018-11-24 22:03 UTC (permalink / raw)
  To: linux-media
  Cc: Matt Ranostay, Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab,
	Akinobu Mita

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;
+	}
 
 	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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
  2018-11-24 22:03 [PATCH v3] media: video-i2c: check if chip struct has set_power function Matt Ranostay
@ 2018-11-25 15:17 ` Akinobu Mita
  2018-11-25 16:55 ` Sakari Ailus
  1 sibling, 0 replies; 5+ messages in thread
From: Akinobu Mita @ 2018-11-25 15:17 UTC (permalink / raw)
  To: Matt Ranostay
  Cc: linux-media, Sakari Ailus, Hans Verkuil, Mauro Carvalho Chehab

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>

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
  2018-11-24 22:03 [PATCH v3] media: video-i2c: check if chip struct has set_power function Matt Ranostay
  2018-11-25 15:17 ` Akinobu Mita
@ 2018-11-25 16:55 ` Sakari Ailus
  2018-11-26 13:50   ` Hans Verkuil
  1 sibling, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2018-11-25 16:55 UTC (permalink / raw)
  To: Matt Ranostay
  Cc: linux-media, Hans Verkuil, Mauro Carvalho Chehab, Akinobu Mita

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
> 

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
  2018-11-25 16:55 ` Sakari Ailus
@ 2018-11-26 13:50   ` Hans Verkuil
  2018-11-26 15:01     ` Matt Ranostay
  0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2018-11-26 13:50 UTC (permalink / raw)
  To: Sakari Ailus, Matt Ranostay
  Cc: linux-media, Hans Verkuil, Mauro Carvalho Chehab, Akinobu Mita

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
  2018-11-26 13:50   ` Hans Verkuil
@ 2018-11-26 15:01     ` Matt Ranostay
  0 siblings, 0 replies; 5+ messages in thread
From: Matt Ranostay @ 2018-11-26 15:01 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sakari Ailus, linux-media, Hans Verkuil, Mauro Carvalho Chehab,
	Akinobu Mita

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-11-27  1:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-24 22:03 [PATCH v3] media: video-i2c: check if chip struct has set_power function Matt Ranostay
2018-11-25 15:17 ` Akinobu Mita
2018-11-25 16:55 ` Sakari Ailus
2018-11-26 13:50   ` Hans Verkuil
2018-11-26 15:01     ` Matt Ranostay

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.