Re: [PATCH] media: usb: s2255: custom V4L2_CIDs

[dean@sensoray.com]

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