* [PATCHv3] media: usb: s2255: add serial number V4L2_CID
@ 2023-12-15 19:14 Dean Anderson
2024-01-08 12:52 ` Sakari Ailus
0 siblings, 1 reply; 6+ messages in thread
From: Dean Anderson @ 2023-12-15 19:14 UTC (permalink / raw)
To: linux-media; +Cc: dean
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)
#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);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID
2023-12-15 19:14 [PATCHv3] media: usb: s2255: add serial number V4L2_CID Dean Anderson
@ 2024-01-08 12:52 ` Sakari Ailus
2024-01-08 13:25 ` Laurent Pinchart
2024-01-10 23:50 ` dean
0 siblings, 2 replies; 6+ messages in thread
From: Sakari Ailus @ 2024-01-08 12:52 UTC (permalink / raw)
To: Dean Anderson; +Cc: linux-media, laurent.pinchart, hverkuil
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);
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID
2024-01-08 12:52 ` Sakari Ailus
@ 2024-01-08 13:25 ` Laurent Pinchart
2024-01-08 22:49 ` dean
2024-01-10 23:50 ` dean
1 sibling, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2024-01-08 13:25 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Dean Anderson, linux-media, hverkuil
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);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID
2024-01-08 13:25 ` Laurent Pinchart
@ 2024-01-08 22:49 ` dean
2024-01-09 17:28 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: dean @ 2024-01-08 22:49 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, hverkuil
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID
2024-01-08 22:49 ` dean
@ 2024-01-09 17:28 ` Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2024-01-09 17:28 UTC (permalink / raw)
To: dean; +Cc: Sakari Ailus, linux-media, hverkuil
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);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID
2024-01-08 12:52 ` Sakari Ailus
2024-01-08 13:25 ` Laurent Pinchart
@ 2024-01-10 23:50 ` dean
1 sibling, 0 replies; 6+ messages in thread
From: dean @ 2024-01-10 23:50 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, hverkuil
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-10 23:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-15 19:14 [PATCHv3] media: usb: s2255: add serial number V4L2_CID Dean Anderson
2024-01-08 12:52 ` Sakari Ailus
2024-01-08 13:25 ` Laurent Pinchart
2024-01-08 22:49 ` dean
2024-01-09 17:28 ` Laurent Pinchart
2024-01-10 23:50 ` dean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.