Hi Laurent,
Thanks for the feedback.
On Thu, Jul 7, 2022 at 8:39 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@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?
>
> 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@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;
> > > > }
>
> --
> Regards,
>
> Laurent Pinchart
