Re: [PATCH 2/3] media: uvcvideo: Refactor uvc_query_ctrl

[Ricardo Ribalda]

Hi Sakari!

On Tue, 8 Oct 2024 at 20:01, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
>
> Hi Ricardo,
>
> On Tue, Oct 08, 2024 at 07:06:15AM +0000, Ricardo Ribalda wrote:
> > Move the query control error logic to its own function.
> > There is no functional change introduced by this patch.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 45 ++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 853dfb7b5f7b..a57272a2c9e1 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -67,30 +67,12 @@ static const char *uvc_query_name(u8 query)
> >       }
> >  }
> >
> > -int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > -                     u8 intfnum, u8 cs, void *data, u16 size)
> > +static int uvc_query_ctrl_error(struct uvc_device *dev, u8 intfnum, void *data)
> >  {
> >       int ret;
> >       u8 error;
> >       u8 tmp;
> >
> > -     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > -                             UVC_CTRL_CONTROL_TIMEOUT);
> > -     if (likely(ret == size))
> > -             return 0;
> > -
> > -     if (ret > 0 && ret < size) {
> > -             memset(data + ret, 0, size - ret);
> > -             return 0;
> > -     }
> > -
> > -     if (ret != -EPIPE) {
> > -             dev_err(&dev->udev->dev,
> > -                     "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > -                     uvc_query_name(query), cs, unit, ret, size);
> > -             return ret ? ret : -EPIPE;
> > -     }
> > -
> >       /* Reuse data[0] to request the error code. */
> >       tmp = *(u8 *)data;
> >
> > @@ -135,6 +117,31 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> >       return -EPIPE;
> >  }
> >
> > +int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
> > +                u8 intfnum, u8 cs, void *data, u16 size)
> > +{
> > +     int ret;
> > +
> > +     ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> > +                            UVC_CTRL_CONTROL_TIMEOUT);
> > +     if (likely(ret == size))
> > +             return 0;
> > +
> > +     if (ret == -EPIPE)
> > +             return uvc_query_ctrl_error(dev, intfnum, data);
> > +
> > +     dev_err(&dev->udev->dev,
> > +             "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> > +             uvc_query_name(query), cs, unit, ret, size);
>
> This message should probably be printed after the check below.

If the device is returning less bytes, the hardware is not behaving
according to spec and it is good information, specially if you are
bringing up a new device.
I could make it  a dev_warn() (or even uvc_debug) if ret <size. WDYT?


>
> I'd actually move the below check before the ret == -EPIPE check as it's a
> successful case (and changing the condition to <= would make the ret ==
> size check redundant).

something like this?

if (ret > 0)  {
   if (ret != size) {
      print_error();
      memcpy();
   }
   return 0;
}

>
> > +
> > +     if (ret > 0 && ret < size) {
> > +             memset(data + ret, 0, size - ret);
> > +             return 0;
> > +     }
> > +
> > +     return ret ? ret : -EPIPE;
> > +}
> > +
> >  static const struct usb_device_id elgato_cam_link_4k = {
> >       USB_DEVICE(0x0fd9, 0x0066)
> >  };
> >
>
> --
> Kind regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda