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

[Jai Luthra <jai.luthra@ideasonboard.com>]

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@siliconsignals.io>

[...]

> +#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.

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.

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