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

[Kieran Bingham]

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