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

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-TXG9oGKWPboh9/7i5KS7/[email protected]>
> > >
> > >[...]
> > >
> > >> +#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 ?

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?


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.

> >
> > >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