Re: [PATCH v3 2/3] media: i2c: add imx576 image sensor driver

From: Jai Luthra

Date: Fri Aug 14 2026 - 08:59:36 EST

Quoting Himanshu Bhavani (2026-08-14 17:05:12)
> Hi Jai,
>
> >Hi Himanshu,
> >
> > Quoting Himanshu Bhavani (2026-08-11 14:44:12)
> > > 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.
> > >
> >
> > That's good to know. Did you check if the sensor itself advertises itself
> > as compliant to a particular version of the CCS standard? You can read the
> > below register:
> >
> > #define CCS_R_MIPI_CCS_VERSION CCI_REG8(0x0007)
> >
> > In general it might help to take a dump of all the CCS registers to see if
> > the ranges given in datasheet match what the sensor's registers say.
> >
>
> I tried reading CCS_R_MIPI_CCS_VERSION (CCI_REG8(0x0007)), but it
> returned 0x00.
> it is expected to return either CCS_MIPI_CCS_VERSION_V1_0 or
> CCS_MIPI_CCS_VERSION_V1_1 ?
>

Not necessarily, it's the same as IMX708 which also returned 0x00.

I interpreted this to mean the sensor isn't compliant to the CCS
specification v1.0 or v1.1.

It supports the same register layout, but some features might make it
incompatible with the spec.

> I have shared you the minimal datasheet in personal,
>
> Could you please clarify how you verified the IMX708 ? Is 0x0007
> documented as the MIPI CCS Version register?

It wasn't documented for IMX708 either. But dumping all CCS registers
(present in ccs-regs.h) worked for IMX708. I also ended up reusing the
register definitions by including ccs-regs.h in my driver, rather than
re-defining all macros.

>
>
> Because I tried applyig the imx576_pll_limits{..} PLL calculation and
> updating the corresponding registers, streaming works fine to me.
> readback those pll related registers confirms they are set as expected by
> the ccs-pll.c calculation.
>

That's good, I would suggest going forward with this rather than hardcoded
values.

Thanks,
Jai

> > >
> > > >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?
> > >
> >
> > I'm not sure I fully understand your question.
> >
> > If you can support more than 1 link frequency using the ccs_pll_calculate()
> > helper, then my suggestion would be to do that, even if it's just 2
> > well-tested frequencies today.
> >
> > That leaves the driver in much better shape for future (for use with a
> > different input clock or frequency). At least that's what I have done for
> > my IMX708 series, which was similar to this sensor.
> >
> > Thanks,
> > Jai
> >
> > > 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
>
> Best Regards,
> Himanshu