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

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

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.

> 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 ?
:-)

> > 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),

-- 
Regards,

Laurent Pinchart