Re: [PATCH v2] media: uvcvideo: user entity get_cur in uvc_ctrl_set

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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