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: <D8CU9UT1XQV9.3NFZ4OAQOKQG0@d3embedded.com>
Date: Mon, 10 Mar 2025 15:39:02 -0400
From: "Sebastian LaVine" <slavine@...mbedded.com>
To: "Laurent Pinchart" <laurent.pinchart@...asonboard.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

On Thu Feb 13, 2025 at 5:19 AM EST, Laurent Pinchart wrote:
>
> ...
>
>> +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 ?
>

In my testing, it takes around two seconds to write this table to the sensor.

I can investigate how to condense the table further, though the
registers for this sensor are more complex than just writing values to
addresses. The meaning of certain address writes depend on previous
writes -- thus the "duplicated" writes you mentioned.

I do not believe this table contains tuning information for our camera
module in particular.

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

Okay, I will drop these functions in v4. Thanks.

--
Sebastian

Please be aware that this email includes email addresses outside of the organization.