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@xxxxxxxxxxxxxxxx> 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@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.
> > > >
> > > > > > + 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
