| Message ID | 20231121215323.462427-1-dean@sensoray.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: usb: s2255: custom V4L2_CIDs | expand |
Hi Dean, Thank you for the patch, but I have some question about this patch: On 21/11/2023 22:53, Dean Anderson wrote: > Adding read-only V4L2 control ids for device-id and on-board > firmware versions. > > base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 > > Signed-off-by: Dean Anderson <dean@sensoray.com> > > --- > drivers/media/usb/s2255/s2255drv.c | 98 ++++++++++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c > index 3c2627712fe9..1c7cb1d37353 100644 > --- a/drivers/media/usb/s2255/s2255drv.c > +++ b/drivers/media/usb/s2255/s2255drv.c > @@ -262,6 +262,7 @@ struct s2255_dev { > int chn_ready; > /* dsp firmware version (f2255usb.bin) */ > int dsp_fw_ver; > + int usb_fw_ver; > u16 pid; /* product id */ > #define S2255_CMDBUF_SIZE 512 > __le32 *cmdbuf; > @@ -323,6 +324,9 @@ 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) > +#define V4L2_CID_S2255_DSPFWVER (V4L2_CID_USER_S2255_BASE + 2) > +#define V4L2_CID_S2255_USBFWVER (V4L2_CID_USER_S2255_BASE + 3) Why do you want to expose this as controls? Usually such information is output to the kernel log during probe since it shouldn't be needed by userspace. At minimum the commit log should explain the reason for adding these controls. > > /* frame prefix size (sent once every frame) */ > #define PREFIX_SIZE 512 > @@ -1232,6 +1236,56 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) > return 0; > } > > +static int s2255_read_reg_burst(struct s2255_dev *dev, u8 dev_addr, > + u16 reg_addr, u8 *data, u8 datalen) > +{ > + int rc; > + > + rc = s2255_vendor_req(dev, 0x22, reg_addr, dev_addr >> 1, data, datalen, 0); > + return rc; Just do 'return s2255_...', no need for the rc variable. > +} > + > +static int s2255_g_volatile_ctrl(struct v4l2_ctrl *ctrl) These controls are definitely not volatile controls. Just read out the registers during probe and create these read-only controls with the correct initial value. No need to do anything else. Unless these versions/device IDs dynamically change, which is very unlikely. > +{ > + u8 *outbuf; > + int rc; > + struct s2255_vc *vc = > + container_of(ctrl->handler, struct s2255_vc, hdl); > + struct s2255_dev *dev = vc->dev; > + > + if (ctrl->id == V4L2_CID_S2255_DSPFWVER) { > + ctrl->val = dev->dsp_fw_ver; > + return 0; > + } > + if (ctrl->id == V4L2_CID_S2255_USBFWVER) { > + ctrl->val = dev->usb_fw_ver; > + return 0; > + } > + if (ctrl->id != V4L2_CID_S2255_DEVICEID) > + return -EINVAL; > + if (dev == NULL) > + return -EINVAL; > +#define S2255_I2C_SIZE 16 > + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); > + if (outbuf == NULL) > + return -ENOMEM; > +#define S2255_I2C_SNDEV 0xa2 > +#define S2255_I2C_SNOFFSET 0x1ff0 > + rc = s2255_read_reg_burst(dev, S2255_I2C_SNDEV, S2255_I2C_SNOFFSET, outbuf, S2255_I2C_SIZE); > + if (rc < 0) { > + kfree(outbuf); > + return rc; > + } > + // verify marker code > + if (*(unsigned int *)outbuf != 0xddccbbaa) { The 'media: usb: s2255: endian fix' patch fixes a bug introduced here. Just fold that fix into this patch instead. > + kfree(outbuf); > + return -EFAULT; > + } > + ctrl->val = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; > + kfree(outbuf); > + return 0; > +} > + > static int vidioc_g_jpegcomp(struct file *file, void *priv, > struct v4l2_jpegcompression *jc) > { > @@ -1569,6 +1623,7 @@ static const struct video_device template = { > > static const struct v4l2_ctrl_ops s2255_ctrl_ops = { > .s_ctrl = s2255_s_ctrl, > + .g_volatile_ctrl = s2255_g_volatile_ctrl, > }; > > static const struct v4l2_ctrl_config color_filter_ctrl = { > @@ -1581,6 +1636,42 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { > .def = 1, > }; > > +static const 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, > + .def = 0, > + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, > +}; > + > +static const struct v4l2_ctrl_config v4l2_ctrl_dspfwver = { > + .ops = &s2255_ctrl_ops, > + .name = "DSP Firmware", > + .id = V4L2_CID_S2255_DSPFWVER, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .max = 0xffffffff, > + .min = 0, > + .step = 1, > + .def = 0, > + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, > +}; > + > +static const struct v4l2_ctrl_config v4l2_ctrl_usbfwver = { > + .ops = &s2255_ctrl_ops, > + .name = "USB Firmware", > + .id = V4L2_CID_S2255_USBFWVER, > + .type = V4L2_CTRL_TYPE_INTEGER, > + .max = 0xffffffff, > + .min = 0, > + .step = 1, > + .def = 0, > + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, > +}; > + > static int s2255_probe_v4l(struct s2255_dev *dev) > { > int ret; > @@ -1615,6 +1706,12 @@ 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_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, > + NULL); > + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_usbfwver, > + NULL); > + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_dspfwver, > + NULL); > if (vc->hdl.error) { > ret = vc->hdl.error; > v4l2_ctrl_handler_free(&vc->hdl); > @@ -1983,6 +2080,7 @@ static int s2255_board_init(struct s2255_dev *dev) > } > /* query the firmware */ > fw_ver = s2255_get_fx2fw(dev); > + dev->usb_fw_ver = fw_ver; > > pr_info("s2255: usb firmware version %d.%d\n", > (fw_ver >> 8) & 0xff, Regards, Hans
On 2023-11-22 04:18, Hans Verkuil wrote: > Hi Dean, > > Thank you for the patch, but I have some question about this patch: > > On 21/11/2023 22:53, Dean Anderson wrote: >> Adding read-only V4L2 control ids for device-id and on-board >> firmware versions. >> >> base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 >> >> Signed-off-by: Dean Anderson <dean@sensoray.com> >> >> --- >> drivers/media/usb/s2255/s2255drv.c | 98 >> ++++++++++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> >> diff --git a/drivers/media/usb/s2255/s2255drv.c >> b/drivers/media/usb/s2255/s2255drv.c >> index 3c2627712fe9..1c7cb1d37353 100644 >> --- a/drivers/media/usb/s2255/s2255drv.c >> +++ b/drivers/media/usb/s2255/s2255drv.c >> @@ -262,6 +262,7 @@ struct s2255_dev { >> int chn_ready; >> /* dsp firmware version (f2255usb.bin) */ >> int dsp_fw_ver; >> + int usb_fw_ver; >> u16 pid; /* product id */ >> #define S2255_CMDBUF_SIZE 512 >> __le32 *cmdbuf; >> @@ -323,6 +324,9 @@ 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) >> +#define V4L2_CID_S2255_DSPFWVER (V4L2_CID_USER_S2255_BASE + 2) >> +#define V4L2_CID_S2255_USBFWVER (V4L2_CID_USER_S2255_BASE + 3) > > Why do you want to expose this as controls? Usually such information > is output to the kernel log during probe since it shouldn't be needed > by userspace. At minimum the commit log should explain the reason for > adding these controls. > The deviceid is essentially the serial number. There are some users that have requested it. Ideally, it would be in the USB descriptors, but this wasn't possible with the hardware as is. I agree the others aren't usually needed, but some customers may want to verify programmatically the loaded firmware is up to date instead of grepping the kernel log. >> >> /* frame prefix size (sent once every frame) */ >> #define PREFIX_SIZE 512 >> @@ -1232,6 +1236,56 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +static int s2255_read_reg_burst(struct s2255_dev *dev, u8 dev_addr, >> + u16 reg_addr, u8 *data, u8 datalen) >> +{ >> + int rc; >> + >> + rc = s2255_vendor_req(dev, 0x22, reg_addr, dev_addr >> 1, data, >> datalen, 0); >> + return rc; > > Just do 'return s2255_...', no need for the rc variable. > >> +} >> + >> +static int s2255_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > These controls are definitely not volatile controls. Just > read out the registers during probe and create these read-only > controls with the correct initial value. No need to do anything > else. Unless these versions/device IDs dynamically change, which > is very unlikely. Correct, they aren't volatile. > >> +{ >> + u8 *outbuf; >> + int rc; >> + struct s2255_vc *vc = >> + container_of(ctrl->handler, struct s2255_vc, hdl); >> + struct s2255_dev *dev = vc->dev; >> + >> + if (ctrl->id == V4L2_CID_S2255_DSPFWVER) { >> + ctrl->val = dev->dsp_fw_ver; >> + return 0; >> + } >> + if (ctrl->id == V4L2_CID_S2255_USBFWVER) { >> + ctrl->val = dev->usb_fw_ver; >> + return 0; >> + } >> + if (ctrl->id != V4L2_CID_S2255_DEVICEID) >> + return -EINVAL; >> + if (dev == NULL) >> + return -EINVAL; >> +#define S2255_I2C_SIZE 16 >> + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); >> + if (outbuf == NULL) >> + return -ENOMEM; >> +#define S2255_I2C_SNDEV 0xa2 >> +#define S2255_I2C_SNOFFSET 0x1ff0 >> + rc = s2255_read_reg_burst(dev, S2255_I2C_SNDEV, S2255_I2C_SNOFFSET, >> outbuf, S2255_I2C_SIZE); >> + if (rc < 0) { >> + kfree(outbuf); >> + return rc; >> + } >> + // verify marker code >> + if (*(unsigned int *)outbuf != 0xddccbbaa) { > > The 'media: usb: s2255: endian fix' patch fixes a bug introduced here. > Just fold > that fix into this patch instead. > >> + kfree(outbuf); >> + return -EFAULT; >> + } >> + ctrl->val = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << >> 8) + outbuf[15]; >> + kfree(outbuf); >> + return 0; >> +} >> + >> static int vidioc_g_jpegcomp(struct file *file, void *priv, >> struct v4l2_jpegcompression *jc) >> { >> @@ -1569,6 +1623,7 @@ static const struct video_device template = { >> >> static const struct v4l2_ctrl_ops s2255_ctrl_ops = { >> .s_ctrl = s2255_s_ctrl, >> + .g_volatile_ctrl = s2255_g_volatile_ctrl, >> }; >> >> static const struct v4l2_ctrl_config color_filter_ctrl = { >> @@ -1581,6 +1636,42 @@ static const struct v4l2_ctrl_config >> color_filter_ctrl = { >> .def = 1, >> }; >> >> +static const 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, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> +static const struct v4l2_ctrl_config v4l2_ctrl_dspfwver = { >> + .ops = &s2255_ctrl_ops, >> + .name = "DSP Firmware", >> + .id = V4L2_CID_S2255_DSPFWVER, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> +static const struct v4l2_ctrl_config v4l2_ctrl_usbfwver = { >> + .ops = &s2255_ctrl_ops, >> + .name = "USB Firmware", >> + .id = V4L2_CID_S2255_USBFWVER, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> static int s2255_probe_v4l(struct s2255_dev *dev) >> { >> int ret; >> @@ -1615,6 +1706,12 @@ 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_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, >> + NULL); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_usbfwver, >> + NULL); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_dspfwver, >> + NULL); >> if (vc->hdl.error) { >> ret = vc->hdl.error; >> v4l2_ctrl_handler_free(&vc->hdl); >> @@ -1983,6 +2080,7 @@ static int s2255_board_init(struct s2255_dev >> *dev) >> } >> /* query the firmware */ >> fw_ver = s2255_get_fx2fw(dev); >> + dev->usb_fw_ver = fw_ver; >> >> pr_info("s2255: usb firmware version %d.%d\n", >> (fw_ver >> 8) & 0xff, > > Regards, > > Hans
I re-submitted the patch with the firmware version controls removed. They could be added in a later update if necessary. The control for the device-id is now non-volatile. I added a description on why the patch is necessary due to the serial-number not being present in the usb descriptors. Thanks, Dean On 2023-11-22 04:18, Hans Verkuil wrote: > Hi Dean, > > Thank you for the patch, but I have some question about this patch: > > On 21/11/2023 22:53, Dean Anderson wrote: >> Adding read-only V4L2 control ids for device-id and on-board >> firmware versions. >> >> base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 >> >> Signed-off-by: Dean Anderson <dean@sensoray.com> >> >> --- >> drivers/media/usb/s2255/s2255drv.c | 98 >> ++++++++++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> >> diff --git a/drivers/media/usb/s2255/s2255drv.c >> b/drivers/media/usb/s2255/s2255drv.c >> index 3c2627712fe9..1c7cb1d37353 100644 >> --- a/drivers/media/usb/s2255/s2255drv.c >> +++ b/drivers/media/usb/s2255/s2255drv.c >> @@ -262,6 +262,7 @@ struct s2255_dev { >> int chn_ready; >> /* dsp firmware version (f2255usb.bin) */ >> int dsp_fw_ver; >> + int usb_fw_ver; >> u16 pid; /* product id */ >> #define S2255_CMDBUF_SIZE 512 >> __le32 *cmdbuf; >> @@ -323,6 +324,9 @@ 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) >> +#define V4L2_CID_S2255_DSPFWVER (V4L2_CID_USER_S2255_BASE + 2) >> +#define V4L2_CID_S2255_USBFWVER (V4L2_CID_USER_S2255_BASE + 3) > > Why do you want to expose this as controls? Usually such information > is output to the kernel log during probe since it shouldn't be needed > by userspace. At minimum the commit log should explain the reason for > adding these controls. > >> >> /* frame prefix size (sent once every frame) */ >> #define PREFIX_SIZE 512 >> @@ -1232,6 +1236,56 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) >> return 0; >> } >> >> +static int s2255_read_reg_burst(struct s2255_dev *dev, u8 dev_addr, >> + u16 reg_addr, u8 *data, u8 datalen) >> +{ >> + int rc; >> + >> + rc = s2255_vendor_req(dev, 0x22, reg_addr, dev_addr >> 1, data, >> datalen, 0); >> + return rc; > > Just do 'return s2255_...', no need for the rc variable. > >> +} >> + >> +static int s2255_g_volatile_ctrl(struct v4l2_ctrl *ctrl) > > These controls are definitely not volatile controls. Just > read out the registers during probe and create these read-only > controls with the correct initial value. No need to do anything > else. Unless these versions/device IDs dynamically change, which > is very unlikely. > >> +{ >> + u8 *outbuf; >> + int rc; >> + struct s2255_vc *vc = >> + container_of(ctrl->handler, struct s2255_vc, hdl); >> + struct s2255_dev *dev = vc->dev; >> + >> + if (ctrl->id == V4L2_CID_S2255_DSPFWVER) { >> + ctrl->val = dev->dsp_fw_ver; >> + return 0; >> + } >> + if (ctrl->id == V4L2_CID_S2255_USBFWVER) { >> + ctrl->val = dev->usb_fw_ver; >> + return 0; >> + } >> + if (ctrl->id != V4L2_CID_S2255_DEVICEID) >> + return -EINVAL; >> + if (dev == NULL) >> + return -EINVAL; >> +#define S2255_I2C_SIZE 16 >> + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); >> + if (outbuf == NULL) >> + return -ENOMEM; >> +#define S2255_I2C_SNDEV 0xa2 >> +#define S2255_I2C_SNOFFSET 0x1ff0 >> + rc = s2255_read_reg_burst(dev, S2255_I2C_SNDEV, S2255_I2C_SNOFFSET, >> outbuf, S2255_I2C_SIZE); >> + if (rc < 0) { >> + kfree(outbuf); >> + return rc; >> + } >> + // verify marker code >> + if (*(unsigned int *)outbuf != 0xddccbbaa) { > > The 'media: usb: s2255: endian fix' patch fixes a bug introduced here. > Just fold > that fix into this patch instead. > >> + kfree(outbuf); >> + return -EFAULT; >> + } >> + ctrl->val = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << >> 8) + outbuf[15]; >> + kfree(outbuf); >> + return 0; >> +} >> + >> static int vidioc_g_jpegcomp(struct file *file, void *priv, >> struct v4l2_jpegcompression *jc) >> { >> @@ -1569,6 +1623,7 @@ static const struct video_device template = { >> >> static const struct v4l2_ctrl_ops s2255_ctrl_ops = { >> .s_ctrl = s2255_s_ctrl, >> + .g_volatile_ctrl = s2255_g_volatile_ctrl, >> }; >> >> static const struct v4l2_ctrl_config color_filter_ctrl = { >> @@ -1581,6 +1636,42 @@ static const struct v4l2_ctrl_config >> color_filter_ctrl = { >> .def = 1, >> }; >> >> +static const 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, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> +static const struct v4l2_ctrl_config v4l2_ctrl_dspfwver = { >> + .ops = &s2255_ctrl_ops, >> + .name = "DSP Firmware", >> + .id = V4L2_CID_S2255_DSPFWVER, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> +static const struct v4l2_ctrl_config v4l2_ctrl_usbfwver = { >> + .ops = &s2255_ctrl_ops, >> + .name = "USB Firmware", >> + .id = V4L2_CID_S2255_USBFWVER, >> + .type = V4L2_CTRL_TYPE_INTEGER, >> + .max = 0xffffffff, >> + .min = 0, >> + .step = 1, >> + .def = 0, >> + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, >> +}; >> + >> static int s2255_probe_v4l(struct s2255_dev *dev) >> { >> int ret; >> @@ -1615,6 +1706,12 @@ 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_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, >> + NULL); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_usbfwver, >> + NULL); >> + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_dspfwver, >> + NULL); >> if (vc->hdl.error) { >> ret = vc->hdl.error; >> v4l2_ctrl_handler_free(&vc->hdl); >> @@ -1983,6 +2080,7 @@ static int s2255_board_init(struct s2255_dev >> *dev) >> } >> /* query the firmware */ >> fw_ver = s2255_get_fx2fw(dev); >> + dev->usb_fw_ver = fw_ver; >> >> pr_info("s2255: usb firmware version %d.%d\n", >> (fw_ver >> 8) & 0xff, > > Regards, > > Hans
diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c index 3c2627712fe9..1c7cb1d37353 100644 --- a/drivers/media/usb/s2255/s2255drv.c +++ b/drivers/media/usb/s2255/s2255drv.c @@ -262,6 +262,7 @@ struct s2255_dev { int chn_ready; /* dsp firmware version (f2255usb.bin) */ int dsp_fw_ver; + int usb_fw_ver; u16 pid; /* product id */ #define S2255_CMDBUF_SIZE 512 __le32 *cmdbuf; @@ -323,6 +324,9 @@ 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) +#define V4L2_CID_S2255_DSPFWVER (V4L2_CID_USER_S2255_BASE + 2) +#define V4L2_CID_S2255_USBFWVER (V4L2_CID_USER_S2255_BASE + 3) /* frame prefix size (sent once every frame) */ #define PREFIX_SIZE 512 @@ -1232,6 +1236,56 @@ static int s2255_s_ctrl(struct v4l2_ctrl *ctrl) return 0; } +static int s2255_read_reg_burst(struct s2255_dev *dev, u8 dev_addr, + u16 reg_addr, u8 *data, u8 datalen) +{ + int rc; + + rc = s2255_vendor_req(dev, 0x22, reg_addr, dev_addr >> 1, data, datalen, 0); + return rc; +} + +static int s2255_g_volatile_ctrl(struct v4l2_ctrl *ctrl) +{ + u8 *outbuf; + int rc; + struct s2255_vc *vc = + container_of(ctrl->handler, struct s2255_vc, hdl); + struct s2255_dev *dev = vc->dev; + + if (ctrl->id == V4L2_CID_S2255_DSPFWVER) { + ctrl->val = dev->dsp_fw_ver; + return 0; + } + if (ctrl->id == V4L2_CID_S2255_USBFWVER) { + ctrl->val = dev->usb_fw_ver; + return 0; + } + if (ctrl->id != V4L2_CID_S2255_DEVICEID) + return -EINVAL; + if (dev == NULL) + return -EINVAL; +#define S2255_I2C_SIZE 16 + outbuf = kzalloc(S2255_I2C_SIZE * sizeof(u8), GFP_KERNEL); + if (outbuf == NULL) + return -ENOMEM; +#define S2255_I2C_SNDEV 0xa2 +#define S2255_I2C_SNOFFSET 0x1ff0 + rc = s2255_read_reg_burst(dev, S2255_I2C_SNDEV, S2255_I2C_SNOFFSET, outbuf, S2255_I2C_SIZE); + if (rc < 0) { + kfree(outbuf); + return rc; + } + // verify marker code + if (*(unsigned int *)outbuf != 0xddccbbaa) { + kfree(outbuf); + return -EFAULT; + } + ctrl->val = (outbuf[12] << 24) + (outbuf[13] << 16) + (outbuf[14] << 8) + outbuf[15]; + kfree(outbuf); + return 0; +} + static int vidioc_g_jpegcomp(struct file *file, void *priv, struct v4l2_jpegcompression *jc) { @@ -1569,6 +1623,7 @@ static const struct video_device template = { static const struct v4l2_ctrl_ops s2255_ctrl_ops = { .s_ctrl = s2255_s_ctrl, + .g_volatile_ctrl = s2255_g_volatile_ctrl, }; static const struct v4l2_ctrl_config color_filter_ctrl = { @@ -1581,6 +1636,42 @@ static const struct v4l2_ctrl_config color_filter_ctrl = { .def = 1, }; +static const 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, + .def = 0, + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, +}; + +static const struct v4l2_ctrl_config v4l2_ctrl_dspfwver = { + .ops = &s2255_ctrl_ops, + .name = "DSP Firmware", + .id = V4L2_CID_S2255_DSPFWVER, + .type = V4L2_CTRL_TYPE_INTEGER, + .max = 0xffffffff, + .min = 0, + .step = 1, + .def = 0, + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, +}; + +static const struct v4l2_ctrl_config v4l2_ctrl_usbfwver = { + .ops = &s2255_ctrl_ops, + .name = "USB Firmware", + .id = V4L2_CID_S2255_USBFWVER, + .type = V4L2_CTRL_TYPE_INTEGER, + .max = 0xffffffff, + .min = 0, + .step = 1, + .def = 0, + .flags = V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY, +}; + static int s2255_probe_v4l(struct s2255_dev *dev) { int ret; @@ -1615,6 +1706,12 @@ 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_new_custom(&vc->hdl, &v4l2_ctrl_deviceid, + NULL); + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_usbfwver, + NULL); + v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_dspfwver, + NULL); if (vc->hdl.error) { ret = vc->hdl.error; v4l2_ctrl_handler_free(&vc->hdl); @@ -1983,6 +2080,7 @@ static int s2255_board_init(struct s2255_dev *dev) } /* query the firmware */ fw_ver = s2255_get_fx2fw(dev); + dev->usb_fw_ver = fw_ver; pr_info("s2255: usb firmware version %d.%d\n", (fw_ver >> 8) & 0xff,
Adding read-only V4L2 control ids for device-id and on-board firmware versions. base-commit: 3e238417254bfdcc23fe207780b59cbb08656762 Signed-off-by: Dean Anderson <dean@sensoray.com> --- drivers/media/usb/s2255/s2255drv.c | 98 ++++++++++++++++++++++++++++++ 1 file changed, 98 insertions(+)