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

dean@xxxxxxxxxxxx · Wed, 22 Nov 2023 11:53:50 -0600

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

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







