* [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.