On Mon, Jan 08, 2024 at 12:52:44PM +0000, Sakari Ailus wrote:
> Hi Dean,
>
> Thanks for the patch.
>
> On Fri, Dec 15, 2023 at 11:14:21AM -0800, 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.
> > Comment added noting v4l2_ctrl_handler_setup is not needed for this driver.
> >
> > Signed-off-by: Dean Anderson <dean@xxxxxxxxxxxx>
> >
> > ---
> > drivers/media/usb/s2255/s2255drv.c | 52 +++++++++++++++++++++++++++++-
> > 1 file changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
> > index 3c2627712fe9..1f3961835711 100644
> > --- a/drivers/media/usb/s2255/s2255drv.c
> > +++ b/drivers/media/usb/s2255/s2255drv.c
> > @@ -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)
>
> It'd be nicer to convert the value obtained from the device to CPU
> endianness rather than the other way around. But as this seems to be a
> pattern already used in the driver, I'm fine with it here.
>
> Given that the serial number itself is big endian, I wonder if the driver
> is just implemented in a funny way and happens to work?
>
> > #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)
>
> I'd document a new standardised control for this purpose. Any other
> opinions?
How about reporting the serial number through struct
media_device_info.serial instead ?
> >
> > /* frame prefix size (sent once every frame) */
> > #define PREFIX_SIZE 512
> > @@ -1232,6 +1234,36 @@ 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.
> > + */
> > +
> > +static int s2255_g_serialnum(struct s2255_dev *dev)
> > +{
> > +#define S2255_SERIALNUM_NONE 0
>
> No need for such a definition, just assign it to zero.
>
> > +#define S2255_I2C_SIZE 16
> > +#define S2255_I2C_SERIALNUM 0xa2
> > +#define S2255_I2C_SERIALNUM_OFFSET 0x1ff0
> > +#define S2255_VENDOR_READREG 0x22
> > +
> > + u8 *buf;
> > + int serialnum = S2255_SERIALNUM_NONE;
>
> u32?
>
> > +
> > + buf = kzalloc(S2255_I2C_SIZE, GFP_KERNEL);
>
> Could this reside in the stack instead? It's just 16 bytes.
>
> > + if (!buf)
> > + return serialnum;
> > +
> > + s2255_vendor_req(dev, S2255_VENDOR_READREG, S2255_I2C_SERIALNUM_OFFSET,
> > + S2255_I2C_SERIALNUM >> 1, buf, S2255_I2C_SIZE, 0);
>
> It'd be nice to check for errors here.
>
> > +
> > + /* verify marker code */
> > + if (*(__le32 *)buf == S2255_MARKER_FIRMWARE)
> > + serialnum = (buf[12] << 24) + (buf[13] << 16) + (buf[14] << 8) + buf[15];
>
> Maybe:
>
> serialnum = be32_to_cpu(*(__be32 *)(buf + 12));
>
> You could add a reference to the serial number as a function argument and
> just return the error if one happens. This isn't exactly the same thing as
> having serial number 0.
>
> > + kfree(buf);
> > + return serialnum;
> > +}
> > +
> > static int vidioc_g_jpegcomp(struct file *file, void *priv,
> > struct v4l2_jpegcompression *jc)
> > {
> > @@ -1581,6 +1613,17 @@ static const struct v4l2_ctrl_config color_filter_ctrl = {
> > .def = 1,
> > };
> >
> > +static const struct v4l2_ctrl_config v4l2_ctrl_serialnum = {
> > + .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;
> > @@ -1588,6 +1631,7 @@ static int s2255_probe_v4l(struct s2255_dev *dev)
> > int cur_nr = video_nr;
> > struct s2255_vc *vc;
> > struct vb2_queue *q;
> > + struct v4l2_ctrl_config tmp = v4l2_ctrl_serialnum;
> >
> > ret = v4l2_device_register(&dev->interface->dev, &dev->v4l2_dev);
> > if (ret)
> > @@ -1598,7 +1642,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 +1659,8 @@ 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);
> > + tmp.def = s2255_g_serialnum(dev);
> > + v4l2_ctrl_new_custom(&vc->hdl, &tmp, NULL);
> > if (vc->hdl.error) {
> > ret = vc->hdl.error;
> > v4l2_ctrl_handler_free(&vc->hdl);
> > @@ -2306,6 +2352,10 @@ static int s2255_probe(struct usb_interface *interface,
> > retval = s2255_board_init(dev);
> > if (retval)
> > goto errorBOARDINIT;
> > + /*
> > + * v4l2_ctrl_handler_setup is not required.
> > + * V4L2 controls initialized when firmware is loaded.
> > + */
>
> I'd drop the comment.
>
> > s2255_fwload_start(dev);
> > /* loads v4l specific */
> > retval = s2255_probe_v4l(dev);
--
Regards,
Laurent Pinchart
