Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
On Mon, Nov 26, 2018 at 2:50 PM Hans Verkuil 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
> >> Cc: Hans Verkuil
> >> Cc: Mauro Carvalho Chehab
> >> Cc: Akinobu Mita
> >> Signed-off-by: Matt Ranostay
> >> ---
> >>
> >> 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(>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(>dev);
> >> pm_runtime_set_active(>dev);
> >> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
> >> pm_runtime_disable(>dev);
> >> pm_runtime_set_suspended(>dev);
> >> pm_runtime_put_noidle(>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(>dev);
> >> pm_runtime_set_suspended(>dev);
> >> pm_runtime_put_noidle(>dev);
> >> -data->chip->set_power(data, false);
> >> +
> >> +if (data->chip->set_power)
> >> +data->chip->set_power(data, false);
> >>
> >> video_unregister_device(>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
> >>
> >
>
Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
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
>> Cc: Hans Verkuil
>> Cc: Mauro Carvalho Chehab
>> Cc: Akinobu Mita
>> Signed-off-by: Matt Ranostay
>> ---
>>
>> 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(>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(>dev);
>> pm_runtime_set_active(>dev);
>> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
>> pm_runtime_disable(>dev);
>> pm_runtime_set_suspended(>dev);
>> pm_runtime_put_noidle(>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(>dev);
>> pm_runtime_set_suspended(>dev);
>> pm_runtime_put_noidle(>dev);
>> -data->chip->set_power(data, false);
>> +
>> +if (data->chip->set_power)
>> +data->chip->set_power(data, false);
>>
>> video_unregister_device(>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
>>
>
Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
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
> Cc: Hans Verkuil
> Cc: Mauro Carvalho Chehab
> Cc: Akinobu Mita
> Signed-off-by: Matt Ranostay
> ---
>
> 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(>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(>dev);
> pm_runtime_set_active(>dev);
> @@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
> pm_runtime_disable(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>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(>dev);
> pm_runtime_set_suspended(>dev);
> pm_runtime_put_noidle(>dev);
> - data->chip->set_power(data, false);
> +
> + if (data->chip->set_power)
> + data->chip->set_power(data, false);
>
> video_unregister_device(>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.ai...@linux.intel.com
Re: [PATCH v3] media: video-i2c: check if chip struct has set_power function
2018年11月25日(日) 7:03 Matt Ranostay : > > 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 > Cc: Hans Verkuil > Cc: Mauro Carvalho Chehab > Cc: Akinobu Mita > Signed-off-by: Matt Ranostay Looks good. Reviewed-by: Akinobu Mita
[PATCH v3] media: video-i2c: check if chip struct has set_power function
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
Cc: Hans Verkuil
Cc: Mauro Carvalho Chehab
Cc: Akinobu Mita
Signed-off-by: Matt Ranostay
---
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(>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(>dev);
pm_runtime_set_active(>dev);
@@ -767,7 +769,9 @@ static int video_i2c_probe(struct i2c_client *client,
pm_runtime_disable(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>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(>dev);
pm_runtime_set_suspended(>dev);
pm_runtime_put_noidle(>dev);
- data->chip->set_power(data, false);
+
+ if (data->chip->set_power)
+ data->chip->set_power(data, false);
video_unregister_device(>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
