Message ID | 20231215191421.36686-1-dean@sensoray.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [PATCHv3] media: usb: s2255: add serial number V4L2_CID | expand |
Hi Dean, Thanks for the patch. On Fri, Dec 15, 2023 at 11:14:21AM -0800, Dean Anderson wrote: > Adding V4L2 read-only control id for serial number as hardware > does not support embedding the serial number in the USB device descriptors. > Comment added noting v4l2_ctrl_handler_setup is not needed for this driver. > > Signed-off-by: Dean Anderson <dean@sensoray.com> > > --- > drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++- > 1 file changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c > index 3c2627712fe9..1f3961835711 100644 > --- a/drivers/media/usb/s2255/s2255drv.c > +++ b/drivers/media/usb/s2255/s2255drv.c > @@ -60,6 +60,7 @@ > #define S2255_MIN_BUFS 2 > #define S2255_SETMODE_TIMEOUT 500 > #define S2255_VIDSTATUS_TIMEOUT 350 > +#define S2255_MARKER_FIRMWARE cpu_to_le32(0xDDCCBBAAL) It'd be nicer to convert the value obtained from the device to CPU endianness rather than the other way around. But as this seems to be a pattern already used in the driver, I'm fine with it here. Given that the serial number itself is big endian, I wonder if the driver is just implemented in a funny way and happens to work? > #define S2255_MARKER_FRAME cpu_to_le32(0x2255DA4AL) > #define S2255_MARKER_RESPONSE cpu_to_le32(0x2255ACACL) > #define S2255_RESPONSE_SETMODE cpu_to_le32(0x01) > @@ -323,6 +324,7 @@ struct s2255_buffer { > #define S2255_V4L2_YC_ON 1 > #define S2255_V4L2_YC_OFF 0 > #define V4L2_CID_S2255_COLORFILTER (V4L2_CID_USER_S2255_BASE + 0) > +#define V4L2_CID_S2255_SERIALNUM (V4L2_CID_USER_S2255_BASE + 1) I'd document a new standardised control for this purpose. Any other opinions? > > /* frame prefix size (sent once every frame) */ > #define PREFIX_SIZE 512 > @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) > return 0; > } > > +/* > + * serial number is not used in usb device descriptors. > + * returns serial number from device, 0 if none found. > + */ > + > +static int s2255_g_serialnum(struct s2255_dev *dev) > +{ > +#define S2255_SERIALNUM_NONE 0 No need for such a definition, just assign it to zero. > +#define S2255_I2C_SIZE 16 > +#define S2255_I2C_SERIALNUM 0xa2 > +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 > +#define S2255_VENDOR_READREG 0x22 > + > + u8 *buf; > + int serialnum = S2255_SERIALNUM_NONE; u32? > + > + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); Could this reside in the stack instead? It's just 16 bytes. > + if (!buf) > + return serialnum; > + > + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET, > + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); It'd be nice to check for errors here. > + > + /* verify marker code */ > + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) > + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15]; Maybe: serialnum = be32_to_cpu(*(__be32 *)(buf + 12)); You could add a reference to the serial number as a function argument and just return the error if one happens. This isn't exactly the same thing as having serial number 0. > + kfree(buf); > + return serialnum; > +} > + > static int vidioc_g_jpegcomp(struct file *file, void *priv, > struct v4l2_jpegcompression *jc) > { > @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { > .def = 1, > }; > > +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { > + .ops = &s2255_ctrl_ops, > + .name = "Serial Number", > + .id = V4L2_CID_S2255_SERIALNUM, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .max = 0x7fffffff, > + .min = 0, > + .step = 1, > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > +}; > + > static int s2255_probe_v4l(struct s2255_dev *dev) > { > int ret; > @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > int cur_nr = video_nr; > struct s2255_vc *vc; > struct vb2_queue *q; > + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; > > ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); > if (ret) > @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > vc = &dev->vc[i]; > INIT_LIST_HEAD(&vc->buf_list); > > - v4l2_ctrl_handler_init(&vc->hdl, 6); > + v4l2_ctrl_handler_init(&vc->hdl, 7); > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > (dev->pid != 0x2257 || vc->idx <= 1)) > v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, > NULL); > + tmp.def = s2255_g_serialnum(dev); > + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); > if (vc->hdl.error) { > ret = vc->hdl.error; > v4l2_ctrl_handler_free(&vc->hdl); > @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface, > retval = s2255_board_init(dev); > if (retval) > goto errorBOARDINIT; > + /* > + * v4l2_ctrl_handler_setup is not required. > + * V4L2 controls initialized when firmware is loaded. > + */ I'd drop the comment. > s2255_fwload_start(dev); > /* loads v4l specific */ > retval = s2255_probe_v4l(dev);
On Mon, Jan 08, 2024 at 12:52:44PM +0000, Sakari Ailus wrote: > Hi Dean, > > Thanks for the patch. > > On Fri, Dec 15, 2023 at 11:14:21AM -0800, Dean Anderson wrote: > > Adding V4L2 read-only control id for serial number as hardware > > does not support embedding the serial number in the USB device descriptors. > > Comment added noting v4l2_ctrl_handler_setup is not needed for this driver. > > > > Signed-off-by: Dean Anderson <dean@sensoray.com> > > > > --- > > drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++- > > 1 file changed, 51 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c > > index 3c2627712fe9..1f3961835711 100644 > > --- a/drivers/media/usb/s2255/s2255drv.c > > +++ b/drivers/media/usb/s2255/s2255drv.c > > @@ -60,6 +60,7 @@ > > #define S2255_MIN_BUFS 2 > > #define S2255_SETMODE_TIMEOUT 500 > > #define S2255_VIDSTATUS_TIMEOUT 350 > > +#define S2255_MARKER_FIRMWARE cpu_to_le32(0xDDCCBBAAL) > > It'd be nicer to convert the value obtained from the device to CPU > endianness rather than the other way around. But as this seems to be a > pattern already used in the driver, I'm fine with it here. > > Given that the serial number itself is big endian, I wonder if the driver > is just implemented in a funny way and happens to work? > > > #define S2255_MARKER_FRAME cpu_to_le32(0x2255DA4AL) > > #define S2255_MARKER_RESPONSE cpu_to_le32(0x2255ACACL) > > #define S2255_RESPONSE_SETMODE cpu_to_le32(0x01) > > @@ -323,6 +324,7 @@ struct s2255_buffer { > > #define S2255_V4L2_YC_ON 1 > > #define S2255_V4L2_YC_OFF 0 > > #define V4L2_CID_S2255_COLORFILTER (V4L2_CID_USER_S2255_BASE + 0) > > +#define V4L2_CID_S2255_SERIALNUM (V4L2_CID_USER_S2255_BASE + 1) > > I'd document a new standardised control for this purpose. Any other > opinions? How about reporting the serial number through struct media_device_info.serial instead ? > > > > /* frame prefix size (sent once every frame) */ > > #define PREFIX_SIZE 512 > > @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) > > return 0; > > } > > > > +/* > > + * serial number is not used in usb device descriptors. > > + * returns serial number from device, 0 if none found. > > + */ > > + > > +static int s2255_g_serialnum(struct s2255_dev *dev) > > +{ > > +#define S2255_SERIALNUM_NONE 0 > > No need for such a definition, just assign it to zero. > > > +#define S2255_I2C_SIZE 16 > > +#define S2255_I2C_SERIALNUM 0xa2 > > +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 > > +#define S2255_VENDOR_READREG 0x22 > > + > > + u8 *buf; > > + int serialnum = S2255_SERIALNUM_NONE; > > u32? > > > + > > + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); > > Could this reside in the stack instead? It's just 16 bytes. > > > + if (!buf) > > + return serialnum; > > + > > + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET, > > + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); > > It'd be nice to check for errors here. > > > + > > + /* verify marker code */ > > + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) > > + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15]; > > Maybe: > > serialnum = be32_to_cpu(*(__be32 *)(buf + 12)); > > You could add a reference to the serial number as a function argument and > just return the error if one happens. This isn't exactly the same thing as > having serial number 0. > > > + kfree(buf); > > + return serialnum; > > +} > > + > > static int vidioc_g_jpegcomp(struct file *file, void *priv, > > struct v4l2_jpegcompression *jc) > > { > > @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { > > .def = 1, > > }; > > > > +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { > > + .ops = &s2255_ctrl_ops, > > + .name = "Serial Number", > > + .id = V4L2_CID_S2255_SERIALNUM, > > + .type = V4L2_CTRL_TYPE_INTEGER, > > + .max = 0x7fffffff, > > + .min = 0, > > + .step = 1, > > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > > +}; > > + > > static int s2255_probe_v4l(struct s2255_dev *dev) > > { > > int ret; > > @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > > int cur_nr = video_nr; > > struct s2255_vc *vc; > > struct vb2_queue *q; > > + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; > > > > ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); > > if (ret) > > @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > > vc = &dev->vc[i]; > > INIT_LIST_HEAD(&vc->buf_list); > > > > - v4l2_ctrl_handler_init(&vc->hdl, 6); > > + v4l2_ctrl_handler_init(&vc->hdl, 7); > > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > > V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); > > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > > @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > > (dev->pid != 0x2257 || vc->idx <= 1)) > > v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, > > NULL); > > + tmp.def = s2255_g_serialnum(dev); > > + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); > > if (vc->hdl.error) { > > ret = vc->hdl.error; > > v4l2_ctrl_handler_free(&vc->hdl); > > @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface, > > retval = s2255_board_init(dev); > > if (retval) > > goto errorBOARDINIT; > > + /* > > + * v4l2_ctrl_handler_setup is not required. > > + * V4L2 controls initialized when firmware is loaded. > > + */ > > I'd drop the comment. > > > s2255_fwload_start(dev); > > /* loads v4l specific */ > > retval = s2255_probe_v4l(dev);
On 2024-01-08 07:25, Laurent Pinchart wrote: > On Mon, Jan 08, 2024 at 12:52:44PM +0000, Sakari Ailus wrote: >> Hi Dean, >> >> Thanks for the patch. >> >> On Fri, Dec 15, 2023 at 11:14:21AM -0800, Dean Anderson wrote: >> > Adding V4L2 read-only control id for serial number as hardware >> > does not support embedding the serial number in the USB device descriptors. >> > Comment added noting v4l2_ctrl_handler_setup is not needed for this driver. >> > >> > Signed-off-by: Dean Anderson <dean@sensoray.com> >> > >> > --- >> > drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++- >> > 1 file changed, 51 insertions(+), 1 deletion(-) >> > >> > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c >> > index 3c2627712fe9..1f3961835711 100644 >> > --- a/drivers/media/usb/s2255/s2255drv.c >> > +++ b/drivers/media/usb/s2255/s2255drv.c >> >> I'd document a new standardised control for this purpose. Any other >> opinions? > > How about reporting the serial number through struct > media_device_info.serial instead ? This is a great idea. My only concern is the CONFIG_MEDIA_CONTROLLER requirement, but most distros have that defined. I'll re-implement this patch with media_device_info. FYI, visl-core.c is missing several CONFIG_MEDIA_CONTROLLER ifdefs for media_device calls. > >> > >> > /* frame prefix size (sent once every frame) */ >> > #define PREFIX_SIZE 512 >> > @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> > return 0; >> > } >> > >> > +/* >> > + * serial number is not used in usb device descriptors. >> > + * returns serial number from device, 0 if none found. >> > + */ >> > + >> > +static int s2255_g_serialnum(struct s2255_dev *dev) >> > +{ >> > +#define S2255_SERIALNUM_NONE 0 >> >> No need for such a definition, just assign it to zero. >> >> > +#define S2255_I2C_SIZE 16 >> > +#define S2255_I2C_SERIALNUM 0xa2 >> > +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 >> > +#define S2255_VENDOR_READREG 0x22 >> > + >> > + u8 *buf; >> > + int serialnum = S2255_SERIALNUM_NONE; >> >> u32? >> >> > + >> > + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); >> >> Could this reside in the stack instead? It's just 16 bytes. >> >> > + if (!buf) >> > + return serialnum; >> > + >> > + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET, >> > + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); >> >> It'd be nice to check for errors here. >> >> > + >> > + /* verify marker code */ >> > + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) >> > + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15]; >> >> Maybe: >> >> serialnum = be32_to_cpu(*(__be32 *)(buf + 12)); yes >> >> You could add a reference to the serial number as a function argument >> and >> just return the error if one happens. This isn't exactly the same >> thing as >> having serial number 0. >> >> > + kfree(buf); >> > + return serialnum; >> > +} >> > + >> > static int vidioc_g_jpegcomp(struct file *file, void *priv, >> > struct v4l2_jpegcompression *jc) >> > { >> > @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { >> > .def = 1, >> > }; >> > >> > +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { >> > + .ops = &s2255_ctrl_ops, >> > + .name = "Serial Number", >> > + .id = V4L2_CID_S2255_SERIALNUM, >> > + .type = V4L2_CTRL_TYPE_INTEGER, >> > + .max = 0x7fffffff, >> > + .min = 0, >> > + .step = 1, >> > + .flags = V4L2_CTRL_FLAG_READ_ONLY, >> > +}; >> > + >> > static int s2255_probe_v4l(struct s2255_dev *dev) >> > { >> > int ret; >> > @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) >> > int cur_nr = video_nr; >> > struct s2255_vc *vc; >> > struct vb2_queue *q; >> > + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; >> > >> > ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); >> > if (ret) >> > @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) >> > vc = &dev->vc[i]; >> > INIT_LIST_HEAD(&vc->buf_list); >> > >> > - v4l2_ctrl_handler_init(&vc->hdl, 6); >> > + v4l2_ctrl_handler_init(&vc->hdl, 7); >> > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, >> > V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); >> > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, >> > @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev *dev) >> > (dev->pid != 0x2257 || vc->idx <= 1)) >> > v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, >> > NULL); >> > + tmp.def = s2255_g_serialnum(dev); >> > + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); >> > if (vc->hdl.error) { >> > ret = vc->hdl.error; >> > v4l2_ctrl_handler_free(&vc->hdl); >> > @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface, >> > retval = s2255_board_init(dev); >> > if (retval) >> > goto errorBOARDINIT; >> > + /* >> > + * v4l2_ctrl_handler_setup is not required. >> > + * V4L2 controls initialized when firmware is loaded. >> > + */ >> >> I'd drop the comment. >> >> > s2255_fwload_start(dev); >> > /* loads v4l specific */ >> > retval = s2255_probe_v4l(dev); > > -- > Regards, > > Laurent Pinchart Regards, Dean
Hi Dean, On Mon, Jan 08, 2024 at 04:49:21PM -0600, dean@sensoray.com wrote: > On 2024-01-08 07:25, Laurent Pinchart wrote: > > On Mon, Jan 08, 2024 at 12:52:44PM +0000, Sakari Ailus wrote: > >> On Fri, Dec 15, 2023 at 11:14:21AM -0800, Dean Anderson wrote: > >> > Adding V4L2 read-only control id for serial number as hardware > >> > does not support embedding the serial number in the USB device descriptors. > >> > Comment added noting v4l2_ctrl_handler_setup is not needed for this driver. > >> > > >> > Signed-off-by: Dean Anderson <dean@sensoray.com> > >> > > >> > --- > >> > drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++- > >> > 1 file changed, 51 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c > >> > index 3c2627712fe9..1f3961835711 100644 > >> > --- a/drivers/media/usb/s2255/s2255drv.c > >> > +++ b/drivers/media/usb/s2255/s2255drv.c > >> > >> I'd document a new standardised control for this purpose. Any other > >> opinions? > > > > How about reporting the serial number through struct > > media_device_info.serial instead ? > > This is a great idea. My only concern is the CONFIG_MEDIA_CONTROLLER > requirement, but most distros have that defined. I'll re-implement this > patch with media_device_info. > > FYI, visl-core.c is missing several CONFIG_MEDIA_CONTROLLER ifdefs for > media_device calls. VIDEO_VISL select MEDIA_CONTROLLER, so the driver shouldn't need #ifdef's. > >> > /* frame prefix size (sent once every frame) */ > >> > #define PREFIX_SIZE 512 > >> > @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) > >> > return 0; > >> > } > >> > > >> > +/* > >> > + * serial number is not used in usb device descriptors. > >> > + * returns serial number from device, 0 if none found. > >> > + */ > >> > + > >> > +static int s2255_g_serialnum(struct s2255_dev *dev) > >> > +{ > >> > +#define S2255_SERIALNUM_NONE 0 > >> > >> No need for such a definition, just assign it to zero. > >> > >> > +#define S2255_I2C_SIZE 16 > >> > +#define S2255_I2C_SERIALNUM 0xa2 > >> > +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 > >> > +#define S2255_VENDOR_READREG 0x22 > >> > + > >> > + u8 *buf; > >> > + int serialnum = S2255_SERIALNUM_NONE; > >> > >> u32? > >> > >> > + > >> > + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); > >> > >> Could this reside in the stack instead? It's just 16 bytes. > >> > >> > + if (!buf) > >> > + return serialnum; > >> > + > >> > + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET, > >> > + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); > >> > >> It'd be nice to check for errors here. > >> > >> > + > >> > + /* verify marker code */ > >> > + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) > >> > + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15]; > >> > >> Maybe: > >> > >> serialnum = be32_to_cpu(*(__be32 *)(buf + 12)); > > yes > > >> You could add a reference to the serial number as a function argument and > >> just return the error if one happens. This isn't exactly the same thing as > >> having serial number 0. > >> > >> > + kfree(buf); > >> > + return serialnum; > >> > +} > >> > + > >> > static int vidioc_g_jpegcomp(struct file *file, void *priv, > >> > struct v4l2_jpegcompression *jc) > >> > { > >> > @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { > >> > .def = 1, > >> > }; > >> > > >> > +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { > >> > + .ops = &s2255_ctrl_ops, > >> > + .name = "Serial Number", > >> > + .id = V4L2_CID_S2255_SERIALNUM, > >> > + .type = V4L2_CTRL_TYPE_INTEGER, > >> > + .max = 0x7fffffff, > >> > + .min = 0, > >> > + .step = 1, > >> > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > >> > +}; > >> > + > >> > static int s2255_probe_v4l(struct s2255_dev *dev) > >> > { > >> > int ret; > >> > @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > >> > int cur_nr = video_nr; > >> > struct s2255_vc *vc; > >> > struct vb2_queue *q; > >> > + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; > >> > > >> > ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); > >> > if (ret) > >> > @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > >> > vc = &dev->vc[i]; > >> > INIT_LIST_HEAD(&vc->buf_list); > >> > > >> > - v4l2_ctrl_handler_init(&vc->hdl, 6); > >> > + v4l2_ctrl_handler_init(&vc->hdl, 7); > >> > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > >> > V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); > >> > v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, > >> > @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev *dev) > >> > (dev->pid != 0x2257 || vc->idx <= 1)) > >> > v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, > >> > NULL); > >> > + tmp.def = s2255_g_serialnum(dev); > >> > + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); > >> > if (vc->hdl.error) { > >> > ret = vc->hdl.error; > >> > v4l2_ctrl_handler_free(&vc->hdl); > >> > @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface, > >> > retval = s2255_board_init(dev); > >> > if (retval) > >> > goto errorBOARDINIT; > >> > + /* > >> > + * v4l2_ctrl_handler_setup is not required. > >> > + * V4L2 controls initialized when firmware is loaded. > >> > + */ > >> > >> I'd drop the comment. > >> > >> > s2255_fwload_start(dev); > >> > /* loads v4l specific */ > >> > retval = s2255_probe_v4l(dev);
On 2024-01-08 06:52, Sakari Ailus wrote: > Hi Dean, > > Thanks for the patch. > > On Fri, Dec 15, 2023 at 11:14:21AM -0800, Dean Anderson wrote: >> Adding V4L2 read-only control id for serial number as hardware >> does not support embedding the serial number in the USB device >> descriptors. >> Comment added noting v4l2_ctrl_handler_setup is not needed for this >> driver. >> >> Signed-off-by: Dean Anderson <dean@sensoray.com> >> >> --- >> drivers/media/usb/s2255/s2255drv.c | 52 >> +++++++++++++++++++++++++++++- >> 1 file changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/usb/s2255/s2255drv.c >> b/drivers/media/usb/s2255/s2255drv.c >> index 3c2627712fe9..1f3961835711 100644 >> --- a/drivers/media/usb/s2255/s2255drv.c >> +++ b/drivers/media/usb/s2255/s2255drv.c >> @@ -60,6 +60,7 @@ >> #define S2255_MIN_BUFS 2 >> #define S2255_SETMODE_TIMEOUT 500 >> #define S2255_VIDSTATUS_TIMEOUT 350 >> +#define S2255_MARKER_FIRMWARE cpu_to_le32(0xDDCCBBAAL) > > It'd be nicer to convert the value obtained from the device to CPU > endianness rather than the other way around. But as this seems to be a > pattern already used in the driver, I'm fine with it here. > > Given that the serial number itself is big endian, I wonder if the > driver > is just implemented in a funny way and happens to work? Good catch. The serial number is the exception. I'll add a comment on it. It's stored big endian in an eeprom and sent byte by byte, but the other values are all little endian in the USB transfers. >> #define S2255_MARKER_FRAME cpu_to_le32(0x2255DA4AL) >> #define S2255_MARKER_RESPONSE cpu_to_le32(0x2255ACACL) >> #define S2255_RESPONSE_SETMODE cpu_to_le32(0x01) >> @@ -323,6 +324,7 @@ struct s2255_buffer { >> #define S2255_V4L2_YC_ON 1 >> #define S2255_V4L2_YC_OFF 0 >> #define V4L2_CID_S2255_COLORFILTER (V4L2_CID_USER_S2255_BASE + 0) >> +#define V4L2_CID_S2255_SERIALNUM (V4L2_CID_USER_S2255_BASE + 1) > > I'd document a new standardised control for this purpose. Any other > opinions? > >> >> /* frame prefix size (sent once every frame) */ >> #define PREFIX_SIZE 512 >> @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +/* >> + * serial number is not used in usb device descriptors. >> + * returns serial number from device, 0 if none found. >> + */ >> + >> +static int s2255_g_serialnum(struct s2255_dev *dev) >> +{ >> +#define S2255_SERIALNUM_NONE 0 > > No need for such a definition, just assign it to zero. > >> +#define S2255_I2C_SIZE 16 >> +#define S2255_I2C_SERIALNUM 0xa2 >> +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 >> +#define S2255_VENDOR_READREG 0x22 >> + >> + u8 *buf; >> + int serialnum = S2255_SERIALNUM_NONE; > > u32? > >> + >> + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); > > Could this reside in the stack instead? It's just 16 bytes. > >> + if (!buf) >> + return serialnum; >> + >> + s2255_vendor_req(dev, S2255_VENDOR_READREG, >> S2255_I2C_SERIALNUM_OFFSET, >> + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); > > It'd be nice to check for errors here. > >> + >> + /* verify marker code */ >> + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) >> + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + >> buf[15]; > > Maybe: > > serialnum = be32_to_cpu(*(__be32 *)(buf + 12)); > > You could add a reference to the serial number as a function argument > and > just return the error if one happens. This isn't exactly the same thing > as > having serial number 0. > >> + kfree(buf); >> + return serialnum; >> +} >> + >> static int vidioc_g_jpegcomp(struct file *file, void *priv, >> struct v4l2_jpegcompression *jc) >> { >> @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config >> color_filter_ctrl = { >> .def = 1, >> }; >> >> +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { >> + .ops = &s2255_ctrl_ops, >> + .name = "Serial Number", >> + .id = V4L2_CID_S2255_SERIALNUM, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .max = 0x7fffffff, >> + .min = 0, >> + .step = 1, >> + .flags = V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> static int s2255_probe_v4l(struct s2255_dev *dev) >> { >> int ret; >> @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev >> *dev) >> int cur_nr = video_nr; >> struct s2255_vc *vc; >> struct vb2_queue *q; >> + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; >> >> ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); >> if (ret) >> @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev >> *dev) >> vc = &dev->vc[i]; >> INIT_LIST_HEAD(&vc->buf_list); >> >> - v4l2_ctrl_handler_init(&vc->hdl, 6); >> + v4l2_ctrl_handler_init(&vc->hdl, 7); >> v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, >> V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); >> v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, >> @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev >> *dev) >> (dev->pid != 0x2257 || vc->idx <= 1)) >> v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, >> NULL); >> + tmp.def = s2255_g_serialnum(dev); >> + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); >> if (vc->hdl.error) { >> ret = vc->hdl.error; >> v4l2_ctrl_handler_free(&vc->hdl); >> @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface >> *interface, >> retval = s2255_board_init(dev); >> if (retval) >> goto errorBOARDINIT; >> + /* >> + * v4l2_ctrl_handler_setup is not required. >> + * V4L2 controls initialized when firmware is loaded. >> + */ > > I'd drop the comment. > >> s2255_fwload_start(dev); >> /* loads v4l specific */ >> retval = s2255_probe_v4l(dev); > > -- > Regards, > > Sakari Ailus
diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c index 3c2627712fe9..1f3961835711 100644 --- a/drivers/media/usb/s2255/s2255drv.c +++ b/drivers/media/usb/s2255/s2255drv.c @@ -60,6 +60,7 @@ #define S2255_MIN_BUFS 2 #define S2255_SETMODE_TIMEOUT 500 #define S2255_VIDSTATUS_TIMEOUT 350 +#define S2255_MARKER_FIRMWARE cpu_to_le32(0xDDCCBBAAL) #define S2255_MARKER_FRAME cpu_to_le32(0x2255DA4AL) #define S2255_MARKER_RESPONSE cpu_to_le32(0x2255ACACL) #define S2255_RESPONSE_SETMODE cpu_to_le32(0x01) @@ -323,6 +324,7 @@ struct s2255_buffer { #define S2255_V4L2_YC_ON 1 #define S2255_V4L2_YC_OFF 0 #define V4L2_CID_S2255_COLORFILTER (V4L2_CID_USER_S2255_BASE + 0) +#define V4L2_CID_S2255_SERIALNUM (V4L2_CID_USER_S2255_BASE + 1) /* frame prefix size (sent once every frame) */ #define PREFIX_SIZE 512 @@ -1232,6 +1234,36 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) return 0; } +/* + * serial number is not used in usb device descriptors. + * returns serial number from device, 0 if none found. + */ + +static int s2255_g_serialnum(struct s2255_dev *dev) +{ +#define S2255_SERIALNUM_NONE 0 +#define S2255_I2C_SIZE 16 +#define S2255_I2C_SERIALNUM 0xa2 +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0 +#define S2255_VENDOR_READREG 0x22 + + u8 *buf; + int serialnum = S2255_SERIALNUM_NONE; + + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL); + if (!buf) + return serialnum; + + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET, + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0); + + /* verify marker code */ + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE) + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15]; + kfree(buf); + return serialnum; +} + static int vidioc_g_jpegcomp(struct file *file, void *priv, struct v4l2_jpegcompression *jc) { @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { .def = 1, }; +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = { + .ops = &s2255_ctrl_ops, + .name = "Serial Number", + .id = V4L2_CID_S2255_SERIALNUM, + .type = V4L2_CTRL_TYPE_INTEGER, + .max = 0x7fffffff, + .min = 0, + .step = 1, + .flags = V4L2_CTRL_FLAG_READ_ONLY, +}; + static int s2255_probe_v4l(struct s2255_dev *dev) { int ret; @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) int cur_nr = video_nr; struct s2255_vc *vc; struct vb2_queue *q; + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum; ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev); if (ret) @@ -1598,7 +1642,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev) vc = &dev->vc[i]; INIT_LIST_HEAD(&vc->buf_list); - v4l2_ctrl_handler_init(&vc->hdl, 6); + v4l2_ctrl_handler_init(&vc->hdl, 7); v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, V4L2_CID_BRIGHTNESS, -127, 127, 1, DEF_BRIGHT); v4l2_ctrl_new_std(&vc->hdl, &s2255_ctrl_ops, @@ -1615,6 +1659,8 @@ static int s2255_probe_v4l(struct s2255_dev *dev) (dev->pid != 0x2257 || vc->idx <= 1)) v4l2_ctrl_new_custom(&vc->hdl, &color_filter_ctrl, NULL); + tmp.def = s2255_g_serialnum(dev); + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL); if (vc->hdl.error) { ret = vc->hdl.error; v4l2_ctrl_handler_free(&vc->hdl); @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface, retval = s2255_board_init(dev); if (retval) goto errorBOARDINIT; + /* + * v4l2_ctrl_handler_setup is not required. + * V4L2 controls initialized when firmware is loaded. + */ s2255_fwload_start(dev); /* loads v4l specific */ retval = s2255_probe_v4l(dev);
Adding V4L2 read-only control id for serial number as hardware does not support embedding the serial number in the USB device descriptors. Comment added noting v4l2_ctrl_handler_setup is not needed for this driver. Signed-off-by: Dean Anderson <dean@sensoray.com> --- drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-)