| Message ID | 20231122194838.13285-1-dean@sensoray.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: usb: s2255: device-id custom V4L2_CID | expand |
Hi Dean, Some comments below: On 22/11/2023 20:48, Dean Anderson wrote: > Adding V4L2 read-only control id for device id as hardware > does not support embedding the device-id in the USB device descriptors. > > base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 Just drop this line, it doesn't belong in a commit message. > > Signed-off-by: Dean Anderson <dean@sensoray.com> > > --- > drivers/media/usb/s2255/s2255drv.c | 49 +++++++++++++++++++++++++++++- > 1 file changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c > index 3c2627712fe9..c2248bbc7840 100644 > --- a/drivers/media/usb/s2255/s2255drv.c > +++ b/drivers/media/usb/s2255/s2255drv.c > @@ -40,7 +40,7 @@ > #include <media/v4l2-ctrls.h> > #include <media/v4l2-event.h> > > -#define S2255_VERSION "1.25.1" > +#define S2255_VERSION "1.26.1" > #define FIRMWARE_FILE_NAME "f2255usb.bin" > > /* default JPEG quality */ > @@ -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_DEVICEID (V4L2_CID_USER_S2255_BASE + 1) > > /* frame prefix size (sent once every frame) */ > #define PREFIX_SIZE 512 > @@ -1232,6 +1234,37 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) > return 0; > } > > +/* deviceid/serial number is not used in usb device descriptors. > + * returns device-id/serial number from device, 0 if none found. Please put the '/*' on a line by itself, that's consistent with the coding style for multi-line comments. Also run the patch through 'checkpatch.pl --strict'. I get several warnings. returns -> Returns One question about this comment: is the Device ID the same as a serial number? "Device ID" can mean either the ID of a device model, or a unique ID for each device. If it is the latter, should it perhaps be called "Device S/N" or just "Serial Number"? > + */ > +#define S2255_DEVICEID_NONE 0 > +static int s2255_g_deviceid(struct s2255_dev *dev) > +{ > + u8 *outbuf; > + int rc; > + int deviceid = S2255_DEVICEID_NONE; > +#define S2255_I2C_SIZE 16 > + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); Drop the "* sizeof(u8)", it serves no purpose. > + if (outbuf == NULL) > + return deviceid; > +#define S2255_I2C_SNDEV 0xa2 > +#define S2255_I2C_SNOFFSET 0x1ff0 > +#define S2255_USBVENDOR_READREG 0x22 > + rc = s2255_vendor_req(dev, S2255_USBVENDOR_READREG, S2255_I2C_SNOFFSET, > + S2255_I2C_SNDEV >> 1, outbuf, S2255_I2C_SIZE, 0); > + if (rc < 0) > + goto exit_g_deviceid; > + > + /* verify marker code */ > + if (*(__le32 *)outbuf != S2255_MARKER_FIRMWARE) > + goto exit_g_deviceid; > + > + deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; > +exit_g_deviceid: > + kfree(outbuf); > + return deviceid; This is overly complicated. How about this: /* verify marker code */ if (*(__le32 *)outbuf == S2255_MARKER_FIRMWARE) deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; kfree(outbuf); return deviceid; > +} > + > static int vidioc_g_jpegcomp(struct file *file, void *priv, > struct v4l2_jpegcompression *jc) > { > @@ -1581,6 +1614,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { > .def = 1, > }; > > +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { > + .ops = &s2255_ctrl_ops, > + .name = "Device ID", > + .id = V4L2_CID_S2255_DEVICEID, > + .type = V4L2_CTRL_TYPE_INTEGER, Please note that TYPE_INTEGER is a signed integer. If you need an unsigned integer, then use TYPE_U32. > + .max = 0xffffffff, > + .min = 0, > + .step = 1, > + .flags = V4L2_CTRL_FLAG_READ_ONLY, > +}; > + > static int s2255_probe_v4l(struct s2255_dev *dev) > { > int ret; > @@ -1615,6 +1659,9 @@ 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); > + v4l2_ctrl_deviceid.def = s2255_g_deviceid(dev); > + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, > + NULL); > if (vc->hdl.error) { > ret = vc->hdl.error; > v4l2_ctrl_handler_free(&vc->hdl); Regards, Hans
> Also run the patch through 'checkpatch.pl --strict'. Thanks for the feedback. I missed the --strict for checkpatch. Perhaps it should be the default setting? > One question about this comment: is the Device ID the same as a serial > number? Yes, it's the serial number, unique to each board. I'll make the revisions and re-submit. Regards, Dean On 2023-12-06 03:43, Hans Verkuil wrote: > Hi Dean, > > Some comments below: > > On 22/11/2023 20:48, Dean Anderson wrote: >> Adding V4L2 read-only control id for device id as hardware >> does not support embedding the device-id in the USB device >> descriptors. >> >> base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 > > Just drop this line, it doesn't belong in a commit message. > >> >> Signed-off-by: Dean Anderson <dean@sensoray.com> >> >> --- >> drivers/media/usb/s2255/s2255drv.c | 49 >> +++++++++++++++++++++++++++++- >> 1 file changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/usb/s2255/s2255drv.c >> b/drivers/media/usb/s2255/s2255drv.c >> index 3c2627712fe9..c2248bbc7840 100644 >> --- a/drivers/media/usb/s2255/s2255drv.c >> +++ b/drivers/media/usb/s2255/s2255drv.c >> @@ -40,7 +40,7 @@ >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-event.h> >> >> -#define S2255_VERSION "1.25.1" >> +#define S2255_VERSION "1.26.1" >> #define FIRMWARE_FILE_NAME "f2255usb.bin" >> >> /* default JPEG quality */ >> @@ -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_DEVICEID (V4L2_CID_USER_S2255_BASE + 1) >> >> /* frame prefix size (sent once every frame) */ >> #define PREFIX_SIZE 512 >> @@ -1232,6 +1234,37 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +/* deviceid/serial number is not used in usb device descriptors. >> + * returns device-id/serial number from device, 0 if none found. > > Please put the '/*' on a line by itself, that's consistent with the > coding style for multi-line comments. > > Also run the patch through 'checkpatch.pl --strict'. I get several > warnings. > > returns -> Returns > > One question about this comment: is the Device ID the same as a serial > number? "Device ID" can mean either the ID of a device model, or a > unique > ID for each device. If it is the latter, should it perhaps be called > "Device S/N" or just "Serial Number"? > >> + */ >> +#define S2255_DEVICEID_NONE 0 >> +static int s2255_g_deviceid(struct s2255_dev *dev) >> +{ >> + u8 *outbuf; >> + int rc; >> + int deviceid = S2255_DEVICEID_NONE; >> +#define S2255_I2C_SIZE 16 >> + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); > > Drop the "* sizeof(u8)", it serves no purpose. > >> + if (outbuf == NULL) >> + return deviceid; >> +#define S2255_I2C_SNDEV 0xa2 >> +#define S2255_I2C_SNOFFSET 0x1ff0 >> +#define S2255_USBVENDOR_READREG 0x22 >> + rc = s2255_vendor_req(dev, S2255_USBVENDOR_READREG, >> S2255_I2C_SNOFFSET, >> + S2255_I2C_SNDEV >> 1, outbuf, S2255_I2C_SIZE, 0); >> + if (rc < 0) >> + goto exit_g_deviceid; >> + >> + /* verify marker code */ >> + if (*(__le32 *)outbuf != S2255_MARKER_FIRMWARE) >> + goto exit_g_deviceid; >> + >> + deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << >> 8) + outbuf[15]; >> +exit_g_deviceid: >> + kfree(outbuf); >> + return deviceid; > > This is overly complicated. How about this: > > /* verify marker code */ > if (*(__le32 *)outbuf == S2255_MARKER_FIRMWARE) > deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << > 8) + outbuf[15]; > kfree(outbuf); > return deviceid; > >> +} >> + >> static int vidioc_g_jpegcomp(struct file *file, void *priv, >> struct v4l2_jpegcompression *jc) >> { >> @@ -1581,6 +1614,17 @@ static const struct v4l2_ctrl_config >> color_filter_ctrl = { >> .def = 1, >> }; >> >> +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { >> + .ops = &s2255_ctrl_ops, >> + .name = "Device ID", >> + .id = V4L2_CID_S2255_DEVICEID, >> + .type = V4L2_CTRL_TYPE_INTEGER, > > Please note that TYPE_INTEGER is a signed integer. > > If you need an unsigned integer, then use TYPE_U32. > >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .flags = V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> static int s2255_probe_v4l(struct s2255_dev *dev) >> { >> int ret; >> @@ -1615,6 +1659,9 @@ 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); >> + v4l2_ctrl_deviceid.def = s2255_g_deviceid(dev); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, >> + NULL); >> if (vc->hdl.error) { >> ret = vc->hdl.error; >> v4l2_ctrl_handler_free(&vc->hdl); > > Regards, > > Hans
Hi Hans, >> +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { >> + .ops = &s2255_ctrl_ops, >> + .name = "Device ID", >> + .id = V4L2_CID_S2255_DEVICEID, >> + .type = V4L2_CTRL_TYPE_INTEGER, > > Please note that TYPE_INTEGER is a signed integer. > > If you need an unsigned integer, then use TYPE_U32. It is unsigned, but it doesn't need to be. The max value should not run into a signed value. I can change the maximum value from 0xffffffff to 0x7fffffff. Is it acceptable to not use TYPE_U32 in order to simplify the user space calls? TYPE_U32 is a compound type. My understanding is VIDIOC_G_CTRL will not work with compound types and some V4L user programs may not implement the compound type queries. Best regards, Dean On 2023-12-06 03:43, Hans Verkuil wrote: > Hi Dean, > > Some comments below: > > On 22/11/2023 20:48, Dean Anderson wrote: >> Adding V4L2 read-only control id for device id as hardware >> does not support embedding the device-id in the USB device >> descriptors. >> >> base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 > > Just drop this line, it doesn't belong in a commit message. > >> >> Signed-off-by: Dean Anderson <dean@sensoray.com> >> >> --- >> drivers/media/usb/s2255/s2255drv.c | 49 >> +++++++++++++++++++++++++++++- >> 1 file changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/usb/s2255/s2255drv.c >> b/drivers/media/usb/s2255/s2255drv.c >> index 3c2627712fe9..c2248bbc7840 100644 >> --- a/drivers/media/usb/s2255/s2255drv.c >> +++ b/drivers/media/usb/s2255/s2255drv.c >> @@ -40,7 +40,7 @@ >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-event.h> >> >> -#define S2255_VERSION "1.25.1" >> +#define S2255_VERSION "1.26.1" >> #define FIRMWARE_FILE_NAME "f2255usb.bin" >> >> /* default JPEG quality */ >> @@ -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_DEVICEID (V4L2_CID_USER_S2255_BASE + 1) >> >> /* frame prefix size (sent once every frame) */ >> #define PREFIX_SIZE 512 >> @@ -1232,6 +1234,37 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +/* deviceid/serial number is not used in usb device descriptors. >> + * returns device-id/serial number from device, 0 if none found. > > Please put the '/*' on a line by itself, that's consistent with the > coding style for multi-line comments. > > Also run the patch through 'checkpatch.pl --strict'. I get several > warnings. > > returns -> Returns > > One question about this comment: is the Device ID the same as a serial > number? "Device ID" can mean either the ID of a device model, or a > unique > ID for each device. If it is the latter, should it perhaps be called > "Device S/N" or just "Serial Number"? > >> + */ >> +#define S2255_DEVICEID_NONE 0 >> +static int s2255_g_deviceid(struct s2255_dev *dev) >> +{ >> + u8 *outbuf; >> + int rc; >> + int deviceid = S2255_DEVICEID_NONE; >> +#define S2255_I2C_SIZE 16 >> + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); > > Drop the "* sizeof(u8)", it serves no purpose. > >> + if (outbuf == NULL) >> + return deviceid; >> +#define S2255_I2C_SNDEV 0xa2 >> +#define S2255_I2C_SNOFFSET 0x1ff0 >> +#define S2255_USBVENDOR_READREG 0x22 >> + rc = s2255_vendor_req(dev, S2255_USBVENDOR_READREG, >> S2255_I2C_SNOFFSET, >> + S2255_I2C_SNDEV >> 1, outbuf, S2255_I2C_SIZE, 0); >> + if (rc < 0) >> + goto exit_g_deviceid; >> + >> + /* verify marker code */ >> + if (*(__le32 *)outbuf != S2255_MARKER_FIRMWARE) >> + goto exit_g_deviceid; >> + >> + deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << >> 8) + outbuf[15]; >> +exit_g_deviceid: >> + kfree(outbuf); >> + return deviceid; > > This is overly complicated. How about this: > > /* verify marker code */ > if (*(__le32 *)outbuf == S2255_MARKER_FIRMWARE) > deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << > 8) + outbuf[15]; > kfree(outbuf); > return deviceid; > >> +} >> + >> static int vidioc_g_jpegcomp(struct file *file, void *priv, >> struct v4l2_jpegcompression *jc) >> { >> @@ -1581,6 +1614,17 @@ static const struct v4l2_ctrl_config >> color_filter_ctrl = { >> .def = 1, >> }; >> >> +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { >> + .ops = &s2255_ctrl_ops, >> + .name = "Device ID", >> + .id = V4L2_CID_S2255_DEVICEID, >> + .type = V4L2_CTRL_TYPE_INTEGER, > > Please note that TYPE_INTEGER is a signed integer. > > If you need an unsigned integer, then use TYPE_U32. > >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .flags = V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> static int s2255_probe_v4l(struct s2255_dev *dev) >> { >> int ret; >> @@ -1615,6 +1659,9 @@ 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); >> + v4l2_ctrl_deviceid.def = s2255_g_deviceid(dev); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, >> + NULL); >> if (vc->hdl.error) { >> ret = vc->hdl.error; >> v4l2_ctrl_handler_free(&vc->hdl); > > Regards, > > Hans
On 08/12/2023 01:03, dean@sensoray.com wrote: > Hi Hans, > >>> +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { >>> + .ops = &s2255_ctrl_ops, >>> + .name = "Device ID", >>> + .id = V4L2_CID_S2255_DEVICEID, >>> + .type = V4L2_CTRL_TYPE_INTEGER, >> >> Please note that TYPE_INTEGER is a signed integer. >> >> If you need an unsigned integer, then use TYPE_U32. > > > It is unsigned, but it doesn't need to be. The max value should not run into a signed value. I can change the maximum value from 0xffffffff to 0x7fffffff. > > Is it acceptable to not use TYPE_U32 in order to simplify the user space calls? Certainly! Hans > > TYPE_U32 is a compound type. My understanding is VIDIOC_G_CTRL will not work with compound types and some V4L user programs may not implement the compound type queries. > > Best regards, > > Dean > > > > > On 2023-12-06 03:43, Hans Verkuil wrote: >> Hi Dean, >> >> Some comments below: >> >> On 22/11/2023 20:48, Dean Anderson wrote: >>> Adding V4L2 read-only control id for device id as hardware >>> does not support embedding the device-id in the USB device descriptors. >>> >>> base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 >> >> Just drop this line, it doesn't belong in a commit message. >> >>> >>> Signed-off-by: Dean Anderson <dean@sensoray.com> >>> >>> --- >>> drivers/media/usb/s2255/s2255drv.c | 49 +++++++++++++++++++++++++++++- >>> 1 file changed, 48 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c >>> index 3c2627712fe9..c2248bbc7840 100644 >>> --- a/drivers/media/usb/s2255/s2255drv.c >>> +++ b/drivers/media/usb/s2255/s2255drv.c >>> @@ -40,7 +40,7 @@ >>> #include <media/v4l2-ctrls.h> >>> #include <media/v4l2-event.h> >>> >>> -#define S2255_VERSION "1.25.1" >>> +#define S2255_VERSION "1.26.1" >>> #define FIRMWARE_FILE_NAME "f2255usb.bin" >>> >>> /* default JPEG quality */ >>> @@ -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_DEVICEID (V4L2_CID_USER_S2255_BASE + 1) >>> >>> /* frame prefix size (sent once every frame) */ >>> #define PREFIX_SIZE 512 >>> @@ -1232,6 +1234,37 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >>> return 0; >>> } >>> >>> +/* deviceid/serial number is not used in usb device descriptors. >>> + * returns device-id/serial number from device, 0 if none found. >> >> Please put the '/*' on a line by itself, that's consistent with the >> coding style for multi-line comments. >> >> Also run the patch through 'checkpatch.pl --strict'. I get several >> warnings. >> >> returns -> Returns >> >> One question about this comment: is the Device ID the same as a serial >> number? "Device ID" can mean either the ID of a device model, or a unique >> ID for each device. If it is the latter, should it perhaps be called >> "Device S/N" or just "Serial Number"? >> >>> + */ >>> +#define S2255_DEVICEID_NONE 0 >>> +static int s2255_g_deviceid(struct s2255_dev *dev) >>> +{ >>> + u8 *outbuf; >>> + int rc; >>> + int deviceid = S2255_DEVICEID_NONE; >>> +#define S2255_I2C_SIZE 16 >>> + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); >> >> Drop the "* sizeof(u8)", it serves no purpose. >> >>> + if (outbuf == NULL) >>> + return deviceid; >>> +#define S2255_I2C_SNDEV 0xa2 >>> +#define S2255_I2C_SNOFFSET 0x1ff0 >>> +#define S2255_USBVENDOR_READREG 0x22 >>> + rc = s2255_vendor_req(dev, S2255_USBVENDOR_READREG, S2255_I2C_SNOFFSET, >>> + S2255_I2C_SNDEV >> 1, outbuf, S2255_I2C_SIZE, 0); >>> + if (rc < 0) >>> + goto exit_g_deviceid; >>> + >>> + /* verify marker code */ >>> + if (*(__le32 *)outbuf != S2255_MARKER_FIRMWARE) >>> + goto exit_g_deviceid; >>> + >>> + deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; >>> +exit_g_deviceid: >>> + kfree(outbuf); >>> + return deviceid; >> >> This is overly complicated. How about this: >> >> /* verify marker code */ >> if (*(__le32 *)outbuf == S2255_MARKER_FIRMWARE) >> deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << >> 8) + outbuf[15]; >> kfree(outbuf); >> return deviceid; >> >>> +} >>> + >>> static int vidioc_g_jpegcomp(struct file *file, void *priv, >>> struct v4l2_jpegcompression *jc) >>> { >>> @@ -1581,6 +1614,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { >>> .def = 1, >>> }; >>> >>> +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { >>> + .ops = &s2255_ctrl_ops, >>> + .name = "Device ID", >>> + .id = V4L2_CID_S2255_DEVICEID, >>> + .type = V4L2_CTRL_TYPE_INTEGER, >> >> Please note that TYPE_INTEGER is a signed integer. >> >> If you need an unsigned integer, then use TYPE_U32. >> >>> + .max = 0xffffffff, >>> + .min = 0, >>> + .step = 1, >>> + .flags = V4L2_CTRL_FLAG_READ_ONLY, >>> +}; >>> + >>> static int s2255_probe_v4l(struct s2255_dev *dev) >>> { >>> int ret; >>> @@ -1615,6 +1659,9 @@ 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); >>> + v4l2_ctrl_deviceid.def = s2255_g_deviceid(dev); >>> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, >>> + NULL); >>> if (vc->hdl.error) { >>> ret = vc->hdl.error; >>> v4l2_ctrl_handler_free(&vc->hdl); >> >> Regards, >> >> Hans
diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c index 3c2627712fe9..c2248bbc7840 100644 --- a/drivers/media/usb/s2255/s2255drv.c +++ b/drivers/media/usb/s2255/s2255drv.c @@ -40,7 +40,7 @@ #include <media/v4l2-ctrls.h> #include <media/v4l2-event.h> -#define S2255_VERSION "1.25.1" +#define S2255_VERSION "1.26.1" #define FIRMWARE_FILE_NAME "f2255usb.bin" /* default JPEG quality */ @@ -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_DEVICEID (V4L2_CID_USER_S2255_BASE + 1) /* frame prefix size (sent once every frame) */ #define PREFIX_SIZE 512 @@ -1232,6 +1234,37 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) return 0; } +/* deviceid/serial number is not used in usb device descriptors. + * returns device-id/serial number from device, 0 if none found. + */ +#define S2255_DEVICEID_NONE 0 +static int s2255_g_deviceid(struct s2255_dev *dev) +{ + u8 *outbuf; + int rc; + int deviceid = S2255_DEVICEID_NONE; +#define S2255_I2C_SIZE 16 + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); + if (outbuf == NULL) + return deviceid; +#define S2255_I2C_SNDEV 0xa2 +#define S2255_I2C_SNOFFSET 0x1ff0 +#define S2255_USBVENDOR_READREG 0x22 + rc = s2255_vendor_req(dev, S2255_USBVENDOR_READREG, S2255_I2C_SNOFFSET, + S2255_I2C_SNDEV >> 1, outbuf, S2255_I2C_SIZE, 0); + if (rc < 0) + goto exit_g_deviceid; + + /* verify marker code */ + if (*(__le32 *)outbuf != S2255_MARKER_FIRMWARE) + goto exit_g_deviceid; + + deviceid = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; +exit_g_deviceid: + kfree(outbuf); + return deviceid; +} + static int vidioc_g_jpegcomp(struct file *file, void *priv, struct v4l2_jpegcompression *jc) { @@ -1581,6 +1614,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { .def = 1, }; +static struct v4l2_ctrl_config v4l2_ctrl_deviceid = { + .ops = &s2255_ctrl_ops, + .name = "Device ID", + .id = V4L2_CID_S2255_DEVICEID, + .type = V4L2_CTRL_TYPE_INTEGER, + .max = 0xffffffff, + .min = 0, + .step = 1, + .flags = V4L2_CTRL_FLAG_READ_ONLY, +}; + static int s2255_probe_v4l(struct s2255_dev *dev) { int ret; @@ -1615,6 +1659,9 @@ 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); + v4l2_ctrl_deviceid.def = s2255_g_deviceid(dev); + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, + NULL); if (vc->hdl.error) { ret = vc->hdl.error; v4l2_ctrl_handler_free(&vc->hdl);
Adding V4L2 read-only control id for device id as hardware does not support embedding the device-id in the USB device descriptors. base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 Signed-off-by: Dean Anderson <dean@sensoray.com> --- drivers/media/usb/s2255/s2255drv.c | 49 +++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)