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

[dean@sensoray.com]

On 2023-12-13 04:03, Hans Verkuil wrote:
> Hi Dean,
> 
> A few more comments below.
> 
> BTW, when you post an new version of a patch, it is good practice to
> show that in the Subject line. So the next version should say: 
> '[PATCHv3]'.
> 
> That helps keeping track of versions.
> 
> On 08/12/2023 23:38, 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.
>> 
>> Signed-off-by: Dean Anderson <dean@sensoray.com>
>> 
>> ---
>>  drivers/media/usb/s2255/s2255drv.c | 46 
>> ++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/media/usb/s2255/s2255drv.c 
>> b/drivers/media/usb/s2255/s2255drv.c
>> index 3c2627712fe9..5fdf12a6c47a 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"
> 
> Something for a separate patch: it is discouraged to have a driver 
> specific
> version number.
> 
> The version number as returned by VIDIOC_QUERYCAP is just filled with 
> the
> kernel version, which is really what you need.
> 
> In practice driver versions are rarely updated, and generally useless.
> 
>>  #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_SERIALNUM (V4L2_CID_USER_S2255_BASE + 1)
>> 
>>  /* frame prefix size (sent once every frame) */
>>  #define PREFIX_SIZE		512
>> @@ -1232,6 +1234,32 @@ 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.
>> + */
>> +#define S2255_SERIALNUM_NONE 0
>> +static int s2255_g_serialnum(struct s2255_dev *dev)
>> +{
>> +	u8 *buf;
>> +	int serialnum = S2255_SERIALNUM_NONE;
>> +#define S2255_I2C_SIZE     16
>> +	buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL);
>> +	if (!buf)
>> +		return serialnum;
>> +#define S2255_I2C_SERIALNUM 0xa2
>> +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0
>> +#define S2255_VENDOR_READREG 0x22
> 
> Having these defines mixed in with the code is rather distracting.
> 
> Just collect them and have them at the beginning of the function,
> right after the opening {.
> 
>> +	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 +1609,17 @@ static const struct v4l2_ctrl_config 
>> color_filter_ctrl = {
>>  	.def = 1,
>>  };
>> 
>> +static struct v4l2_ctrl_config v4l2_ctrl_serialnum = {
> 
> This should be 'const'.
> 
>> +	.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;
>> @@ -1598,7 +1637,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 +1654,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_serialnum.def = s2255_g_serialnum(dev);
> 
> You can't do this, this could cause problems if you have multiple S2255
> devices connected.
> 

v4l2_ctrl_new_custom copies out the default before calling 
v4l2_ctrl_fill, but I agree about the scope. It also must be changed 
with const added.

> Easiest is to do something like this:
> 
> 	struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum;
> 
> 	tmp.def = s2255_g_serialnum(dev);
> 
> and then pass &tmp to v4l2_ctrl_new_custom below.
> 
>> +		v4l2_ctrl_new_custom(&vc->hdl, &v4l2_ctrl_serialnum,
>> +				     NULL);
>>  		if (vc->hdl.error) {
>>  			ret = vc->hdl.error;
>>  			v4l2_ctrl_handler_free(&vc->hdl);

> 
> I never noticed this before, but there is a call to
> v4l2_ctrl_handler_setup() missing
> in this driver! That call ensures that s2255_s_ctrl() is called with
> the initial control
> values.
> It's probably something that should be added.
> 
> Regards,
> 
> 	Hans