Re: Re: [PATCH 03/19] media: i2c: imx290: Specify HMAX values in decimal

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi Laurent,

Am Donnerstag, 21. Juli 2022, 13:31:56 CEST schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Thu, Jul 21, 2022 at 11:18:24AM +0200, Alexander Stein wrote:
> > Hi Laurent,
> > 
> > thanks for working on this, I've also some patches on top for imx327
> > support.
> I'm using an IMX327 with this driver without any need for additional
> patches :-) I'd be happy to test our patches though.

Same for me, but I feel unpleasent writing to reserved bits ;-)

> > Am Donnerstag, 21. Juli 2022, 10:35:24 CEST schrieb Laurent Pinchart:
> > > The HMAX value specifies the total line length in pixels. It's thus more
> > > readable in decimal than hexadecimal. Fix it.
> > 
> > I understand the motivation, pixels are more natural in decimal numbers,
> > but e.g. what is 4400 pixels on this sensor? It is only mentioned
> > sparsely and 3300d is not mentioned at all, 0ce4h is instead.
> 
> I'm not even sure if HMAX is expressed in pixels or in cycles of some
> internal clock different than the pixel clocks.
> 
> > I also like to have the hexadecimal numbers here as you can find them
> > directly in the datasheet. To me this seems more sensible for
> > cross-checking. Just my opinion.
> 
> It's a matter of readability of the driver code vs. matching with the
> datasheet. I have a preference for decimal numbers here, but I don't
> mind too much. What I would actually like is dropping the hmax value
> from the imx290_mode structure and computing it dynamically based on a
> hblank value set by userspace. Do you think you could help me doing so ?

Well, in my patch stack there is one adding vmax to this struct ;-)
It works without modification but this is essentially the so called 'long 
exposure operation'.
Reducing vmax on 720p increases the FPS, so this is a sensible thing to do. 
The exposure has to be recalculated from that (dynamic) vmax value.

But computing those values completely dynamically seems a sensible thing to 
do. Until it is fully understood I would prefer keeping hexadecimal values.
I mean do you know why it is 4400 pixels for 1080p and 6600 for 720p on 2 
lanes?

I know this is part of your [1] branch. So I'll post my patches once this 
series is settled.

Best regards,
Alexander

[1] https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.19/
sensors/imx

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/i2c/imx290.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 64bd43813dbf..f60a4135dc9c 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -308,7 +308,7 @@ static const struct imx290_mode
> > > imx290_modes_2lanes[] =
> > > { {
> > > 
> > >  		.width = 1920,
> > >  		.height = 1080,
> > > 
> > > -		.hmax = 0x1130,
> > > +		.hmax = 4400,
> > > 
> > >  		.link_freq_index = FREQ_INDEX_1080P,
> > >  		.data = imx290_1080p_settings,
> > >  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > 
> > > @@ -316,7 +316,7 @@ static const struct imx290_mode
> > > imx290_modes_2lanes[] =
> > > { {
> > > 
> > >  		.width = 1280,
> > >  		.height = 720,
> > > 
> > > -		.hmax = 0x19c8,
> > > +		.hmax = 6600,
> > > 
> > >  		.link_freq_index = FREQ_INDEX_720P,
> > >  		.data = imx290_720p_settings,
> > >  		.data_size = ARRAY_SIZE(imx290_720p_settings),
> > > 
> > > @@ -327,7 +327,7 @@ static const struct imx290_mode
> > > imx290_modes_4lanes[] =
> > > { {
> > > 
> > >  		.width = 1920,
> > >  		.height = 1080,
> > > 
> > > -		.hmax = 0x0898,
> > > +		.hmax = 2200,
> > > 
> > >  		.link_freq_index = FREQ_INDEX_1080P,
> > >  		.data = imx290_1080p_settings,
> > >  		.data_size = ARRAY_SIZE(imx290_1080p_settings),
> > > 
> > > @@ -335,7 +335,7 @@ static const struct imx290_mode
> > > imx290_modes_4lanes[] =
> > > { {
> > > 
> > >  		.width = 1280,
> > >  		.height = 720,
> > > 
> > > -		.hmax = 0x0ce4,
> > > +		.hmax = 3300,
> > > 
> > >  		.link_freq_index = FREQ_INDEX_720P,
> > >  		.data = imx290_720p_settings,
> > >  		.data_size = ARRAY_SIZE(imx290_720p_settings),