Thanks Ricardo for the review!
On Wed, Jun 22, 2022 at 4:58 AM Ricardo Ribalda <ribalda@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> > 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
