Hi Shravan,
Quoting shravan kumar (2024-11-29 10:00:36)
> From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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@30fps */
> +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@30fps */
> +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@30fps */
> 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
>
