Re: [PATCH v3 2/3] media: i2c: add imx576 image sensor driver
From: Himanshu Bhavani
Date: Tue Aug 11 2026 - 05:22:22 EST
Hi Jai,
>Hi Himanshu,
>
>Thanks for the patch.
>
>Not a full review, but just a few comments on the PLL programming.
>
>Quoting Himanshu Bhavani (2026-08-06 17:32:08)
>> Add a v4l2 subdevice driver for the Sony imx576 sensor.
>>
>> The Sony IMX576 image sensor with an active
>> array size of 5760 x 4312
>>
>> The following features are supported:
>> - Manual exposure an gain control support
>> - vblank/hblank control support
>> - Supported resolution: 5760 x 4312 30fps (SRGGB10)
>>
>> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
>
>[...]
>
>> +#define IMX576_INCLK_RATE (24 * HZ_PER_MHZ)
>> +
>> +#define IMX576_REG_CHIP_ID CCI_REG16(0x0016)
>> +#define IMX576_CHIP_ID 0x0576
>> +
>> +#define IMX576_REG_MODE_SELECT CCI_REG8(0x0100)
>> +#define IMX576_MODE_STANDBY 0x00
>> +#define IMX576_MODE_STREAMING 0x01
>> +
>> +#define IMX576_REG_HOLD CCI_REG8(0x0104)
>> +
>> +#define IMX576_REG_EXPOSURE CCI_REG16(0x0202)
>> +#define IMX576_EXPOSURE_MIN 8
>> +#define IMX576_EXPOSURE_OFFSET 22
>> +#define IMX576_EXPOSURE_STEP 1
>> +#define IMX576_EXPOSURE_DEFAULT 0x0648
>> +
>> +#define IMX576_REG_ANALOG_GAIN CCI_REG16(0x0204)
>> +#define IMX576_ANA_GAIN_MIN 0
>> +#define IMX576_ANA_GAIN_MAX 978
>> +#define IMX576_ANA_GAIN_STEP 1
>> +#define IMX576_ANA_GAIN_DEFAULT 0
>> +
>> +#define IMX576_REG_IVT_PXCK_DIV CCI_REG8(0x0301)
>> +#define IMX576_REG_IVT_SYCK_DIV CCI_REG8(0x0303)
>> +#define IMX576_REG_IVT_PREPLLCK_DIV CCI_REG8(0x0305)
>> +#define IMX576_REG_IVT_PLL_MPY CCI_REG16(0x0306)
>> +#define IMX576_REG_IOP_SYCK_DIV CCI_REG8(0x030b)
>> +#define IMX576_REG_IOP_PREPLLCK_DIV CCI_REG8(0x030d)
>> +#define IMX576_REG_IOP_PLL_MPY CCI_REG16(0x030e)
>> +#define IMX576_REG_PLL_MULT_DRIV CCI_REG8(0x0310)
>> +
>> +#define IMX576_REG_FRM_LENGTH_LINES CCI_REG16(0x0340)
>> +#define IMX576_REG_LINE_LENGTH_PCK CCI_REG16(0x0342)
>> +#define IMX576_LINE_LENGTH 6144
>> +#define IMX576_VBLANK_DEF 4387
>> +
>> +/* FIXME: Exact VBLANK limit unknown (no datasheet). */
>> +#define IMX576_VBLANK_MAX 32420
>> +
>> +#define IMX576_REG_X_ADD_STA CCI_REG16(0x0344)
>> +#define IMX576_REG_Y_ADD_STA CCI_REG16(0x0346)
>> +#define IMX576_REG_X_ADD_END CCI_REG16(0x0348)
>> +#define IMX576_REG_Y_ADD_END CCI_REG16(0x034a)
>> +#define IMX576_REG_X_OUTPUT_SIZE CCI_REG16(0x034c)
>> +#define IMX576_REG_Y_OUTPUT_SIZE CCI_REG16(0x034e)
>> +
>> +#define IMX576_REG_DIG_CROP_X_OFFSET CCI_REG16(0x0408)
>> +#define IMX576_REG_DIG_CROP_Y_OFFSET CCI_REG16(0x040a)
>> +#define IMX576_REG_DIG_CROP_WIDTH CCI_REG16(0x040c)
>> +#define IMX576_REG_DIG_CROP_HEIGHT CCI_REG16(0x040e)
>
>This sensor's registers seem quite similar to the MIPI CCS specification.
Yes, you are right some registers seems to be similar I will address it.
>Please check if the sensor is CCS compatible. Even if it is not, I think
>the PLL programming is compatible, so you can use the helpers from
>ccs-pll.c.
Most PLL values are derived from the limited IMX576 datasheet information available.
For the unknown values, I referred to out of tree driver and validated and assumed them based on the PLL configuration.
Should I proceed with the PLL programming as it is, or drop the PLL approach and retain only the CCS-supported registers?
static const struct ccs_pll_limits imx576_pll_limits = {
.min_ext_clk_freq_hz = 6000000,
.max_ext_clk_freq_hz = 27000000,
.vt_fr = {
/* Confirmed values as per datasheet */
.min_pre_pll_clk_div = 1,
.max_pre_pll_clk_div = 4,
.min_pll_ip_clk_freq_hz = 6000000,
.max_pll_ip_clk_freq_hz = 12000000,
.min_pll_multiplier = 87,
.max_pll_multiplier = 350,
.min_pll_op_clk_freq_hz = 1050000000,
.max_pll_op_clk_freq_hz = 2100000000,
},
.vt_bk = {
/* here the sys_clk_div (IVT_SYCK_DIV -> 0303) value as per out of tree driver they are using div as 2 or 4 */
.min_sys_clk_div = 2,
.max_sys_clk_div = 4,
.min_sys_clk_freq_hz = 262500000,
.max_sys_clk_freq_hz = 1050000000,
/* pix_clk_div also being used as an Fixed to 5 */
.min_pix_clk_div = 5,
.max_pix_clk_div = 5,
.min_pix_clk_freq_hz = 52500000,
.max_pix_clk_freq_hz = 210000000,
},
.op_fr = {
/* Confirmed values as per datasheet */
.min_pre_pll_clk_div = 1,
.max_pre_pll_clk_div = 15,
.min_pll_ip_clk_freq_hz = 6000000,
.max_pll_ip_clk_freq_hz = 12000000,
.min_pll_multiplier = 47,
.max_pll_multiplier = 2300,
.min_pll_op_clk_freq_hz = 1250000000,
.max_pll_op_clk_freq_hz = 2300000000,
},
.op_bk = {
/* Similarly here also (OP_SYS_CLK_DIV -> 030b) value being used is 1,2,4 in out of tree driver
* But as per ccs-limits { CCS_R_MAX_OP_SYS_CLK_DIV = 2 }
*/
.min_sys_clk_div = 1,
.max_sys_clk_div = 2,
.min_sys_clk_freq_hz = 625000000,
.max_sys_clk_freq_hz = 2300000000,
/* (RAW8 and RAW10) */
.min_pix_clk_div = 8,
.max_pix_clk_div = 10,
.min_pix_clk_freq_hz = 62500000,
.max_pix_clk_freq_hz = 2300000000,
},
>> +
>> +#define IMX576_REG_BINNING_MODE CCI_REG8(0x0900)
>> +#define IMX576_REG_BINNING_TYPE CCI_REG8(0x0901)
>> +#define IMX576_REG_BINNING_WEIGHT CCI_REG8(0x0902)
>> +
>> +#define IMX576_PIXEL_RATE 813600000
>> +#define IMX576_NUM_DATA_LANES 4
>> +
>> +/* IMX576 native and active pixel array size */
>> +static const struct v4l2_rect imx576_native_area = {
>> + .top = 0,
>> + .left = 0,
>> + .width = 5792,
>> + .height = 4464,
>> +};
>> +
>> +static const struct v4l2_rect imx576_active_area = {
>> + .top = 136,
>> + .left = 16,
>> + .width = 5760,
>> + .height = 4312,
>> +};
>> +
>> +static const char * const imx576_supply_names[] = {
>> + "vana", /* Analog Power */
>> + "vif", /* Interface Power */
>> + "vdig", /* Digital Power */
>> +};
>> +
>> +static const struct cci_reg_sequence imx576_common_regs[] = {
>
>[...]
>
>> + { IMX576_REG_IVT_PXCK_DIV, 0x05 },
>> + { IMX576_REG_IVT_SYCK_DIV, 0x02 },
>> + { IMX576_REG_IVT_PREPLLCK_DIV, 0x04 },
>> + { IMX576_REG_IVT_PLL_MPY, 0x0154 },
>> + { IMX576_REG_IOP_SYCK_DIV, 0x01 },
>> + { IMX576_REG_IOP_PREPLLCK_DIV, 0x04 },
>> + { IMX576_REG_PLL_MULT_DRIV, 0x01 },
>
>I think when Laurent asked to make the PLL configurable for different link
>frequencies, he meant deriving these and the OP_PLL_MPY value instead of
>hardcoding all of them and only supporting one link freq.
>> + { CCI_REG8(0x0b06), 0x01 },
>> + { CCI_REG8(0x3620), 0x01 },
>> + { CCI_REG8(0x3f0c), 0x00 },
>> +};
>> +
>
>Thanks,
> Jai
Best Regards,
Himanshu