media: usb: s2255: add serial number V4L2_CID

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(-)

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.

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

On 2023-12-13 04:03, Hans Verkuil wrote:
> Hi Dean,
> 
> A few more comments below.
> 
...
> 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.

If this is added, it needs done very carefully. Adding it to probe will 
100% break the firmware load. s2255_s_ctrl sends a USB command to the 
board and the asynchronous firmware load shares the endpoint for 
commands.

The initial brightness, contrast, hue, and saturation default values 
match what the firmware sets when first loaded. It's redundant to set 
them again immediately after the firmware load succeeds.  I think at a 
minimum this belongs in a separate patch.

Regards,

Dean

> 
> Regards,
> 
> 	Hans

On 15/12/2023 00:28, dean@sensoray.com wrote:
> On 2023-12-13 04:03, Hans Verkuil wrote:
>> Hi Dean,
>>
>> A few more comments below.
>>
> ...
>> 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.
> 
> If this is added, it needs done very carefully. Adding it to probe will 100% break the firmware load. s2255_s_ctrl sends a USB command to the board and the asynchronous firmware load shares the
> endpoint for commands.
> 
> The initial brightness, contrast, hue, and saturation default values match what the firmware sets when first loaded. It's redundant to set them again immediately after the firmware load succeeds.  I
> think at a minimum this belongs in a separate patch.

I agree with that. If you are absolutely certain that the default values match that
of the firmware, then you can omit the setup call, but that has to be documented.

I see that the fw is loaded at open(), so v4l2_ctrl_handler_setup() should probably
be called there once the fw is successfully loaded.

Regards,

	Hans

> 
> Regards,
> 
> Dean
> 
>>
>> Regards,
>>
>>     Hans