From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from omta035.useast.a.cloudfilter.net (omta035.useast.a.cloudfilter.net [44.202.169.34]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2BEAE2C6A0 for ; Thu, 14 Dec 2023 22:23:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=sensoray.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=sensoray.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=sensoray.com header.i=@sensoray.com header.b="BHyoHg/f" Received: from eig-obgw-6004a.ext.cloudfilter.net ([10.0.30.197]) by cmsmtp with ESMTPS id Dq0CrGds2gpyEDu5Qror1Y; Thu, 14 Dec 2023 22:21:44 +0000 Received: from gator3086.hostgator.com ([50.87.144.121]) by cmsmtp with ESMTPS id Du5PrQ5kLRGmSDu5Prna9F; Thu, 14 Dec 2023 22:21:43 +0000 X-Authority-Analysis: v=2.4 cv=efcuwpIH c=1 sm=1 tr=0 ts=657b7ff7 a=qMXOcmIMY6YlrKEg1GzxDg==:117 a=QsTHvn2EeHXCImuSLmd++Q==:17 a=OWjo9vPv0XrRhIrVQ50Ab3nP57M=:19 a=dLZJa+xiwSxG16/P+YVxDGlgEgI=:19 a=kj9zAlcOel0A:10 a=e2cXIFwxEfEA:10 a=6kiSLZGAxYIA:10 a=wXneSEFuAAAA:8 a=FnjLlJzlEcOYaycGDHYA:9 a=CjuIK1q_8ugA:10 a=YVKGGmaMxpnpCiYzuRtG:22 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sensoray.com; s=default; h=Content-Transfer-Encoding:Content-Type: Message-ID:References:In-Reply-To:Subject:Cc:To:From:Date:MIME-Version:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=gkI0lHWduau2geKhPB7egGmBn7VdNk2wOdKaGYHCA/E=; b=BHyoHg/fAaHOt1rRhzrvCNdk96 Eb/bjvlCKCHs+hI6koCrKFN/DWcOQ4KeJQZILXSOKMhqA9KMLcny67k2EOVIuKdWeEHIqC2v/xFvG 4Q6uYuwEhMTlQw2kdT9bWlcasUdo6i5bjvPxUQwwh9cv2ZmbUotm63YR7ONQHLdKKB7c=; Received: from gator3086.hostgator.com ([50.87.144.121]:30044) by gator3086.hostgator.com with esmtpa (Exim 4.95) (envelope-from ) id 1rDu5P-0039CP-7J; Thu, 14 Dec 2023 16:21:43 -0600 Received: from mail.thomaswright.com ([50.126.89.90]) by www.sensoray.com with HTTP (HTTP/1.1 POST); Thu, 14 Dec 2023 16:21:42 -0600 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Date: Thu, 14 Dec 2023 16:21:42 -0600 From: dean@sensoray.com To: Hans Verkuil Cc: linux-media@vger.kernel.org Subject: Re: [PATCH] media: usb: s2255: add serial number V4L2_CID In-Reply-To: <917c4e00-28bd-416f-9be7-b87717b3cf2f@xs4all.nl> References: <20231208223815.130450-1-dean@sensoray.com> <917c4e00-28bd-416f-9be7-b87717b3cf2f@xs4all.nl> User-Agent: Roundcube Webmail/1.4.12 Message-ID: X-Sender: dean@sensoray.com Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - gator3086.hostgator.com X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - sensoray.com X-BWhitelist: no X-Source-IP: 50.87.144.121 X-Source-L: Yes X-Exim-ID: 1rDu5P-0039CP-7J X-Source: X-Source-Args: X-Source-Dir: X-Source-Sender: gator3086.hostgator.com [50.87.144.121]:30044 X-Source-Auth: dean@sensoray.com X-Email-Count: 1 X-Org: HG=hgshared;ORG=hostgator; X-Source-Cap: c2Vuc29yYXk7c2Vuc29yYXk7Z2F0b3IzMDg2Lmhvc3RnYXRvci5jb20= X-Local-Domain: yes X-CMAE-Envelope: MS4xfDA4lipuk6Q3eR7hW/5LYjas715/HXuEse3xvARnFIWBaVppBvr7j0las0fhsP4MJeISbuqek0tyDc5RJ10/dtFfiDuNEb44pkx5CWZaRGU9h7VaTBGF Mu3xdTIiPkzI7xQXwdMPC6P5Tp2Sai2JyHEzbP6jV72natT6zxKds5hXt+PQxzRLcWMWA5GF4ZwCOSdbiRYAkcIEvESIYR4a7NM= On 2023-12-13 04:03, Hans Verkuil wrote: > 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 >> >> --- >> 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 >> #include >> >> -#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. > v4l2_ctrl_new_custom copies out the default before calling v4l2_ctrl_fill, but I agree about the scope. It also must be changed with const added. > 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