Re: [PATCHv3] media: usb: s2255: add serial number V4L2_CID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> 
> ---
>  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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux