Message ID | 20221025184724.6170-1-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Commit | 5f0e659d2a2ac6172d495915f2775fa592c7ab17 |
Headers | show |
Series | media: uvcvideo: Factor out usb_string() calls | expand |
On 10/25/22 11:47, Laurent Pinchart wrote: > When parsing UVC descriptors to instantiate entity, the driver calls > usb_string() to retrieve the entity name from the device, and falls back > to a default name if the string can't be retrieved. This code pattern > occurs multiple times. Factor it out to a separate helper function. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Ricardo, Guenter, this applies on top of "media: uvcvideo: Handle errors > from calls to usb_string". Any opinion ? Great cleanup. Looks good to me. Reviewed-by: Guenter Roeck <linux@roeck-us.net> Guenter > --- > drivers/media/usb/uvc/uvc_driver.c | 59 ++++++++++++++++++------------ > 1 file changed, 35 insertions(+), 24 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index bd3716a359b0..6eb011f452e5 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -813,6 +813,27 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, > return entity; > } > > +static void uvc_entity_set_name(struct uvc_device *dev, struct uvc_entity *entity, > + const char *type_name, u8 string_id) > +{ > + int ret; > + > + /* > + * First attempt to read the entity name from the device. If the entity > + * has no associated string, or if reading the string fails (most > + * likely due to a buggy firmware), fall back to default names based on > + * the entity type. > + */ > + if (string_id) { > + ret = usb_string(dev->udev, string_id, entity->name, > + sizeof(entity->name)); > + if (!ret) > + return; > + } > + > + sprintf(entity->name, "%s %u", type_name, entity->id); > +} > + > /* Parse vendor-specific extensions. */ > static int uvc_parse_vendor_control(struct uvc_device *dev, > const unsigned char *buffer, int buflen) > @@ -879,9 +900,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev, > + n; > memcpy(unit->extension.bmControls, &buffer[23+p], 2*n); > > - if (buffer[24+p+2*n] == 0 || > - usb_string(udev, buffer[24+p+2*n], unit->name, sizeof(unit->name)) < 0) > - sprintf(unit->name, "Extension %u", buffer[3]); > + uvc_entity_set_name(dev, unit, "Extension", buffer[24+p+2*n]); > > list_add_tail(&unit->list, &dev->entities); > handled = 1; > @@ -899,6 +918,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > struct usb_interface *intf; > struct usb_host_interface *alts = dev->intf->cur_altsetting; > unsigned int i, n, p, len; > + const char *type_name; > u16 type; > > switch (buffer[2]) { > @@ -1004,15 +1024,14 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > memcpy(term->media.bmTransportModes, &buffer[10+n], p); > } > > - if (buffer[7] == 0 || > - usb_string(udev, buffer[7], term->name, sizeof(term->name)) < 0) { > - if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) > - sprintf(term->name, "Camera %u", buffer[3]); > - if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) > - sprintf(term->name, "Media %u", buffer[3]); > - else > - sprintf(term->name, "Input %u", buffer[3]); > - } > + if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) > + type_name = "Camera"; > + else if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) > + type_name = "Media"; > + else > + type_name = "Input"; > + > + uvc_entity_set_name(dev, term, type_name, buffer[7]); > > list_add_tail(&term->list, &dev->entities); > break; > @@ -1045,9 +1064,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > > memcpy(term->baSourceID, &buffer[7], 1); > > - if (buffer[8] == 0 || > - usb_string(udev, buffer[8], term->name, sizeof(term->name)) < 0) > - sprintf(term->name, "Output %u", buffer[3]); > + uvc_entity_set_name(dev, term, "Output", buffer[8]); > > list_add_tail(&term->list, &dev->entities); > break; > @@ -1068,9 +1085,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > > memcpy(unit->baSourceID, &buffer[5], p); > > - if (buffer[5+p] == 0 || > - usb_string(udev, buffer[5+p], unit->name, sizeof(unit->name)) < 0) > - sprintf(unit->name, "Selector %u", buffer[3]); > + uvc_entity_set_name(dev, unit, "Selector", buffer[5+p]); > > list_add_tail(&unit->list, &dev->entities); > break; > @@ -1099,9 +1114,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > if (dev->uvc_version >= 0x0110) > unit->processing.bmVideoStandards = buffer[9+n]; > > - if (buffer[8+n] == 0 || > - usb_string(udev, buffer[8+n], unit->name, sizeof(unit->name)) < 0) > - sprintf(unit->name, "Processing %u", buffer[3]); > + uvc_entity_set_name(dev, unit, "Processing", buffer[8+n]); > > list_add_tail(&unit->list, &dev->entities); > break; > @@ -1128,9 +1141,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, > unit->extension.bmControls = (u8 *)unit + sizeof(*unit); > memcpy(unit->extension.bmControls, &buffer[23+p], n); > > - if (buffer[23+p+n] == 0 || > - usb_string(udev, buffer[23+p+n], unit->name, sizeof(unit->name)) < 0) > - sprintf(unit->name, "Extension %u", buffer[3]); > + uvc_entity_set_name(dev, unit, "Extension", buffer[23+p+n]); > > list_add_tail(&unit->list, &dev->entities); > break;
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index bd3716a359b0..6eb011f452e5 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -813,6 +813,27 @@ static struct uvc_entity *uvc_alloc_entity(u16 type, u16 id, return entity; } +static void uvc_entity_set_name(struct uvc_device *dev, struct uvc_entity *entity, + const char *type_name, u8 string_id) +{ + int ret; + + /* + * First attempt to read the entity name from the device. If the entity + * has no associated string, or if reading the string fails (most + * likely due to a buggy firmware), fall back to default names based on + * the entity type. + */ + if (string_id) { + ret = usb_string(dev->udev, string_id, entity->name, + sizeof(entity->name)); + if (!ret) + return; + } + + sprintf(entity->name, "%s %u", type_name, entity->id); +} + /* Parse vendor-specific extensions. */ static int uvc_parse_vendor_control(struct uvc_device *dev, const unsigned char *buffer, int buflen) @@ -879,9 +900,7 @@ static int uvc_parse_vendor_control(struct uvc_device *dev, + n; memcpy(unit->extension.bmControls, &buffer[23+p], 2*n); - if (buffer[24+p+2*n] == 0 || - usb_string(udev, buffer[24+p+2*n], unit->name, sizeof(unit->name)) < 0) - sprintf(unit->name, "Extension %u", buffer[3]); + uvc_entity_set_name(dev, unit, "Extension", buffer[24+p+2*n]); list_add_tail(&unit->list, &dev->entities); handled = 1; @@ -899,6 +918,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, struct usb_interface *intf; struct usb_host_interface *alts = dev->intf->cur_altsetting; unsigned int i, n, p, len; + const char *type_name; u16 type; switch (buffer[2]) { @@ -1004,15 +1024,14 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(term->media.bmTransportModes, &buffer[10+n], p); } - if (buffer[7] == 0 || - usb_string(udev, buffer[7], term->name, sizeof(term->name)) < 0) { - if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) - sprintf(term->name, "Camera %u", buffer[3]); - if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) - sprintf(term->name, "Media %u", buffer[3]); - else - sprintf(term->name, "Input %u", buffer[3]); - } + if (UVC_ENTITY_TYPE(term) == UVC_ITT_CAMERA) + type_name = "Camera"; + else if (UVC_ENTITY_TYPE(term) == UVC_ITT_MEDIA_TRANSPORT_INPUT) + type_name = "Media"; + else + type_name = "Input"; + + uvc_entity_set_name(dev, term, type_name, buffer[7]); list_add_tail(&term->list, &dev->entities); break; @@ -1045,9 +1064,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(term->baSourceID, &buffer[7], 1); - if (buffer[8] == 0 || - usb_string(udev, buffer[8], term->name, sizeof(term->name)) < 0) - sprintf(term->name, "Output %u", buffer[3]); + uvc_entity_set_name(dev, term, "Output", buffer[8]); list_add_tail(&term->list, &dev->entities); break; @@ -1068,9 +1085,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, memcpy(unit->baSourceID, &buffer[5], p); - if (buffer[5+p] == 0 || - usb_string(udev, buffer[5+p], unit->name, sizeof(unit->name)) < 0) - sprintf(unit->name, "Selector %u", buffer[3]); + uvc_entity_set_name(dev, unit, "Selector", buffer[5+p]); list_add_tail(&unit->list, &dev->entities); break; @@ -1099,9 +1114,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, if (dev->uvc_version >= 0x0110) unit->processing.bmVideoStandards = buffer[9+n]; - if (buffer[8+n] == 0 || - usb_string(udev, buffer[8+n], unit->name, sizeof(unit->name)) < 0) - sprintf(unit->name, "Processing %u", buffer[3]); + uvc_entity_set_name(dev, unit, "Processing", buffer[8+n]); list_add_tail(&unit->list, &dev->entities); break; @@ -1128,9 +1141,7 @@ static int uvc_parse_standard_control(struct uvc_device *dev, unit->extension.bmControls = (u8 *)unit + sizeof(*unit); memcpy(unit->extension.bmControls, &buffer[23+p], n); - if (buffer[23+p+n] == 0 || - usb_string(udev, buffer[23+p+n], unit->name, sizeof(unit->name)) < 0) - sprintf(unit->name, "Extension %u", buffer[3]); + uvc_entity_set_name(dev, unit, "Extension", buffer[23+p+n]); list_add_tail(&unit->list, &dev->entities); break;
When parsing UVC descriptors to instantiate entity, the driver calls usb_string() to retrieve the entity name from the device, and falls back to a default name if the string can't be retrieved. This code pattern occurs multiple times. Factor it out to a separate helper function. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Ricardo, Guenter, this applies on top of "media: uvcvideo: Handle errors from calls to usb_string". Any opinion ? --- drivers/media/usb/uvc/uvc_driver.c | 59 ++++++++++++++++++------------ 1 file changed, 35 insertions(+), 24 deletions(-)