On 23/03/2021 16:25, Ricardo Ribalda Delgado wrote: > Hi Hans > > Thanks for the patch. I like how uvc is ending :) > > On Mon, Mar 22, 2021 at 1:09 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: >> >> - If __uvc_query_ctrl() failed with a non-EPIPE error, then >> report that with dev_err. If an error code is obtained, then >> report that with dev_dbg. >> >> - For error 2 (Wrong state) return -EACCES instead of -EILSEQ. >> EACCES is a much more appropriate error code. EILSEQ will return >> "Invalid or incomplete multibyte or wide character." in strerror(), >> which is a *very* confusing message. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> >> --- >> Ricardo, this too can be added to the uvc series. >> --- >> drivers/media/usb/uvc/uvc_video.c | 44 +++++++++++++++++-------------- >> 1 file changed, 24 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c >> index b63c073ec30e..3f461bb4eeb9 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -68,7 +68,7 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >> u8 intfnum, u8 cs, void *data, u16 size) >> { >> int ret; >> - u8 error; >> + u8 error = 0; >> u8 tmp; >> >> ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size, >> @@ -76,35 +76,39 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit, >> if (likely(ret == size)) >> return 0; >> >> - dev_dbg(&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); >> + ret = ret < 0 ? ret : -EPIPE; >> >> - if (ret != -EPIPE) >> - return ret; >> - >> - tmp = *(u8 *)data; >> + if (ret == -EPIPE) { >> + tmp = *(u8 *)data; >> >> - ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >> - UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >> - UVC_CTRL_CONTROL_TIMEOUT); >> + ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum, >> + UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1, >> + UVC_CTRL_CONTROL_TIMEOUT); >> >> - error = *(u8 *)data; >> - *(u8 *)data = tmp; >> + if (ret == 1) >> + error = *(u8 *)data; >> + *(u8 *)data = tmp; >> + if (ret != 1) >> + ret = ret < 0 ? ret : -EPIPE; >> + } >> >> - if (ret != 1) >> - return ret < 0 ? ret : -EPIPE; >> + if (error) >> + dev_dbg(&dev->udev->dev, >> + "Failed to query (%s) UVC control %u on unit %u: got error %u.\n", >> + uvc_query_name(query), cs, unit, error); >> + else >> + 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); > > If __uvc_query_ctrl and UVC_VC_REQUEST_ERROR_CODE_CONTROL failed, > error is 0. And I think that you want to show a dev_err in that case. > Maybe we can initialize error to 7 ? I'm confused, if error == 0, then it does show dev_err. > > >> >> - uvc_dbg(dev, CONTROL, "Control error %u\n", error); >> + if (!error) >> + return ret; > I think we do not want these two lines (read next comment) >> >> switch (error) { >> - case 0: >> - /* Cannot happen - we received a STALL */ >> - return -EPIPE; >> case 1: /* Not ready */ >> return -EBUSY; >> case 2: /* Wrong state */ >> - return -EILSEQ; >> + return -EACCES; >> case 3: /* Power */ >> return -EREMOTE; >> case 4: /* Out of range */ > > Maybe we want a dev_dbg if the error code is unknown and return ret? Make sense. Regards, Hans