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