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