Hi Alexander, On Thu, Jul 21, 2022 at 01:54:18PM +0200, Alexander Stein wrote: > Am Donnerstag, 21. Juli 2022, 13:31:56 CEST schrieb Laurent Pinchart: > > 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. vmax should be easier as that's expressed in lines, so you can more easily map it to V4L2_CID_VBLANK. > 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 think it may not expressed in pixels, but in units of some other internal clock, whose frequency is different for different numbers of lanes. > I know this is part of your [1] branch. So I'll post my patches once this > series is settled. > > [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), -- Regards, Laurent Pinchart
