| Message ID | 7807bf0a-a0a1-65ad-1a10-3a3234e566e9@edgarthier.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, I was wondering if there are any problems with my latest patch or if it simply slipped through. Regards, Edgar On 10/12/2017 09:54 AM, Edgar Thier wrote: > > Use flags the device exposes for UVC controls. > This allows the device to define which property flags are set. > > Since some cameras offer auto-adjustments for properties (e.g. auto-gain), > the values of other properties (e.g. gain) can change in the camera. > Examining the flags ensures that the driver is aware of such properties. > > Signed-off-by: Edgar Thier <info@edgarthier.net> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++---------- > 1 file changed, 49 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 20397aba..8f902a41 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, > } > } > > +/* > + * Retrieve flags for a given control > + */ > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, > + const struct uvc_control_info *info) > +{ > + u8 *data; > + int ret = 0; > + int flags = 0; > + > + data = kmalloc(2, GFP_KERNEL); > + if (data == NULL) > + return -ENOMEM; > + > + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > + info->selector, data, 1); > + if (ret < 0) { > + uvc_trace(UVC_TRACE_CONTROL, > + "GET_INFO failed on control %pUl/%u (%d).\n", > + info->entity, info->selector, ret); > + } else { > + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > + | (data[0] & UVC_CONTROL_CAP_GET ? > + UVC_CTRL_FLAG_GET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_SET ? > + UVC_CTRL_FLAG_SET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > + UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + } > + kfree(data); > + return flags; > +} > + > /* > * Query control information (size and flags) for XU controls. > */ > @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > const struct uvc_control *ctrl, struct uvc_control_info *info) > { > u8 *data; > + int flags; > int ret; > > data = kmalloc(2, GFP_KERNEL); > @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > > info->size = le16_to_cpup((__le16 *)data); > > - /* Query the control information (GET_INFO) */ > - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > - info->selector, data, 1); > - if (ret < 0) { > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + > + if (flags < 0) { > uvc_trace(UVC_TRACE_CONTROL, > - "GET_INFO failed on control %pUl/%u (%d).\n", > - info->entity, info->selector, ret); > + "Failed to retrieve flags (%d).\n", ret); > + ret = flags; > goto done; > } > - > - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > - | (data[0] & UVC_CONTROL_CAP_GET ? > - UVC_CTRL_FLAG_GET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_SET ? > - UVC_CTRL_FLAG_SET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > - UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + info->flags = flags; > > uvc_ctrl_fixup_xu_info(dev, ctrl, info); > > @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > const struct uvc_control_info *info) > { > int ret = 0; > + int flags = 0; > > ctrl->info = *info; > INIT_LIST_HEAD(&ctrl->info.mappings); > @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > goto done; > } > > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + if (flags < 0) > + uvc_trace(UVC_TRACE_CONTROL, > + "Failed to retrieve flags (%d).\n", ret); > + else > + ctrl->info.flags = flags; > + > ctrl->initialized = 1; > > uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " >
Hi Edgar, On 15/11/17 08:11, Edgar Thier wrote: > Hi, > > I was wondering if there are any problems with my latest patch or if it simply slipped through. Slipped though in high mail volumes :) I think it's easier to see updated patches if they are posted as a new thread, with an increased version number. [PATCH v2], [PATCH v3] etc... Not a problem now - but might help your updated patches get seen next time. Looks like my concerns were addressed, so that's a Reviewed-by: tag from me. -- Kieran > > Regards, > > Edgar > > On 10/12/2017 09:54 AM, Edgar Thier wrote: >> >> Use flags the device exposes for UVC controls. >> This allows the device to define which property flags are set. >> >> Since some cameras offer auto-adjustments for properties (e.g. auto-gain), >> the values of other properties (e.g. gain) can change in the camera. >> Examining the flags ensures that the driver is aware of such properties. >> >> Signed-off-by: Edgar Thier <info@edgarthier.net> >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++---------- >> 1 file changed, 49 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index 20397aba..8f902a41 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, >> } >> } >> >> +/* >> + * Retrieve flags for a given control >> + */ >> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, >> + const struct uvc_control_info *info) >> +{ >> + u8 *data; >> + int ret = 0; >> + int flags = 0; >> + >> + data = kmalloc(2, GFP_KERNEL); >> + if (data == NULL) >> + return -ENOMEM; >> + >> + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, >> + info->selector, data, 1); >> + if (ret < 0) { >> + uvc_trace(UVC_TRACE_CONTROL, >> + "GET_INFO failed on control %pUl/%u (%d).\n", >> + info->entity, info->selector, ret); >> + } else { >> + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX >> + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF >> + | (data[0] & UVC_CONTROL_CAP_GET ? >> + UVC_CTRL_FLAG_GET_CUR : 0) >> + | (data[0] & UVC_CONTROL_CAP_SET ? >> + UVC_CTRL_FLAG_SET_CUR : 0) >> + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? >> + UVC_CTRL_FLAG_AUTO_UPDATE : 0); >> + } >> + kfree(data); >> + return flags; >> +} >> + >> /* >> * Query control information (size and flags) for XU controls. >> */ >> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, >> const struct uvc_control *ctrl, struct uvc_control_info *info) >> { >> u8 *data; >> + int flags; >> int ret; >> >> data = kmalloc(2, GFP_KERNEL); >> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, >> >> info->size = le16_to_cpup((__le16 *)data); >> >> - /* Query the control information (GET_INFO) */ >> - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, >> - info->selector, data, 1); >> - if (ret < 0) { >> + flags = uvc_ctrl_get_flags(dev, ctrl, info); >> + >> + if (flags < 0) { >> uvc_trace(UVC_TRACE_CONTROL, >> - "GET_INFO failed on control %pUl/%u (%d).\n", >> - info->entity, info->selector, ret); >> + "Failed to retrieve flags (%d).\n", ret); >> + ret = flags; >> goto done; >> } >> - >> - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX >> - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF >> - | (data[0] & UVC_CONTROL_CAP_GET ? >> - UVC_CTRL_FLAG_GET_CUR : 0) >> - | (data[0] & UVC_CONTROL_CAP_SET ? >> - UVC_CTRL_FLAG_SET_CUR : 0) >> - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? >> - UVC_CTRL_FLAG_AUTO_UPDATE : 0); >> + info->flags = flags; >> >> uvc_ctrl_fixup_xu_info(dev, ctrl, info); >> >> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> const struct uvc_control_info *info) >> { >> int ret = 0; >> + int flags = 0; >> >> ctrl->info = *info; >> INIT_LIST_HEAD(&ctrl->info.mappings); >> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> goto done; >> } >> >> + flags = uvc_ctrl_get_flags(dev, ctrl, info); >> + if (flags < 0) >> + uvc_trace(UVC_TRACE_CONTROL, >> + "Failed to retrieve flags (%d).\n", ret); >> + else >> + ctrl->info.flags = flags; >> + >> ctrl->initialized = 1; >> >> uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " >>
Hi Edgar, Thanks for addressing my concerns in this updated patch. On 12/10/17 08:54, Edgar Thier wrote: > > Use flags the device exposes for UVC controls. > This allows the device to define which property flags are set. > > Since some cameras offer auto-adjustments for properties (e.g. auto-gain), > the values of other properties (e.g. gain) can change in the camera. > Examining the flags ensures that the driver is aware of such properties. > > Signed-off-by: Edgar Thier <info@edgarthier.net> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++---------- > 1 file changed, 49 insertions(+), 15 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 20397aba..8f902a41 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, > } > } > > +/* > + * Retrieve flags for a given control > + */ > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, > + const struct uvc_control_info *info) > +{ > + u8 *data; > + int ret = 0; > + int flags = 0; > + > + data = kmalloc(2, GFP_KERNEL); > + if (data == NULL) > + return -ENOMEM; > + > + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > + info->selector, data, 1); > + if (ret < 0) { > + uvc_trace(UVC_TRACE_CONTROL, > + "GET_INFO failed on control %pUl/%u (%d).\n", > + info->entity, info->selector, ret); > + } else { > + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > + | (data[0] & UVC_CONTROL_CAP_GET ? > + UVC_CTRL_FLAG_GET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_SET ? > + UVC_CTRL_FLAG_SET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > + UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + } > + kfree(data); > + return flags; > +} > + > /* > * Query control information (size and flags) for XU controls. > */ > @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > const struct uvc_control *ctrl, struct uvc_control_info *info) > { > u8 *data; > + int flags; > int ret; > > data = kmalloc(2, GFP_KERNEL); > @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, > > info->size = le16_to_cpup((__le16 *)data); > > - /* Query the control information (GET_INFO) */ > - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > - info->selector, data, 1); > - if (ret < 0) { > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + > + if (flags < 0) { > uvc_trace(UVC_TRACE_CONTROL, > - "GET_INFO failed on control %pUl/%u (%d).\n", > - info->entity, info->selector, ret); > + "Failed to retrieve flags (%d).\n", ret); > + ret = flags; > goto done; > } > - > - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > - | (data[0] & UVC_CONTROL_CAP_GET ? > - UVC_CTRL_FLAG_GET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_SET ? > - UVC_CTRL_FLAG_SET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > - UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + info->flags = flags; > > uvc_ctrl_fixup_xu_info(dev, ctrl, info); > > @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > const struct uvc_control_info *info) > { > int ret = 0; > + int flags = 0; > > ctrl->info = *info; > INIT_LIST_HEAD(&ctrl->info.mappings); > @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, > goto done; > } > > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + if (flags < 0) > + uvc_trace(UVC_TRACE_CONTROL, > + "Failed to retrieve flags (%d).\n", ret); > + else > + ctrl->info.flags = flags; > + > ctrl->initialized = 1; > > uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " >
Hi Kieran, > I think it's easier to see updated patches if they are posted as a new thread, > with an increased version number. [PATCH v2], [PATCH v3] etc... > > Not a problem now - but might help your updated patches get seen next time. I will keep that in mind for next time. :) > Looks like my concerns were addressed, so that's a Reviewed-by: tag from me. Thanks! Regards, Edgar
Hi, Another month, another mail. Are there still issues keeping this from being merged? Regards, Edgar On 11/15/2017 12:54 PM, Kieran Bingham wrote: > Hi Edgar, > > Thanks for addressing my concerns in this updated patch. > > On 12/10/17 08:54, Edgar Thier wrote: >> >> Use flags the device exposes for UVC controls. >> This allows the device to define which property flags are set. >> >> Since some cameras offer auto-adjustments for properties (e.g. auto-gain), >> the values of other properties (e.g. gain) can change in the camera. >> Examining the flags ensures that the driver is aware of such properties. >> >> Signed-off-by: Edgar Thier <info@edgarthier.net> > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> --- >> drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++---------- >> 1 file changed, 49 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c >> index 20397aba..8f902a41 100644 >> --- a/drivers/media/usb/uvc/uvc_ctrl.c >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c >> @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, >> } >> } >> >> +/* >> + * Retrieve flags for a given control >> + */ >> +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, >> + const struct uvc_control_info *info) >> +{ >> + u8 *data; >> + int ret = 0; >> + int flags = 0; >> + >> + data = kmalloc(2, GFP_KERNEL); >> + if (data == NULL) >> + return -ENOMEM; >> + >> + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, >> + info->selector, data, 1); >> + if (ret < 0) { >> + uvc_trace(UVC_TRACE_CONTROL, >> + "GET_INFO failed on control %pUl/%u (%d).\n", >> + info->entity, info->selector, ret); >> + } else { >> + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX >> + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF >> + | (data[0] & UVC_CONTROL_CAP_GET ? >> + UVC_CTRL_FLAG_GET_CUR : 0) >> + | (data[0] & UVC_CONTROL_CAP_SET ? >> + UVC_CTRL_FLAG_SET_CUR : 0) >> + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? >> + UVC_CTRL_FLAG_AUTO_UPDATE : 0); >> + } >> + kfree(data); >> + return flags; >> +} >> + >> /* >> * Query control information (size and flags) for XU controls. >> */ >> @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, >> const struct uvc_control *ctrl, struct uvc_control_info *info) >> { >> u8 *data; >> + int flags; >> int ret; >> >> data = kmalloc(2, GFP_KERNEL); >> @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, >> >> info->size = le16_to_cpup((__le16 *)data); >> >> - /* Query the control information (GET_INFO) */ >> - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, >> - info->selector, data, 1); >> - if (ret < 0) { >> + flags = uvc_ctrl_get_flags(dev, ctrl, info); >> + >> + if (flags < 0) { >> uvc_trace(UVC_TRACE_CONTROL, >> - "GET_INFO failed on control %pUl/%u (%d).\n", >> - info->entity, info->selector, ret); >> + "Failed to retrieve flags (%d).\n", ret); >> + ret = flags; >> goto done; >> } >> - >> - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX >> - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF >> - | (data[0] & UVC_CONTROL_CAP_GET ? >> - UVC_CTRL_FLAG_GET_CUR : 0) >> - | (data[0] & UVC_CONTROL_CAP_SET ? >> - UVC_CTRL_FLAG_SET_CUR : 0) >> - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? >> - UVC_CTRL_FLAG_AUTO_UPDATE : 0); >> + info->flags = flags; >> >> uvc_ctrl_fixup_xu_info(dev, ctrl, info); >> >> @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> const struct uvc_control_info *info) >> { >> int ret = 0; >> + int flags = 0; >> >> ctrl->info = *info; >> INIT_LIST_HEAD(&ctrl->info.mappings); >> @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, >> goto done; >> } >> >> + flags = uvc_ctrl_get_flags(dev, ctrl, info); >> + if (flags < 0) >> + uvc_trace(UVC_TRACE_CONTROL, >> + "Failed to retrieve flags (%d).\n", ret); >> + else >> + ctrl->info.flags = flags; >> + >> ctrl->initialized = 1; >> >> uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " >>
Hi Edgar, Thank you for the patch. On Thursday, 12 October 2017 10:54:17 EET Edgar Thier wrote: > Use flags the device exposes for UVC controls. > This allows the device to define which property flags are set. > > Since some cameras offer auto-adjustments for properties (e.g. auto-gain), > the values of other properties (e.g. gain) can change in the camera. > Examining the flags ensures that the driver is aware of such properties. > > Signed-off-by: Edgar Thier <info@edgarthier.net> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 64 +++++++++++++++++++++++++++---------- > 1 file changed, 49 insertions(+), 15 deletions(-) Running scripts/checkpatch.pl on the patch produces the following warning and errors. WARNING: line over 80 characters #83: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1635: +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, ERROR: code indent should use tabs where possible #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: + ^I^Iret = flags;$ WARNING: please, no space before tabs #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: + ^I^Iret = flags;$ WARNING: please, no spaces at the start of a line #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: + ^I^Iret = flags;$ The last three should be fixed. The first one can sometimes be considered a matter of taste, but in this case it's easy to fix so we should address it too. > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c index 20397aba..8f902a41 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device > *dev, } > } > > +/* > + * Retrieve flags for a given control > + */ > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct > uvc_control *ctrl, > + const struct uvc_control_info *info) > +{ > + u8 *data; > + int ret = 0; No need to initialize ret to 0. > + int flags = 0; > + > + data = kmalloc(2, GFP_KERNEL); > + if (data == NULL) > + return -ENOMEM; > + > + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > + info->selector, data, 1); > + if (ret < 0) { > + uvc_trace(UVC_TRACE_CONTROL, > + "GET_INFO failed on control %pUl/%u (%d).\n", > + info->entity, info->selector, ret); > + } else { > + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > + | (data[0] & UVC_CONTROL_CAP_GET ? > + UVC_CTRL_FLAG_GET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_SET ? > + UVC_CTRL_FLAG_SET_CUR : 0) > + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > + UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + } > + kfree(data); > + return flags; So in case of failure you will return 0, and the caller will ignore the failure. This changes the behaviour of uvc_ctrl_fill_xu_info() which I don't think is a good idea. For uvc_ctrl_add_info(), on the other hand, ignoring errors is probably good, as otherwise we could introduce regressions with devices that don't implement GET_INFO properly on standard controls (the driver currently doesn't query information for standard controls so doesn't catch those devices). > +} > + > /* > * Query control information (size and flags) for XU controls. > */ > @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device > *dev, const struct uvc_control *ctrl, struct uvc_control_info *info) > { > u8 *data; > + int flags; > int ret; > > data = kmalloc(2, GFP_KERNEL); > @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device > *dev, > > info->size = le16_to_cpup((__le16 *)data); > > - /* Query the control information (GET_INFO) */ > - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > - info->selector, data, 1); > - if (ret < 0) { > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + No need for a blank line here. > + if (flags < 0) { > uvc_trace(UVC_TRACE_CONTROL, > - "GET_INFO failed on control %pUl/%u (%d).\n", > - info->entity, info->selector, ret); > + "Failed to retrieve flags (%d).\n", ret); > + ret = flags; > goto done; > } > - > - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > - | (data[0] & UVC_CONTROL_CAP_GET ? > - UVC_CTRL_FLAG_GET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_SET ? > - UVC_CTRL_FLAG_SET_CUR : 0) > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > - UVC_CTRL_FLAG_AUTO_UPDATE : 0); > + info->flags = flags; > > uvc_ctrl_fixup_xu_info(dev, ctrl, info); > > @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, > struct uvc_control *ctrl, const struct uvc_control_info *info) > { > int ret = 0; > + int flags = 0; There's no need to initialize the flags variable to 0. > > ctrl->info = *info; > INIT_LIST_HEAD(&ctrl->info.mappings); > @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, > struct uvc_control *ctrl, goto done; > } > > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > + if (flags < 0) > + uvc_trace(UVC_TRACE_CONTROL, > + "Failed to retrieve flags (%d).\n", ret); > + else > + ctrl->info.flags = flags; > + > ctrl->initialized = 1; > > uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " All these are small issues. Let me try to address them, I'll send you an updated patch shortly.
Hi Edgar, A few more comments. On Tuesday, 2 January 2018 22:07:24 EET Laurent Pinchart wrote: > On Thursday, 12 October 2017 10:54:17 EET Edgar Thier wrote: > > Use flags the device exposes for UVC controls. > > This allows the device to define which property flags are set. > > > > Since some cameras offer auto-adjustments for properties (e.g. auto-gain), > > the values of other properties (e.g. gain) can change in the camera. > > Examining the flags ensures that the driver is aware of such properties. > > > > Signed-off-by: Edgar Thier <info@edgarthier.net> > > --- > > > > drivers/media/usb/uvc/uvc_ctrl.c | 64 +++++++++++++++++++++++++---------- > > 1 file changed, 49 insertions(+), 15 deletions(-) > > Running scripts/checkpatch.pl on the patch produces the following warning > and errors. > > WARNING: line over 80 characters > #83: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1635: > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct > uvc_control *ctrl, > > ERROR: code indent should use tabs where possible > #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: > + ^I^Iret = flags;$ > > WARNING: please, no space before tabs > #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: > + ^I^Iret = flags;$ > > WARNING: please, no spaces at the start of a line > #140: FILE: drivers/media/usb/uvc/uvc_ctrl.c:1702: > + ^I^Iret = flags;$ > > The last three should be fixed. The first one can sometimes be considered a > matter of taste, but in this case it's easy to fix so we should address it > too. > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > > b/drivers/media/usb/uvc/uvc_ctrl.c index 20397aba..8f902a41 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct > > uvc_device > > *dev, } > > > > } > > > > +/* > > + * Retrieve flags for a given control > > + */ > > +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct > > uvc_control *ctrl, > > + const struct uvc_control_info *info) > > +{ > > + u8 *data; > > + int ret = 0; > > No need to initialize ret to 0. > > > + int flags = 0; > > + > > + data = kmalloc(2, GFP_KERNEL); Isn't 1 byte enough ? > > + if (data == NULL) > > + return -ENOMEM; > > + > > + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > > + info->selector, data, 1); Indentation is incorrect here. > > + if (ret < 0) { > > + uvc_trace(UVC_TRACE_CONTROL, > > + "GET_INFO failed on control %pUl/%u (%d).\n", > > + info->entity, info->selector, ret); And here too. > > + } else { > > + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > > + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > > + | (data[0] & UVC_CONTROL_CAP_GET ? > > + UVC_CTRL_FLAG_GET_CUR : 0) > > + | (data[0] & UVC_CONTROL_CAP_SET ? > > + UVC_CTRL_FLAG_SET_CUR : 0) > > + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > > + UVC_CTRL_FLAG_AUTO_UPDATE : 0); > > + } > > + kfree(data); > > + return flags; > > So in case of failure you will return 0, and the caller will ignore the > failure. This changes the behaviour of uvc_ctrl_fill_xu_info() which I don't > think is a good idea. For uvc_ctrl_add_info(), on the other hand, ignoring > errors is probably good, as otherwise we could introduce regressions with > devices that don't implement GET_INFO properly on standard controls (the > driver currently doesn't query information for standard controls so doesn't > catch those devices). > > > +} > > + > > /* > > * Query control information (size and flags) for XU controls. > > */ > > @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device > > *dev, const struct uvc_control *ctrl, struct uvc_control_info *info) > > { > > u8 *data; > > + int flags; > > int ret; > > > > data = kmalloc(2, GFP_KERNEL); > > @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device > > *dev, > > > > info->size = le16_to_cpup((__le16 *)data); > > > > - /* Query the control information (GET_INFO) */ > > - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, > > - info->selector, data, 1); > > - if (ret < 0) { > > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > > + > > No need for a blank line here. > > > + if (flags < 0) { > > uvc_trace(UVC_TRACE_CONTROL, > > - "GET_INFO failed on control %pUl/%u (%d).\n", > > - info->entity, info->selector, ret); > > + "Failed to retrieve flags (%d).\n", ret); > > + ret = flags; > > goto done; > > } > > - > > - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX > > - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF > > - | (data[0] & UVC_CONTROL_CAP_GET ? > > - UVC_CTRL_FLAG_GET_CUR : 0) > > - | (data[0] & UVC_CONTROL_CAP_SET ? > > - UVC_CTRL_FLAG_SET_CUR : 0) > > - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? > > - UVC_CTRL_FLAG_AUTO_UPDATE : 0); > > + info->flags = flags; > > > > uvc_ctrl_fixup_xu_info(dev, ctrl, info); > > > > @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, > > struct uvc_control *ctrl, const struct uvc_control_info *info) > > { > > int ret = 0; > > + int flags = 0; > > There's no need to initialize the flags variable to 0. > > > ctrl->info = *info; > > INIT_LIST_HEAD(&ctrl->info.mappings); > > > > @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device > > *dev, struct uvc_control *ctrl, > > goto done; > > } > > > > + flags = uvc_ctrl_get_flags(dev, ctrl, info); > > + if (flags < 0) > > + uvc_trace(UVC_TRACE_CONTROL, > > + "Failed to retrieve flags (%d).\n", ret); > > + else > > + ctrl->info.flags = flags; > > + > > ctrl->initialized = 1; > > > > uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s " > > All these are small issues. Let me try to address them, I'll send you an > updated patch shortly.
Hi Emmanuel, >>> + int flags = 0; >>> + >>> + data = kmalloc(2, GFP_KERNEL); > > Isn't 1 byte enough ? > To quote from Kieran further up this thread: >> kmalloc seems a bit of an overhead for 2 bytes (only one of which is used). >> Can this use local stack storage? >> >> (Laurent, looks like you originally wrote the code that did that, was there a >> reason for the kmalloc for 2 bytes?) > Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't DMA to > the stack - I hadn't realised the 'data' was being DMA'd .. >> >> All these are small issues. Let me try to address them, I'll send you an >> updated patch shortly. I'll be waiting. Regards, Edgar
Hi Edgar, On Wednesday, 3 January 2018 08:07:44 EET Edgar Thier wrote: > Hi Emmanuel, If we pick names randomly I'll call you David :-) > >>> + int flags = 0; > >>> + > >>> + data = kmalloc(2, GFP_KERNEL); > > > > Isn't 1 byte enough ? > > To quote from Kieran further up this thread: > > >> kmalloc seems a bit of an overhead for 2 bytes (only one of which is > >> used). Can this use local stack storage? > >> > >> (Laurent, looks like you originally wrote the code that did that, was > >> there a reason for the kmalloc for 2 bytes?) > > > > Aha - OK, Just spoke with Laurent and - yes this is needed, as we can't > > DMA to the stack - I hadn't realised the 'data' was being DMA'd .. I don't dispute the fact that we need to kmalloc the memory, but I think we only need to kmalloc one byte, not two. The existing 2 bytes allocation comes from the size of the GET_LEN reply. Now that you only issue a GET_INFO here, one byte is enough. > >> All these are small issues. Let me try to address them, I'll send you an > >> updated patch shortly. > > I'll be waiting.
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 20397aba..8f902a41 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1629,6 +1629,40 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev, } } +/* + * Retrieve flags for a given control + */ +static int uvc_ctrl_get_flags(struct uvc_device *dev, const struct uvc_control *ctrl, + const struct uvc_control_info *info) +{ + u8 *data; + int ret = 0; + int flags = 0; + + data = kmalloc(2, GFP_KERNEL); + if (data == NULL) + return -ENOMEM; + + ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, + info->selector, data, 1); + if (ret < 0) { + uvc_trace(UVC_TRACE_CONTROL, + "GET_INFO failed on control %pUl/%u (%d).\n", + info->entity, info->selector, ret); + } else { + flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX + | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF + | (data[0] & UVC_CONTROL_CAP_GET ? + UVC_CTRL_FLAG_GET_CUR : 0) + | (data[0] & UVC_CONTROL_CAP_SET ? + UVC_CTRL_FLAG_SET_CUR : 0) + | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? + UVC_CTRL_FLAG_AUTO_UPDATE : 0); + } + kfree(data); + return flags; +} + /* * Query control information (size and flags) for XU controls. */ @@ -1636,6 +1670,7 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, const struct uvc_control *ctrl, struct uvc_control_info *info) { u8 *data; + int flags; int ret; data = kmalloc(2, GFP_KERNEL); @@ -1659,24 +1694,15 @@ static int uvc_ctrl_fill_xu_info(struct uvc_device *dev, info->size = le16_to_cpup((__le16 *)data); - /* Query the control information (GET_INFO) */ - ret = uvc_query_ctrl(dev, UVC_GET_INFO, ctrl->entity->id, dev->intfnum, - info->selector, data, 1); - if (ret < 0) { + flags = uvc_ctrl_get_flags(dev, ctrl, info); + + if (flags < 0) { uvc_trace(UVC_TRACE_CONTROL, - "GET_INFO failed on control %pUl/%u (%d).\n", - info->entity, info->selector, ret); + "Failed to retrieve flags (%d).\n", ret); + ret = flags; goto done; } - - info->flags = UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX - | UVC_CTRL_FLAG_GET_RES | UVC_CTRL_FLAG_GET_DEF - | (data[0] & UVC_CONTROL_CAP_GET ? - UVC_CTRL_FLAG_GET_CUR : 0) - | (data[0] & UVC_CONTROL_CAP_SET ? - UVC_CTRL_FLAG_SET_CUR : 0) - | (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ? - UVC_CTRL_FLAG_AUTO_UPDATE : 0); + info->flags = flags; uvc_ctrl_fixup_xu_info(dev, ctrl, info); @@ -1890,6 +1916,7 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, const struct uvc_control_info *info) { int ret = 0; + int flags = 0; ctrl->info = *info; INIT_LIST_HEAD(&ctrl->info.mappings); @@ -1902,6 +1929,13 @@ static int uvc_ctrl_add_info(struct uvc_device *dev, struct uvc_control *ctrl, goto done; } + flags = uvc_ctrl_get_flags(dev, ctrl, info); + if (flags < 0) + uvc_trace(UVC_TRACE_CONTROL, + "Failed to retrieve flags (%d).\n", ret); + else + ctrl->info.flags = flags; + ctrl->initialized = 1; uvc_trace(UVC_TRACE_CONTROL, "Added control %pUl/%u to device %s "
Use flags the device exposes for UVC controls. This allows the device to define which property flags are set. Since some cameras offer auto-adjustments for properties (e.g. auto-gain), the values of other properties (e.g. gain) can change in the camera. Examining the flags ensures that the driver is aware of such properties. Signed-off-by: Edgar Thier <info@edgarthier.net> --- drivers/media/usb/uvc/uvc_ctrl.c | 64 ++++++++++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 15 deletions(-)