* [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
@ 2022-06-21 6:15 Yunke Cao
2022-06-21 19:58 ` Ricardo Ribalda
0 siblings, 1 reply; 8+ messages in thread
From: Yunke Cao @ 2022-06-21 6:15 UTC (permalink / raw)
To: Laurent Pinchart, Ricardo Ribalda, Mauro Carvalho Chehab
Cc: linux-media, Yunke Cao
Entity controls should get_cur using an entity-defined function
instead of via a query. Fix this in uvc_ctrl_set.
Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
Signed-off-by: Yunke Cao <yunkec@google.com>
---
Changelog since v1:
-Factored out common logic into __uvc_ctrl_load_cur().
drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
1 file changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 0e78233fc8a0..e25177c95571 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
return value;
}
-static int __uvc_ctrl_get(struct uvc_video_chain *chain,
- struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
- s32 *value)
+static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
+ struct uvc_control *ctrl)
{
- int ret;
+ int ret = 0;
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
return -EACCES;
- if (!ctrl->loaded) {
- if (ctrl->entity->get_cur) {
- ret = ctrl->entity->get_cur(chain->dev,
- ctrl->entity,
- ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
- ctrl->info.size);
- } else {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
- ctrl->entity->id,
- chain->dev->intfnum,
- ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
- ctrl->info.size);
- }
- if (ret < 0)
- return ret;
+ if (ctrl->loaded)
+ return 0;
- ctrl->loaded = 1;
+ if (ctrl->entity->get_cur) {
+ ret = ctrl->entity->get_cur(chain->dev,
+ ctrl->entity,
+ ctrl->info.selector,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
+ ctrl->info.size);
+ } else {
+ ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
+ ctrl->entity->id, chain->dev->intfnum,
+ ctrl->info.selector,
+ uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
+ ctrl->info.size);
}
+ if (ret < 0)
+ return ret;
+
+ ctrl->loaded = 1;
+
+ return ret;
+}
+
+static int __uvc_ctrl_get(struct uvc_video_chain *chain,
+ struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
+ s32 *value)
+{
+ int ret = __uvc_ctrl_load_cur(chain, ctrl);
+
+ if (ret < 0)
+ return ret;
+
*value = __uvc_ctrl_get_value(mapping,
uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
@@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
0, ctrl->info.size);
} else {
- ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
- ctrl->entity->id, chain->dev->intfnum,
- ctrl->info.selector,
- uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
- ctrl->info.size);
+ ret = __uvc_ctrl_load_cur(chain, ctrl);
if (ret < 0)
return ret;
}
--
2.37.0.rc0.104.g0611611a94-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-06-21 6:15 [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set Yunke Cao
@ 2022-06-21 19:58 ` Ricardo Ribalda
2022-06-27 1:08 ` Yunke Cao
0 siblings, 1 reply; 8+ messages in thread
From: Ricardo Ribalda @ 2022-06-21 19:58 UTC (permalink / raw)
To: Yunke Cao; +Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media
Hi Yunke
Thanks for your patch
Another way to do it could be:
__uvc_ctrl_load_cur() {
if (loaded)
return
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
memset
loaded = true;
return
}
...
}
int __uvc_ctrl_get() {
if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
return -EACCES;
....
}
Then you could even simplify more the code in uvc_ctrl_set, and limit
the m handling of the loaded flag.
if ( (ctrl->info.size * 8) != mapping->size)
___uvc_ctrl_load_cur()
But It is probably just a matter of taste ;). It is up to you and
Laurent to pick what do you prefer.
Regards!
On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
>
> Entity controls should get_cur using an entity-defined function
> instead of via a query. Fix this in uvc_ctrl_set.
>
> Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> Signed-off-by: Yunke Cao <yunkec@google.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
> Changelog since v1:
> -Factored out common logic into __uvc_ctrl_load_cur().
>
> drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> 1 file changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 0e78233fc8a0..e25177c95571 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> return value;
> }
>
> -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> - s32 *value)
> +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl)
> {
> - int ret;
> + int ret = 0;
nit : why do you init to 0?
>
> if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> return -EACCES;
>
> - if (!ctrl->loaded) {
> - if (ctrl->entity->get_cur) {
nit: is this spaced properly?
> - ret = ctrl->entity->get_cur(chain->dev,
> - ctrl->entity,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> - } else {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id,
> - chain->dev->intfnum,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> - }
> - if (ret < 0)
> - return ret;
> + if (ctrl->loaded)
> + return 0;
>
> - ctrl->loaded = 1;
> + if (ctrl->entity->get_cur) {
> + ret = ctrl->entity->get_cur(chain->dev,
> + ctrl->entity,
> + ctrl->info.selector,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> + ctrl->info.size);
> + } else {
> + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> + ctrl->entity->id, chain->dev->intfnum,
> + ctrl->info.selector,
> + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> + ctrl->info.size);
> }
>
> + if (ret < 0)
> + return ret;
> +
> + ctrl->loaded = 1;
> +
> + return ret;
> +}
> +
> +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> + s32 *value)
> +{
nit: Is the alignment correct here?
> + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> +
> + if (ret < 0)
> + return ret;
> +
> *value = __uvc_ctrl_get_value(mapping,
> uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
>
> @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> 0, ctrl->info.size);
> } else {
> - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> - ctrl->entity->id, chain->dev->intfnum,
> - ctrl->info.selector,
> - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> - ctrl->info.size);
> + ret = __uvc_ctrl_load_cur(chain, ctrl);
> if (ret < 0)
> return ret;
> }
> --
> 2.37.0.rc0.104.g0611611a94-goog
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-06-21 19:58 ` Ricardo Ribalda
@ 2022-06-27 1:08 ` Yunke Cao
2022-06-27 6:00 ` Ricardo Ribalda
2022-07-06 23:39 ` Laurent Pinchart
0 siblings, 2 replies; 8+ messages in thread
From: Yunke Cao @ 2022-06-27 1:08 UTC (permalink / raw)
To: Ricardo Ribalda; +Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media
Thanks Ricardo for the review!
On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Yunke
>
> Thanks for your patch
>
> Another way to do it could be:
>
> __uvc_ctrl_load_cur() {
> if (loaded)
> return
> if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> memset
> loaded = true;
> return
> }
> ...
> }
>
> int __uvc_ctrl_get() {
> if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> return -EACCES;
> ....
> }
>
> Then you could even simplify more the code in uvc_ctrl_set, and limit
> the m handling of the loaded flag.
>
>
> if ( (ctrl->info.size * 8) != mapping->size)
> ___uvc_ctrl_load_cur()
>
>
> But It is probably just a matter of taste ;). It is up to you and
> Laurent to pick what do you prefer.
>
> Regards!
Thanks for the suggestion. This does look cleaner. I like your idea better.
Laurent, do you have any preference?
>
>
> On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> >
> > Entity controls should get_cur using an entity-defined function
> > instead of via a query. Fix this in uvc_ctrl_set.
> >
> > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > Signed-off-by: Yunke Cao <yunkec@google.com>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > Changelog since v1:
> > -Factored out common logic into __uvc_ctrl_load_cur().
> >
> > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > 1 file changed, 35 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 0e78233fc8a0..e25177c95571 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > return value;
> > }
> >
> > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > - s32 *value)
> > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > + struct uvc_control *ctrl)
> > {
> > - int ret;
> > + int ret = 0;
> nit : why do you init to 0?
I will remove it in v3.
> >
> > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > return -EACCES;
> >
> > - if (!ctrl->loaded) {
> > - if (ctrl->entity->get_cur) {
> nit: is this spaced properly?
This is deleted code or am I looking in the wrong place.
> > - ret = ctrl->entity->get_cur(chain->dev,
> > - ctrl->entity,
> > - ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > - ctrl->info.size);
> > - } else {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > - ctrl->entity->id,
> > - chain->dev->intfnum,
> > - ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > - ctrl->info.size);
> > - }
> > - if (ret < 0)
> > - return ret;
> > + if (ctrl->loaded)
> > + return 0;
> >
> > - ctrl->loaded = 1;
> > + if (ctrl->entity->get_cur) {
> > + ret = ctrl->entity->get_cur(chain->dev,
> > + ctrl->entity,
> > + ctrl->info.selector,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > + ctrl->info.size);
> > + } else {
> > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > + ctrl->entity->id, chain->dev->intfnum,
> > + ctrl->info.selector,
> > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > + ctrl->info.size);
> > }
> >
> > + if (ret < 0)
> > + return ret;
> > +
> > + ctrl->loaded = 1;
> > +
> > + return ret;
> > +}
> > +
> > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > + s32 *value)
> > +{
> nit: Is the alignment correct here?
Will fix it in v3.
Best,
Yunke
> > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > *value = __uvc_ctrl_get_value(mapping,
> > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> >
> > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > 0, ctrl->info.size);
> > } else {
> > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > - ctrl->entity->id, chain->dev->intfnum,
> > - ctrl->info.selector,
> > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > - ctrl->info.size);
> > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > if (ret < 0)
> > return ret;
> > }
> > --
> > 2.37.0.rc0.104.g0611611a94-goog
>
>
>
> --
> Ricardo Ribalda
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-06-27 1:08 ` Yunke Cao
@ 2022-06-27 6:00 ` Ricardo Ribalda
2022-07-06 23:39 ` Laurent Pinchart
1 sibling, 0 replies; 8+ messages in thread
From: Ricardo Ribalda @ 2022-06-27 6:00 UTC (permalink / raw)
To: Yunke Cao; +Cc: Laurent Pinchart, Mauro Carvalho Chehab, linux-media
On Mon, 27 Jun 2022 at 03:08, Yunke Cao <yunkec@google.com> wrote:
>
> Thanks Ricardo for the review!
>
> On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > Hi Yunke
> >
> > Thanks for your patch
> >
> > Another way to do it could be:
> >
> > __uvc_ctrl_load_cur() {
> > if (loaded)
> > return
> > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> > memset
> > loaded = true;
> > return
> > }
> > ...
> > }
> >
> > int __uvc_ctrl_get() {
> > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > return -EACCES;
> > ....
> > }
> >
> > Then you could even simplify more the code in uvc_ctrl_set, and limit
> > the m handling of the loaded flag.
> >
> >
> > if ( (ctrl->info.size * 8) != mapping->size)
> > ___uvc_ctrl_load_cur()
> >
> >
> > But It is probably just a matter of taste ;). It is up to you and
> > Laurent to pick what do you prefer.
> >
> > Regards!
>
> Thanks for the suggestion. This does look cleaner. I like your idea better.
> Laurent, do you have any preference?
>
> >
> >
> > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> > >
> > > Entity controls should get_cur using an entity-defined function
> > > instead of via a query. Fix this in uvc_ctrl_set.
> > >
> > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > Changelog since v1:
> > > -Factored out common logic into __uvc_ctrl_load_cur().
> > >
> > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > > 1 file changed, 35 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 0e78233fc8a0..e25177c95571 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > > return value;
> > > }
> > >
> > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > - s32 *value)
> > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > > + struct uvc_control *ctrl)
> > > {
> > > - int ret;
> > > + int ret = 0;
> > nit : why do you init to 0?
>
> I will remove it in v3.
>
> > >
> > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > return -EACCES;
> > >
> > > - if (!ctrl->loaded) {
> > > - if (ctrl->entity->get_cur) {
> > nit: is this spaced properly?
>
> This is deleted code or am I looking in the wrong place.
I probably screwed up, sorry ;(
>
> > > - ret = ctrl->entity->get_cur(chain->dev,
> > > - ctrl->entity,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > - } else {
> > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > - ctrl->entity->id,
> > > - chain->dev->intfnum,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > - }
> > > - if (ret < 0)
> > > - return ret;
> > > + if (ctrl->loaded)
> > > + return 0;
> > >
> > > - ctrl->loaded = 1;
> > > + if (ctrl->entity->get_cur) {
> > > + ret = ctrl->entity->get_cur(chain->dev,
> > > + ctrl->entity,
> > > + ctrl->info.selector,
> > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > + ctrl->info.size);
> > > + } else {
> > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > + ctrl->entity->id, chain->dev->intfnum,
> > > + ctrl->info.selector,
> > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > + ctrl->info.size);
> > > }
> > >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ctrl->loaded = 1;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > + s32 *value)
> > > +{
> > nit: Is the alignment correct here?
>
> Will fix it in v3.
>
> Best,
> Yunke
>
> > > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > *value = __uvc_ctrl_get_value(mapping,
> > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > >
> > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > 0, ctrl->info.size);
> > > } else {
> > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > - ctrl->entity->id, chain->dev->intfnum,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > if (ret < 0)
> > > return ret;
> > > }
> > > --
> > > 2.37.0.rc0.104.g0611611a94-goog
> >
> >
> >
> > --
> > Ricardo Ribalda
--
Ricardo Ribalda
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-06-27 1:08 ` Yunke Cao
2022-06-27 6:00 ` Ricardo Ribalda
@ 2022-07-06 23:39 ` Laurent Pinchart
2022-07-07 4:54 ` Yunke Cao
1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2022-07-06 23:39 UTC (permalink / raw)
To: Yunke Cao; +Cc: Ricardo Ribalda, Mauro Carvalho Chehab, linux-media
On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote:
> Thanks Ricardo for the review!
>
> On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
> >
> > Hi Yunke
> >
> > Thanks for your patch
> >
> > Another way to do it could be:
> >
> > __uvc_ctrl_load_cur() {
> > if (loaded)
> > return
> > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> > memset
> > loaded = true;
> > return
> > }
> > ...
> > }
> >
> > int __uvc_ctrl_get() {
> > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > return -EACCES;
> > ....
> > }
> >
> > Then you could even simplify more the code in uvc_ctrl_set, and limit
> > the m handling of the loaded flag.
> >
> >
> > if ( (ctrl->info.size * 8) != mapping->size)
> > ___uvc_ctrl_load_cur()
> >
> >
> > But It is probably just a matter of taste ;). It is up to you and
> > Laurent to pick what do you prefer.
> >
> > Regards!
>
> Thanks for the suggestion. This does look cleaner. I like your idea better.
> Laurent, do you have any preference?
I agree, it looks cleaner.
Do I understand correctly that this patch will currently have no effect,
given that only the fake GPIO entity has a .get_cur() function, and that
entity doesn't expose any control that can be set ?
> > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> > >
> > > Entity controls should get_cur using an entity-defined function
> > > instead of via a query. Fix this in uvc_ctrl_set.
> > >
> > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > > Changelog since v1:
> > > -Factored out common logic into __uvc_ctrl_load_cur().
> > >
> > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > > 1 file changed, 35 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 0e78233fc8a0..e25177c95571 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > > return value;
> > > }
> > >
> > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > - s32 *value)
> > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > > + struct uvc_control *ctrl)
> > > {
> > > - int ret;
> > > + int ret = 0;
> > nit : why do you init to 0?
>
> I will remove it in v3.
>
> > >
> > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > return -EACCES;
> > >
> > > - if (!ctrl->loaded) {
> > > - if (ctrl->entity->get_cur) {
> > nit: is this spaced properly?
>
> This is deleted code or am I looking in the wrong place.
>
> > > - ret = ctrl->entity->get_cur(chain->dev,
> > > - ctrl->entity,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > - } else {
> > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > - ctrl->entity->id,
> > > - chain->dev->intfnum,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > - }
> > > - if (ret < 0)
> > > - return ret;
> > > + if (ctrl->loaded)
> > > + return 0;
> > >
> > > - ctrl->loaded = 1;
> > > + if (ctrl->entity->get_cur) {
> > > + ret = ctrl->entity->get_cur(chain->dev,
> > > + ctrl->entity,
> > > + ctrl->info.selector,
> > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > + ctrl->info.size);
> > > + } else {
> > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > + ctrl->entity->id, chain->dev->intfnum,
> > > + ctrl->info.selector,
> > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > + ctrl->info.size);
> > > }
> > >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ctrl->loaded = 1;
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > + s32 *value)
> > > +{
> > nit: Is the alignment correct here?
>
> Will fix it in v3.
>
> Best,
> Yunke
>
> > > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > +
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > *value = __uvc_ctrl_get_value(mapping,
> > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > >
> > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > 0, ctrl->info.size);
> > > } else {
> > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > - ctrl->entity->id, chain->dev->intfnum,
> > > - ctrl->info.selector,
> > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > - ctrl->info.size);
> > > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > if (ret < 0)
> > > return ret;
> > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-07-06 23:39 ` Laurent Pinchart
@ 2022-07-07 4:54 ` Yunke Cao
2022-07-07 8:12 ` Laurent Pinchart
0 siblings, 1 reply; 8+ messages in thread
From: Yunke Cao @ 2022-07-07 4:54 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Ricardo Ribalda, Mauro Carvalho Chehab, linux-media
Hi Laurent,
Thanks for the feedback.
On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote:
> > Thanks Ricardo for the review!
> >
> > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > >
> > > Hi Yunke
> > >
> > > Thanks for your patch
> > >
> > > Another way to do it could be:
> > >
> > > __uvc_ctrl_load_cur() {
> > > if (loaded)
> > > return
> > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> > > memset
> > > loaded = true;
> > > return
> > > }
> > > ...
> > > }
> > >
> > > int __uvc_ctrl_get() {
> > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > return -EACCES;
> > > ....
> > > }
> > >
> > > Then you could even simplify more the code in uvc_ctrl_set, and limit
> > > the m handling of the loaded flag.
> > >
> > >
> > > if ( (ctrl->info.size * 8) != mapping->size)
> > > ___uvc_ctrl_load_cur()
> > >
> > >
> > > But It is probably just a matter of taste ;). It is up to you and
> > > Laurent to pick what do you prefer.
> > >
> > > Regards!
> >
> > Thanks for the suggestion. This does look cleaner. I like your idea better.
> > Laurent, do you have any preference?
>
> I agree, it looks cleaner.
>
> Do I understand correctly that this patch will currently have no effect,
> given that only the fake GPIO entity has a .get_cur() function, and that
> entity doesn't expose any control that can be set ?
>
Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong.
Do we still want to merge this? If so, I will send v3 shortly.
Best,
Yunke
> > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> > > >
> > > > Entity controls should get_cur using an entity-defined function
> > > > instead of via a query. Fix this in uvc_ctrl_set.
> > > >
> > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > ---
> > > > Changelog since v1:
> > > > -Factored out common logic into __uvc_ctrl_load_cur().
> > > >
> > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > > > 1 file changed, 35 insertions(+), 27 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 0e78233fc8a0..e25177c95571 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > > > return value;
> > > > }
> > > >
> > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > - s32 *value)
> > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > > > + struct uvc_control *ctrl)
> > > > {
> > > > - int ret;
> > > > + int ret = 0;
> > > nit : why do you init to 0?
> >
> > I will remove it in v3.
> >
> > > >
> > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > > return -EACCES;
> > > >
> > > > - if (!ctrl->loaded) {
> > > > - if (ctrl->entity->get_cur) {
> > > nit: is this spaced properly?
> >
> > This is deleted code or am I looking in the wrong place.
> >
> > > > - ret = ctrl->entity->get_cur(chain->dev,
> > > > - ctrl->entity,
> > > > - ctrl->info.selector,
> > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > - ctrl->info.size);
> > > > - } else {
> > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > - ctrl->entity->id,
> > > > - chain->dev->intfnum,
> > > > - ctrl->info.selector,
> > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > - ctrl->info.size);
> > > > - }
> > > > - if (ret < 0)
> > > > - return ret;
> > > > + if (ctrl->loaded)
> > > > + return 0;
> > > >
> > > > - ctrl->loaded = 1;
> > > > + if (ctrl->entity->get_cur) {
> > > > + ret = ctrl->entity->get_cur(chain->dev,
> > > > + ctrl->entity,
> > > > + ctrl->info.selector,
> > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > + ctrl->info.size);
> > > > + } else {
> > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > + ctrl->entity->id, chain->dev->intfnum,
> > > > + ctrl->info.selector,
> > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > + ctrl->info.size);
> > > > }
> > > >
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > + ctrl->loaded = 1;
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > + s32 *value)
> > > > +{
> > > nit: Is the alignment correct here?
> >
> > Will fix it in v3.
> >
> > Best,
> > Yunke
> >
> > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > +
> > > > + if (ret < 0)
> > > > + return ret;
> > > > +
> > > > *value = __uvc_ctrl_get_value(mapping,
> > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > > >
> > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > 0, ctrl->info.size);
> > > > } else {
> > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > - ctrl->entity->id, chain->dev->intfnum,
> > > > - ctrl->info.selector,
> > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > - ctrl->info.size);
> > > > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > if (ret < 0)
> > > > return ret;
> > > > }
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-07-07 4:54 ` Yunke Cao
@ 2022-07-07 8:12 ` Laurent Pinchart
2022-07-07 8:55 ` Yunke Cao
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2022-07-07 8:12 UTC (permalink / raw)
To: Yunke Cao; +Cc: Ricardo Ribalda, Mauro Carvalho Chehab, linux-media
Hi Yunke,
On Thu, Jul 07, 2022 at 01:54:52PM +0900, Yunke Cao wrote:
> On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart wrote:
> > On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote:
> > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda wrote:
> > > >
> > > > Hi Yunke
> > > >
> > > > Thanks for your patch
> > > >
> > > > Another way to do it could be:
> > > >
> > > > __uvc_ctrl_load_cur() {
> > > > if (loaded)
> > > > return
> > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> > > > memset
> > > > loaded = true;
> > > > return
> > > > }
> > > > ...
> > > > }
> > > >
> > > > int __uvc_ctrl_get() {
> > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > > return -EACCES;
> > > > ....
> > > > }
> > > >
> > > > Then you could even simplify more the code in uvc_ctrl_set, and limit
> > > > the m handling of the loaded flag.
> > > >
> > > >
> > > > if ( (ctrl->info.size * 8) != mapping->size)
> > > > ___uvc_ctrl_load_cur()
> > > >
> > > >
> > > > But It is probably just a matter of taste ;). It is up to you and
> > > > Laurent to pick what do you prefer.
> > > >
> > > > Regards!
> > >
> > > Thanks for the suggestion. This does look cleaner. I like your idea better.
> > > Laurent, do you have any preference?
> >
> > I agree, it looks cleaner.
> >
> > Do I understand correctly that this patch will currently have no effect,
> > given that only the fake GPIO entity has a .get_cur() function, and that
> > entity doesn't expose any control that can be set ?
>
> Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong.
> Do we still want to merge this? If so, I will send v3 shortly.
It's cleaner and will prevent future problems in case more users of
.get_cur() appear, so I think the patch has value.
> > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> > > > >
> > > > > Entity controls should get_cur using an entity-defined function
> > > > > instead of via a query. Fix this in uvc_ctrl_set.
> > > > >
> > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > > > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > ---
> > > > > Changelog since v1:
> > > > > -Factored out common logic into __uvc_ctrl_load_cur().
> > > > >
> > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > > > > 1 file changed, 35 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > index 0e78233fc8a0..e25177c95571 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > > > > return value;
> > > > > }
> > > > >
> > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > > - s32 *value)
> > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > > > > + struct uvc_control *ctrl)
> > > > > {
> > > > > - int ret;
> > > > > + int ret = 0;
> > > > nit : why do you init to 0?
> > >
> > > I will remove it in v3.
> > >
> > > > >
> > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > > > return -EACCES;
> > > > >
> > > > > - if (!ctrl->loaded) {
> > > > > - if (ctrl->entity->get_cur) {
> > > > nit: is this spaced properly?
> > >
> > > This is deleted code or am I looking in the wrong place.
> > >
> > > > > - ret = ctrl->entity->get_cur(chain->dev,
> > > > > - ctrl->entity,
> > > > > - ctrl->info.selector,
> > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > - ctrl->info.size);
> > > > > - } else {
> > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > - ctrl->entity->id,
> > > > > - chain->dev->intfnum,
> > > > > - ctrl->info.selector,
> > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > - ctrl->info.size);
> > > > > - }
> > > > > - if (ret < 0)
> > > > > - return ret;
> > > > > + if (ctrl->loaded)
> > > > > + return 0;
> > > > >
> > > > > - ctrl->loaded = 1;
> > > > > + if (ctrl->entity->get_cur) {
> > > > > + ret = ctrl->entity->get_cur(chain->dev,
> > > > > + ctrl->entity,
> > > > > + ctrl->info.selector,
> > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > + ctrl->info.size);
> > > > > + } else {
> > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > + ctrl->entity->id, chain->dev->intfnum,
> > > > > + ctrl->info.selector,
> > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > + ctrl->info.size);
> > > > > }
> > > > >
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > + ctrl->loaded = 1;
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > > + s32 *value)
> > > > > +{
> > > > nit: Is the alignment correct here?
> > >
> > > Will fix it in v3.
> > >
> > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > > +
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > +
> > > > > *value = __uvc_ctrl_get_value(mapping,
> > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > > > >
> > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > 0, ctrl->info.size);
> > > > > } else {
> > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > - ctrl->entity->id, chain->dev->intfnum,
> > > > > - ctrl->info.selector,
> > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > - ctrl->info.size);
> > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > > if (ret < 0)
> > > > > return ret;
> > > > > }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set
2022-07-07 8:12 ` Laurent Pinchart
@ 2022-07-07 8:55 ` Yunke Cao
0 siblings, 0 replies; 8+ messages in thread
From: Yunke Cao @ 2022-07-07 8:55 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Ricardo Ribalda, Mauro Carvalho Chehab, linux-media
Thanks Laurent!
Just sent out v3:
Changelog since v2:
-Move the logic of setting ctrl_data to 0 into load_cur.
-Do not initialize ret=0.
-Fix __uvc_ctrl_get() spacing.
-Fix typo in the title.
Best,
Yunke
On Thu, Jul 7, 2022 at 5:12 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Yunke,
>
> On Thu, Jul 07, 2022 at 01:54:52PM +0900, Yunke Cao wrote:
> > On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart wrote:
> > > On Mon, Jun 27, 2022 at 10:08:14AM +0900, Yunke Cao wrote:
> > > > On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda wrote:
> > > > >
> > > > > Hi Yunke
> > > > >
> > > > > Thanks for your patch
> > > > >
> > > > > Another way to do it could be:
> > > > >
> > > > > __uvc_ctrl_load_cur() {
> > > > > if (loaded)
> > > > > return
> > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) {
> > > > > memset
> > > > > loaded = true;
> > > > > return
> > > > > }
> > > > > ...
> > > > > }
> > > > >
> > > > > int __uvc_ctrl_get() {
> > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > > > return -EACCES;
> > > > > ....
> > > > > }
> > > > >
> > > > > Then you could even simplify more the code in uvc_ctrl_set, and limit
> > > > > the m handling of the loaded flag.
> > > > >
> > > > >
> > > > > if ( (ctrl->info.size * 8) != mapping->size)
> > > > > ___uvc_ctrl_load_cur()
> > > > >
> > > > >
> > > > > But It is probably just a matter of taste ;). It is up to you and
> > > > > Laurent to pick what do you prefer.
> > > > >
> > > > > Regards!
> > > >
> > > > Thanks for the suggestion. This does look cleaner. I like your idea better.
> > > > Laurent, do you have any preference?
> > >
> > > I agree, it looks cleaner.
> > >
> > > Do I understand correctly that this patch will currently have no effect,
> > > given that only the fake GPIO entity has a .get_cur() function, and that
> > > entity doesn't expose any control that can be set ?
> >
> > Yes, That's my understanding as well. Ricardo, please correct me if I'm wrong.
> > Do we still want to merge this? If so, I will send v3 shortly.
>
> It's cleaner and will prevent future problems in case more users of
> .get_cur() appear, so I think the patch has value.
>
> > > > > On Tue, 21 Jun 2022 at 08:15, Yunke Cao <yunkec@google.com> wrote:
> > > > > >
> > > > > > Entity controls should get_cur using an entity-defined function
> > > > > > instead of via a query. Fix this in uvc_ctrl_set.
> > > > > >
> > > > > > Fixes: 65900c581d01 ("media: uvcvideo: Allow entity-defined get_info and get_cur")
> > > > > > Signed-off-by: Yunke Cao <yunkec@google.com>
> > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > > ---
> > > > > > Changelog since v1:
> > > > > > -Factored out common logic into __uvc_ctrl_load_cur().
> > > > > >
> > > > > > drivers/media/usb/uvc/uvc_ctrl.c | 62 ++++++++++++++++++--------------
> > > > > > 1 file changed, 35 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > index 0e78233fc8a0..e25177c95571 100644
> > > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > > @@ -963,36 +963,48 @@ static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > > > > > return value;
> > > > > > }
> > > > > >
> > > > > > -static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > > > - struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > > > - s32 *value)
> > > > > > +static int __uvc_ctrl_load_cur(struct uvc_video_chain *chain,
> > > > > > + struct uvc_control *ctrl)
> > > > > > {
> > > > > > - int ret;
> > > > > > + int ret = 0;
> > > > > nit : why do you init to 0?
> > > >
> > > > I will remove it in v3.
> > > >
> > > > > >
> > > > > > if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > > > > > return -EACCES;
> > > > > >
> > > > > > - if (!ctrl->loaded) {
> > > > > > - if (ctrl->entity->get_cur) {
> > > > > nit: is this spaced properly?
> > > >
> > > > This is deleted code or am I looking in the wrong place.
> > > >
> > > > > > - ret = ctrl->entity->get_cur(chain->dev,
> > > > > > - ctrl->entity,
> > > > > > - ctrl->info.selector,
> > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > - ctrl->info.size);
> > > > > > - } else {
> > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > > - ctrl->entity->id,
> > > > > > - chain->dev->intfnum,
> > > > > > - ctrl->info.selector,
> > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > - ctrl->info.size);
> > > > > > - }
> > > > > > - if (ret < 0)
> > > > > > - return ret;
> > > > > > + if (ctrl->loaded)
> > > > > > + return 0;
> > > > > >
> > > > > > - ctrl->loaded = 1;
> > > > > > + if (ctrl->entity->get_cur) {
> > > > > > + ret = ctrl->entity->get_cur(chain->dev,
> > > > > > + ctrl->entity,
> > > > > > + ctrl->info.selector,
> > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > + ctrl->info.size);
> > > > > > + } else {
> > > > > > + ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > > + ctrl->entity->id, chain->dev->intfnum,
> > > > > > + ctrl->info.selector,
> > > > > > + uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > + ctrl->info.size);
> > > > > > }
> > > > > >
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + ctrl->loaded = 1;
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> > > > > > + struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > > > > > + s32 *value)
> > > > > > +{
> > > > > nit: Is the alignment correct here?
> > > >
> > > > Will fix it in v3.
> > > >
> > > > > > + int ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > > > +
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > *value = __uvc_ctrl_get_value(mapping,
> > > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > > > > >
> > > > > > @@ -1788,11 +1800,7 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > > > > memset(uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > 0, ctrl->info.size);
> > > > > > } else {
> > > > > > - ret = uvc_query_ctrl(chain->dev, UVC_GET_CUR,
> > > > > > - ctrl->entity->id, chain->dev->intfnum,
> > > > > > - ctrl->info.selector,
> > > > > > - uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT),
> > > > > > - ctrl->info.size);
> > > > > > + ret = __uvc_ctrl_load_cur(chain, ctrl);
> > > > > > if (ret < 0)
> > > > > > return ret;
> > > > > > }
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-07-07 8:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-21 6:15 [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set Yunke Cao
2022-06-21 19:58 ` Ricardo Ribalda
2022-06-27 1:08 ` Yunke Cao
2022-06-27 6:00 ` Ricardo Ribalda
2022-07-06 23:39 ` Laurent Pinchart
2022-07-07 4:54 ` Yunke Cao
2022-07-07 8:12 ` Laurent Pinchart
2022-07-07 8:55 ` Yunke Cao
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.