linux-kernel - Re: [PATCH 3/6] media: imx335: Update the native pixel array width

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <175508015948.560048.7430206645162546917@ping.linuxembedded.co.uk>
Date: Wed, 13 Aug 2025 11:15:59 +0100
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: Jai Luthra <jai.luthra@...asonboard.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, Jai Luthra <jai.luthra@...asonboard.com>
Subject: Re: [PATCH 3/6] media: imx335: Update the native pixel array width

Quoting Jai Luthra (2025-08-13 08:20:34)
> The sensor datasheet reports actual total number of pixels as 2696x2044.

Err ...

Page 2 of the IMX335LQN-D datasheet I have says:

Total number of pixels: 2704 (H) x 2104 (V) approx 5.69 M pixels
Number of Effective Pixels: 2616 (H) x 1964 (V) approx 5.14 M pixels
Number of Active Pixels: 2616 (H) x 1964 (V) approx 5.04 M pixels

Where does 2696x2044 come from ?


Argh - then on page 8 - indeed it says
Total Number of pixels 2696(H) x 2044(V) = 5.51M


In imx283.c I've moved to defining these windows as a v4l2_rect. I find
that's a nicer way to convey the rectangles specifically instead of
through #defines and then they can be directly used to report crop
rectangles:

i.e.:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/plain/drivers/media/i2c/imx283.c:

/* IMX283 native and active pixel array size. */
static const struct v4l2_rect imx283_native_area = {
	.top = 0,
	.left = 0,
	.width = 5592,
	.height = 3710,
};

static const struct v4l2_rect imx283_active_area = {
	.top = 40,
	.left = 108,
	.width = 5472,
	.height = 3648,
};

Not required - but just an idea (that obviously I like :D)


> 
> This becomes important for supporting 2x2 binning modes that can go
> beyond the current maximum pixel array width set here.
> 
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 6369bdbd2b09ba1f89c143cdf6be061820f2d051..dbf2db4bf9cbfd792ff5865264c6f465eb79a43b 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -124,10 +124,10 @@
>  #define IMX335_NUM_DATA_LANES  4
>  
>  /* IMX335 native and active pixel array size. */
> -#define IMX335_NATIVE_WIDTH            2616U
> -#define IMX335_NATIVE_HEIGHT           1964U
> -#define IMX335_PIXEL_ARRAY_LEFT                12U
> -#define IMX335_PIXEL_ARRAY_TOP         12U
> +#define IMX335_NATIVE_WIDTH            2696U

The all scan mode on page 56 doesn't show these at all.
Just 12 + 2592 + 12

But I think that's the datasheet being inconsistent/restrictive about
what it says an all scan mode could be.

It would be interesting to make a 'super resolution' output mode that
can transmit every pixel possible but not required for this series
development just for fun tests.

I'm torn here - as the datasheet changes it's points of reference to
make it's "all scan mode" essentially start at 0 which is clearly not
correct against the 'native' positions.

So ... I think I'm just going to say I think we don't believe the
datasheet as we *know* there are more pixels and we are using them so :


> +#define IMX335_NATIVE_HEIGHT           2044U
> +#define IMX335_PIXEL_ARRAY_LEFT                52U
> +#define IMX335_PIXEL_ARRAY_TOP         50U

I see you have taken the '80' extra pixels on both width and height and
divided half before and half after centering the window. I have no
information to say if it's position is otherwise so I think this is all
we can do:

Reviewed-by: Kieran Bingham <kieran.bingham@...asonboard.com>

>  #define IMX335_PIXEL_ARRAY_WIDTH       2592U
>  #define IMX335_PIXEL_ARRAY_HEIGHT      1944U
>  
> 
> -- 
> 2.50.1
>