Re: [PATCH v2 4/4] media: rcar-csi2: Add D-PHY support for V4H

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, May 12, 2025 at 10:48:43AM +0200, Niklas Söderlund wrote:
> On 2025-05-12 09:37:48 +0200, Geert Uytterhoeven wrote:
> > On Sun, 11 May 2025 at 22:03, Niklas Söderlund wrote:
> > > On 2025-05-12 00:37:09 +0530, ALOK TIWARI wrote:
> > > > On 11-05-2025 23:17, Niklas Söderlund wrote:
> > > > > +   rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG, 0x0404);
> > > > > +   rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG, 0x040c);
> > > > > +   rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG, 0x0414);
> > > > > +   rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG, 0x041c);
> > 
> >  [...]
> > 
> > > > Instead of manually writing each call, it could use a loop ?
> > > >
> > > > for (int i = 0x0404; i <= 0x07fc; i += 0x08) {
> > > >     rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG, i);
> > >
> > > Unfortunately the values are not all sequential, see the progression
> > > 0x061c -> 0x0623 and 0x071c -> 0x0723 for example.
> > >
> > > > or if values are not strictly sequential, iterating over the array.
> > > > static const u16 register_values[]= {0x0404, 0x040c, 0x0414 etc,,}
> > > > rcsi2_write16(priv, V4H_CORE_DIG_COMMON_RW_DESKEW_FINE_MEM_REG,
> > > > register_values[i]);
> > >
> > > I agree with you, a array of values would make this look a tad less
> > > silly and would reduce the number of lines. I considered this while
> > > writing it but opted for this. My reason was as most of the register
> > > writes needed to setup the PHY are not documented in the docs I have and
> > > I wanted to keep the driver as close to the table of magic values I have
> > > to make it easy to compare driver and the limited documentation.
> > >
> > > I guess it's really a matter of style. I have no real strong opinion, if
> > > people think an array would be nicer I have no issue switching to that.
> > 
> > Have you looked at the impact on kernel size?
> 
> That is a good point, I'm sure an array would reduce the kernel size. I 
> could possibly even craft a few clever loops to to generate the values 
> as they are almost sequential. As these are magic values I had opted to 
> keep it close to the docs, but seems people prefere something else. Will 
> change this and send new version.

I recommend an array of values. That will significantly reduce the size,
while keeping the code easy to compare with the documentation. We can
try to be more clever than that in a separate patch if desired.

-- 
Regards,

Laurent Pinchart