On Mon, 27 Jun 2022 at 03:08, Yunke Cao <yunkec@xxxxxxxxxx> wrote:
>
> 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.
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
