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] [day] [month] [year] [list]
Message-ID: <173288149590.1143262.2853735316281702828@ping.linuxembedded.co.uk>
Date: Fri, 29 Nov 2024 11:58:15 +0000
From: Kieran Bingham <kieran.bingham@...asonboard.com>
To: mchehab@...nel.org, sakari.ailus@...ux.intel.com, shravan kumar <shravan.chippa@...rochip.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, conor.dooley@...rochip.com, valentina.fernandezalanis@...rochip.com, praveen.kumar@...rochip.com, shravan.chippa@...rochip.com
Subject: Re: [PATCH] media: i2c: imx334: Add support for 1280x720 & 640x480 resolutions

Hi Shravan,

Quoting shravan kumar (2024-11-29 10:00:36)
> From: Shravan Chippa <shravan.chippa@...rochip.com>
> 
> Add support for 1280x720@30 and 640x480@30 resolutions
> 

Thanks for improving the driver, but I am weary there is a lot of work
required to clean up the imx334 driver.

Do you have the datasheet for this device?

> Signed-off-by: Shravan Chippa <shravan.chippa@...rochip.com>
> ---
>  drivers/media/i2c/imx334.c | 352 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 352 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index a544fc3df39c..b7cdebc2382e 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -167,6 +167,332 @@ static const s64 link_freq[] = {
>         IMX334_LINK_FREQ_445M,
>  };
>  
> +/* Sensor mode registers for 640x480@...ps */
> +static const struct imx334_reg mode_640x480_regs[] = {
> +       {0x3000, 0x01},
> +       {0x3018, 0x04},
> +       {0x3030, 0xca},
> +       {0x3031, 0x08},
> +       {0x3032, 0x00},
> +       {0x3034, 0x4c},
> +       {0x3035, 0x04},
> +       {0x302c, 0x70},
> +       {0x302d, 0x06},
> +       {0x302e, 0x80},
> +       {0x302f, 0x02},
> +       {0x3074, 0x48},
> +       {0x3075, 0x07},
> +       {0x308e, 0x49},
> +       {0x308f, 0x07},
> +       {0x3076, 0xe0},
> +       {0x3077, 0x01},
> +       {0x3090, 0xe0},
> +       {0x3091, 0x01},
> +       {0x3308, 0xe0},
> +       {0x3309, 0x01},
> +       {0x30d8, 0x30},
> +       {0x30d9, 0x0b},
> +       {0x30C6, 0x00},
> +       {0x30c7, 0x00},
> +       {0x30ce, 0x00},
> +       {0x30cf, 0x00},
> +       {0x304c, 0x00},
> +       {0x304e, 0x00},
> +       {0x304f, 0x00},
> +       {0x3050, 0x00},
> +       {0x30b6, 0x00},
> +       {0x30b7, 0x00},
> +       {0x3116, 0x08},
> +       {0x3117, 0x00},
> +       {0x31a0, 0x20},
> +       {0x31a1, 0x0f},
> +       {0x300c, 0x3b},
> +       {0x300d, 0x29},
> +       {0x314c, 0x29},
> +       {0x314d, 0x01},
> +       {0x315a, 0x0a},
> +       {0x3168, 0xa0},
> +       {0x316a, 0x7e},
> +       {0x319e, 0x02},
> +       {0x3199, 0x00},
> +       {0x319d, 0x00},
> +       {0x31dd, 0x03},
> +       {0x3300, 0x00},
> +       {0x341c, 0xff},
> +       {0x341d, 0x01},
> +       {0x3a01, 0x03},
> +       {0x3a18, 0x7f},
> +       {0x3a19, 0x00},
> +       {0x3a1a, 0x37},
> +       {0x3a1b, 0x00},
> +       {0x3a1c, 0x37},
> +       {0x3a1d, 0x00},
> +       {0x3a1e, 0xf7},
> +       {0x3a1f, 0x00},
> +       {0x3a20, 0x3f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x6f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x3f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x5f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x2f},
> +       {0x3a21, 0x00},
> +       {0x3078, 0x02},
> +       {0x3079, 0x00},
> +       {0x307a, 0x00},
> +       {0x307b, 0x00},
> +       {0x3080, 0x02},
> +       {0x3081, 0x00},
> +       {0x3082, 0x00},
> +       {0x3083, 0x00},
> +       {0x3088, 0x02},
> +       {0x3094, 0x00},
> +       {0x3095, 0x00},
> +       {0x3096, 0x00},
> +       {0x309b, 0x02},
> +       {0x309c, 0x00},
> +       {0x309d, 0x00},
> +       {0x309e, 0x00},
> +       {0x30a4, 0x00},
> +       {0x30a5, 0x00},
> +       {0x3288, 0x21},
> +       {0x328a, 0x02},
> +       {0x3414, 0x05},
> +       {0x3416, 0x18},
> +       {0x35Ac, 0x0e},
> +       {0x3648, 0x01},
> +       {0x364a, 0x04},
> +       {0x364c, 0x04},
> +       {0x3678, 0x01},
> +       {0x367c, 0x31},
> +       {0x367e, 0x31},
> +       {0x3708, 0x02},
> +       {0x3714, 0x01},
> +       {0x3715, 0x02},
> +       {0x3716, 0x02},
> +       {0x3717, 0x02},
> +       {0x371c, 0x3d},
> +       {0x371d, 0x3f},
> +       {0x372c, 0x00},
> +       {0x372d, 0x00},
> +       {0x372e, 0x46},
> +       {0x372f, 0x00},
> +       {0x3730, 0x89},
> +       {0x3731, 0x00},
> +       {0x3732, 0x08},
> +       {0x3733, 0x01},
> +       {0x3734, 0xfe},
> +       {0x3735, 0x05},
> +       {0x375d, 0x00},
> +       {0x375e, 0x00},
> +       {0x375f, 0x61},
> +       {0x3760, 0x06},
> +       {0x3768, 0x1b},
> +       {0x3769, 0x1b},
> +       {0x376a, 0x1a},
> +       {0x376b, 0x19},
> +       {0x376c, 0x18},
> +       {0x376d, 0x14},
> +       {0x376e, 0x0f},
> +       {0x3776, 0x00},
> +       {0x3777, 0x00},
> +       {0x3778, 0x46},
> +       {0x3779, 0x00},
> +       {0x377a, 0x08},
> +       {0x377b, 0x01},
> +       {0x377c, 0x45},
> +       {0x377d, 0x01},
> +       {0x377e, 0x23},
> +       {0x377f, 0x02},
> +       {0x3780, 0xd9},
> +       {0x3781, 0x03},
> +       {0x3782, 0xf5},
> +       {0x3783, 0x06},
> +       {0x3784, 0xa5},
> +       {0x3788, 0x0f},
> +       {0x378a, 0xd9},
> +       {0x378b, 0x03},
> +       {0x378c, 0xeb},
> +       {0x378d, 0x05},
> +       {0x378e, 0x87},
> +       {0x378f, 0x06},
> +       {0x3790, 0xf5},
> +       {0x3792, 0x43},
> +       {0x3794, 0x7a},
> +       {0x3796, 0xa1},
> +       {0x37b0, 0x37},
> +       {0x3e04, 0x0e},
> +       {0x30e8, 0x50},
> +       {0x30e9, 0x00},
> +       {0x3e04, 0x0e},
> +       {0x3002, 0x00},
> +};
> +
> +/* Sensor mode registers for 1280x720@...ps */
> +static const struct imx334_reg mode_1280x720_regs[] = {
> +       {0x3000, 0x01},
> +       {0x3018, 0x04},
> +       {0x3030, 0xca},
> +       {0x3031, 0x08},
> +       {0x3032, 0x00},
> +       {0x3034, 0x4c},
> +       {0x3035, 0x04},
> +       {0x302c, 0x30},
> +       {0x302d, 0x05},
> +       {0x302e, 0x00},
> +       {0x302f, 0x05},
> +       {0x3074, 0x84},
> +       {0x3075, 0x03},
> +       {0x308e, 0x85},
> +       {0x308f, 0x03},
> +       {0x3076, 0xd0},
> +       {0x3077, 0x02},
> +       {0x3090, 0xd0},
> +       {0x3091, 0x02},
> +       {0x3308, 0xd0},
> +       {0x3309, 0x02},
> +       {0x30d8, 0x30},
> +       {0x30d9, 0x0b},
> +       {0x30C6, 0x00},
> +       {0x30c7, 0x00},
> +       {0x30ce, 0x00},
> +       {0x30cf, 0x00},
> +       {0x304c, 0x00},
> +       {0x304e, 0x00},
> +       {0x304f, 0x00},
> +       {0x3050, 0x00},
> +       {0x30b6, 0x00},
> +       {0x30b7, 0x00},
> +       {0x3116, 0x08},
> +       {0x3117, 0x00},
> +       {0x31a0, 0x20},
> +       {0x31a1, 0x0f},
> +       {0x300c, 0x3b},
> +       {0x300d, 0x29},
> +       {0x314c, 0x29},
> +       {0x314d, 0x01},
> +       {0x315a, 0x0a},
> +       {0x3168, 0xa0},
> +       {0x316a, 0x7e},
> +       {0x319e, 0x02},
> +       {0x3199, 0x00},
> +       {0x319d, 0x00},
> +       {0x31dd, 0x03},
> +       {0x3300, 0x00},
> +       {0x341c, 0xff},
> +       {0x341d, 0x01},
> +       {0x3a01, 0x03},
> +       {0x3a18, 0x7f},
> +       {0x3a19, 0x00},
> +       {0x3a1a, 0x37},
> +       {0x3a1b, 0x00},
> +       {0x3a1c, 0x37},
> +       {0x3a1d, 0x00},
> +       {0x3a1e, 0xf7},
> +       {0x3a1f, 0x00},
> +       {0x3a20, 0x3f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x6f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x3f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x5f},
> +       {0x3a21, 0x00},
> +       {0x3a20, 0x2f},
> +       {0x3a21, 0x00},
> +       {0x3078, 0x02},
> +       {0x3079, 0x00},
> +       {0x307a, 0x00},
> +       {0x307b, 0x00},
> +       {0x3080, 0x02},
> +       {0x3081, 0x00},
> +       {0x3082, 0x00},
> +       {0x3083, 0x00},
> +       {0x3088, 0x02},
> +       {0x3094, 0x00},
> +       {0x3095, 0x00},
> +       {0x3096, 0x00},
> +       {0x309b, 0x02},
> +       {0x309c, 0x00},
> +       {0x309d, 0x00},
> +       {0x309e, 0x00},
> +       {0x30a4, 0x00},
> +       {0x30a5, 0x00},
> +       {0x3288, 0x21},
> +       {0x328a, 0x02},
> +       {0x3414, 0x05},
> +       {0x3416, 0x18},
> +       {0x35Ac, 0x0e},
> +       {0x3648, 0x01},
> +       {0x364a, 0x04},
> +       {0x364c, 0x04},
> +       {0x3678, 0x01},
> +       {0x367c, 0x31},
> +       {0x367e, 0x31},
> +       {0x3708, 0x02},
> +       {0x3714, 0x01},
> +       {0x3715, 0x02},
> +       {0x3716, 0x02},
> +       {0x3717, 0x02},
> +       {0x371c, 0x3d},
> +       {0x371d, 0x3f},
> +       {0x372c, 0x00},
> +       {0x372d, 0x00},
> +       {0x372e, 0x46},
> +       {0x372f, 0x00},
> +       {0x3730, 0x89},
> +       {0x3731, 0x00},
> +       {0x3732, 0x08},
> +       {0x3733, 0x01},
> +       {0x3734, 0xfe},
> +       {0x3735, 0x05},
> +       {0x375d, 0x00},
> +       {0x375e, 0x00},
> +       {0x375f, 0x61},
> +       {0x3760, 0x06},
> +       {0x3768, 0x1b},
> +       {0x3769, 0x1b},
> +       {0x376a, 0x1a},
> +       {0x376b, 0x19},
> +       {0x376c, 0x18},
> +       {0x376d, 0x14},
> +       {0x376e, 0x0f},
> +       {0x3776, 0x00},
> +       {0x3777, 0x00},
> +       {0x3778, 0x46},
> +       {0x3779, 0x00},
> +       {0x377a, 0x08},
> +       {0x377b, 0x01},
> +       {0x377c, 0x45},
> +       {0x377d, 0x01},
> +       {0x377e, 0x23},
> +       {0x377f, 0x02},
> +       {0x3780, 0xd9},
> +       {0x3781, 0x03},
> +       {0x3782, 0xf5},
> +       {0x3783, 0x06},
> +       {0x3784, 0xa5},
> +       {0x3788, 0x0f},
> +       {0x378a, 0xd9},
> +       {0x378b, 0x03},
> +       {0x378c, 0xeb},
> +       {0x378d, 0x05},
> +       {0x378e, 0x87},
> +       {0x378f, 0x06},
> +       {0x3790, 0xf5},
> +       {0x3792, 0x43},
> +       {0x3794, 0x7a},
> +       {0x3796, 0xa1},
> +       {0x37b0, 0x37},
> +       {0x3e04, 0x0e},
> +       {0x30e8, 0x50},
> +       {0x30e9, 0x00},
> +       {0x3e04, 0x0e},
> +       {0x3002, 0x00},
> +};

This is an enormous amount of duplicated data that could be factored
out.

These are also /very/ common against the existing mode register tables
too.

I think several things need to happen in this driver:

 1. It should be converted to use the CCI helpers.
 2. Whereever identifiable, the register names should be used instead of
    just the addresses.
 3. The common factors of these tables should be de-duplicated.

In your additions you only have differences in the following registers
from those entire tables:

+	{0x302c, 0x70},
+	{0x302d, 0x06},
+	{0x302e, 0x80},
+	{0x302f, 0x02},
+	{0x3074, 0x48},
+	{0x3075, 0x07},
+	{0x308e, 0x49},
+	{0x308f, 0x07},
+	{0x3076, 0xe0},
+	{0x3077, 0x01},
+	{0x3090, 0xe0},
+	{0x3091, 0x01},
+	{0x3308, 0xe0},
+	{0x3309, 0x01},
+	{0x30d8, 0x30},
+	{0x30d9, 0x0b},

 4. And ideally - the differences which determine the modes should be
    factored out to calculations so that we are not writing out large
    tables just to write a parameterised frame size.


I would beleive that at least steps 1 and 3 would be achievable, 2 and 4
would depend upon access to the datasheet.

Is that anything you could work on ?

Regards
--
Kieran





> +
>  /* Sensor mode registers for 1920x1080@...ps */
>  static const struct imx334_reg mode_1920x1080_regs[] = {
>         {0x3000, 0x01},
> @@ -505,6 +831,32 @@ static const struct imx334_mode supported_modes[] = {
>                         .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>                         .regs = mode_1920x1080_regs,
>                 },
> +       }, {
> +               .width = 1280,
> +               .height = 720,
> +               .hblank = 2480,
> +               .vblank = 1170,
> +               .vblank_min = 45,
> +               .vblank_max = 132840,
> +               .pclk = 297000000,
> +               .link_freq_idx = 1,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +                       .regs = mode_1280x720_regs,
> +               },
> +       }, {
> +               .width = 640,
> +               .height = 480,
> +               .hblank = 2480,
> +               .vblank = 1170,
> +               .vblank_min = 45,
> +               .vblank_max = 132840,
> +               .pclk = 297000000,
> +               .link_freq_idx = 1,
> +               .reg_list = {
> +                       .num_of_regs = ARRAY_SIZE(mode_640x480_regs),
> +                       .regs = mode_640x480_regs,
> +               },
>         },
>  };
>  
> -- 
> 2.34.1
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ