linux-kernel - Re: [PATCH v3 09/25] media: i2c: imx258: Add support for running on 2 CSI data lanes

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: <171247991454.2374960.14379287208228554950@ping.linuxembedded.co.uk>
Date: Sun, 07 Apr 2024 09:51:54 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Luis Garcia <git@...gi311.com>, Pavel Machek <pavel@....cz>
Cc: linux-media@...r.kernel.org, dave.stevenson@...pberrypi.com, jacopo.mondi@...asonboard.com, mchehab@...nel.org, robh@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com, sakari.ailus@...ux.intel.com, devicetree@...r.kernel.org, imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, phone-devel@...r.kernel.org
Subject: Re: [PATCH v3 09/25] media: i2c: imx258: Add support for running on 2 CSI data lanes

Quoting Luis Garcia (2024-04-06 06:25:41)
> On 4/3/24 12:45, Pavel Machek wrote:
> > Hi!
> > 
> >> +/*
> >> + * 4208x3120 @ 30 fps needs 1267Mbps/lane, 4 lanes.
> >> + * To avoid further computation of clock settings, adopt the same per
> >> + * lane data rate when using 2 lanes, thus allowing a maximum of 15fps.
> >> + */
> >> +static const struct imx258_reg mipi_1267mbps_19_2mhz_2l[] = {
> >> +    { 0x0136, 0x13 },
> >> +    { 0x0137, 0x33 },
> >> +    { 0x0301, 0x0A },
> >> +    { 0x0303, 0x02 },
> >> +    { 0x0305, 0x03 },
> >> +    { 0x0306, 0x00 },
> >> +    { 0x0307, 0xC6 },
> >> +    { 0x0309, 0x0A },
> >> +    { 0x030B, 0x01 },
> >> +    { 0x030D, 0x02 },
> >> +    { 0x030E, 0x00 },
> >> +    { 0x030F, 0xD8 },
> >> +    { 0x0310, 0x00 },
> >> +
> >> +    { 0x0114, 0x01 },
> >> +    { 0x0820, 0x09 },
> >> +    { 0x0821, 0xa6 },
> >> +    { 0x0822, 0x66 },
> >> +    { 0x0823, 0x66 },
> >> +};
> >> +
> >> +static const struct imx258_reg mipi_1267mbps_19_2mhz_4l[] = {
> >>      { 0x0136, 0x13 },
> >>      { 0x0137, 0x33 },
> >>      { 0x0301, 0x05 },
> > 
> > I wish we did not have to copy all the magic values like this.
> > 
> > Best regards,
> >                                                               Pavel
> >                                                               
> 
> no kidding, magic values everywhere.... it makes it annoying
> for me to move things around because they all start to look
> similar. Down the line we added in more defined names so its
> not as bad but still its bad lol.

This series converts the defines to names, which is great. It would have
been nicer if the series converted first, but I know the history here
means you have done the register naming on top of existing patches - so
I don't think there's a requirement to change the ordering now.

But I see new drivers coming in with register tables. I hope we can
start to apply more pressure to driver submitters to use higher quality
named register sets in the future, now that we have a greater precendent
of sensor drivers 'doing the right thing'.

Sets of tables like we have are basically a binary blob stored as ascii
and make maintainance far more difficult IMO.

Maybe I should hit send on my comments on the latest GalaxyCore driver
coming in that I hesitated on ...
--
Kieran