linux-kernel - Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250213101903.GH5888@pendragon.ideasonboard.com>
Date: Thu, 13 Feb 2025 12:19:03 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Sebastian LaVine <slavine@...mbedded.com>
Cc: devicetree@...r.kernel.org, imx@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org,
	Nícolas F. R. A. Prado <nfraprado@...labora.com>,
	Abel Vesa <abel.vesa@...aro.org>,
	Achath Vaishnav <vaishnav.a@...com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Ard Biesheuvel <ardb@...nel.org>,
	Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
	Biju Das <biju.das.jz@...renesas.com>,
	Bjorn Andersson <quic_bjorande@...cinc.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Conor Dooley <conor+dt@...nel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
	Elinor Montmasson <elinor.montmasson@...oirfairelinux.com>,
	Fabio Estevam <festevam@...il.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Hans Verkuil <hverkuil@...all.nl>,
	Javier Carrasco <javier.carrasco@...fvision.net>,
	Jianzhong Xu <xuj@...com>,
	Julien Massot <julien.massot@...labora.com>,
	Kieran Bingham <kieran.bingham@...asonboard.com>,
	Kory Maincent <kory.maincent@...tlin.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Mikhail Rudenko <mike.rudenko@...il.com>,
	Nishanth Menon <nm@...com>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Rob Herring <robh@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Stuart Burtner <sburtner@...mbedded.com>,
	Tero Kristo <kristo@...nel.org>, Thakkar Devarsh <devarsht@...com>,
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Umang Jain <umang.jain@...asonboard.com>,
	Vignesh Raghavendra <vigneshr@...com>,
	Will Deacon <will@...nel.org>, Zhi Mao <zhi.mao@...iatek.com>
Subject: Re: [PATCH 2/4] media: i2c: Add driver for Sony IMX728

Hi Sebastian,

Thank you for the patch.

I'll start with a partial review.

On Wed, Feb 12, 2025 at 02:56:54PM -0500, Sebastian LaVine wrote:
> Adds a driver for the Sony IMX728 image sensor.
> 
> Signed-off-by: Sebastian LaVine <slavine@...mbedded.com>
> Mentored-by: Stuart Burtner <sburtner@...mbedded.com>
> ---
>  MAINTAINERS                  |    1 +
>  arch/arm64/configs/defconfig |    1 +
>  drivers/media/i2c/Kconfig    |   12 +
>  drivers/media/i2c/Makefile   |    1 +
>  drivers/media/i2c/imx728.c   | 9655 ++++++++++++++++++++++++++++++++++
>  5 files changed, 9670 insertions(+)
>  create mode 100644 drivers/media/i2c/imx728.c

[snip]

> diff --git a/drivers/media/i2c/imx728.c b/drivers/media/i2c/imx728.c
> new file mode 100644
> index 000000000000..75120ca01ce6
> --- /dev/null
> +++ b/drivers/media/i2c/imx728.c
> @@ -0,0 +1,9655 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sony IMX728 CMOS Image Sensor Driver
> + *
> + * Copyright (c) 2024-2025 Define Design Deploy Corp
> + */

[snip]

> +static const struct cci_reg_sequence imx728_wdr_12bit_3856x2176[] = {

This table is way too big, with over 8000 entries. Some are even
duplicated, with identical or different values for the same register. It
will take more than a second at 400kHz to program this.

At the very least I would expect a way to compact the table and make use
of I2C register address auto-increment. Default power-up values should
also likely be just dropped.

I haven't checked in details, but doesn't this table also contain tuning
data for your specific camera ?

[snip]

> +};

[snip]

> +static int imx728_get_frame_interval(struct v4l2_subdev *sd,
> +                                    struct v4l2_subdev_state *sd_state,
> +                                    struct v4l2_subdev_frame_interval *fi)
> +{
> +       struct imx728 *imx728 = to_imx728(sd);
> +
> +       fi->interval.numerator = 1;
> +       fi->interval.denominator = imx728->fps;
> +       return 0;
> +}
> +
> +static int imx728_set_frame_interval(struct v4l2_subdev *sd,
> +                                    struct v4l2_subdev_state *sd_state,
> +                                    struct v4l2_subdev_frame_interval *fi)
> +{
> +       struct imx728 *imx728 = to_imx728(sd);
> +       u32 req_fps;
> +
> +       mutex_lock(&imx728->lock);
> +
> +       if (fi->interval.numerator == 0 || fi->interval.denominator == 0) {
> +               fi->interval.denominator = IMX728_FRAMERATE_DEFAULT;
> +               fi->interval.numerator = 1;
> +       }
> +
> +       req_fps = clamp_val(DIV_ROUND_CLOSEST(fi->interval.denominator,
> +                                             fi->interval.numerator),
> +                           IMX728_FRAMERATE_MIN, IMX728_FRAMERATE_MAX);
> +
> +       fi->interval.numerator = 1;
> +       fi->interval.denominator = req_fps;
> +
> +       imx728->fps = req_fps;
> +
> +       mutex_unlock(&imx728->lock);
> +       dev_dbg(imx728->dev, "%s frame rate = %d\n", __func__, imx728->fps);
> +
> +       return 0;
> +}

The frame rate on raw sensors is controlled through h/v blanking. You
can drop thse functions, especially given that imx728->fps isn't used
anywhere else.

-- 
Regards,

Laurent Pinchart