Re: [PATCH 06/11] media: i2c: ov5640: split out the LSC registers

[Kieran Bingham <kieran.bingham@ideasonboard.com>]

Quoting Jacopo Mondi (2026-05-14 09:23:50)
> Hi Kieran
> 
> On Fri, May 01, 2026 at 04:39:08PM +0100, Kieran Bingham wrote:
> > Lens shading is a characteristic which is specific to the lens of a
> > given module.
> >
> > Separate the Lens Shading Calibration registers from the init_setting
> > to identify the registers which must be updated when changing a lens.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> >
> > The ordering of the table entries here are maintained and not resorted
> > (i.e. that first line) to make it easy to confirm that no adjustment is
> > made to the data here.
> >
> > I've also moved the LSC 'above' the init table as otherwise the diff
> > becomes unreviewable - as it shows a different hunk being conceptually
> > moved instead and hides the ability to see what is moving.
> >
> > Though that itself also confirms that the values don't change here, so
> > I'm happy to move it down if requested.
> > ---
> >  drivers/media/i2c/ov5640.c | 51 +++++++++++++++++++++++++++-------------------
> >  1 file changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index e1e253730206..b4e1ec4364df 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -522,6 +522,31 @@ static const struct v4l2_mbus_framefmt ov5640_dvp_default_fmt = {
> >       .field = V4L2_FIELD_NONE,
> >  };
> >
> > +static const struct reg_value ov5640_lsc[] = {
> > +     /* Lens Shading Correction */
> > +     {0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> > +     {0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> > +     {0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> > +     {0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> > +     {0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> > +     {0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> > +     {0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> > +     {0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> > +     {0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> > +     {0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> > +     {0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> > +     {0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> > +     {0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> > +     {0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> > +     {0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> > +     {0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> > +     {0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> > +     {0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> > +     {0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> > +     {0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> > +     {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> > +};
> > +
> >  static const struct reg_value ov5640_init_setting[] = {
> >       {0x3103, 0x11, 0, 0},
> >       {0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
> > @@ -574,27 +599,8 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x548d, 0xcd, 0, 0}, {0x548e, 0xdd, 0, 0}, {0x548f, 0xea, 0, 0},
> >       {0x5490, 0x1d, 0, 0}, {0x5580, 0x02, 0, 0}, {0x5583, 0x40, 0, 0},
> >       {0x5584, 0x10, 0, 0}, {0x5589, 0x10, 0, 0}, {0x558a, 0x00, 0, 0},
> > -     {0x558b, 0xf8, 0, 0}, {0x5800, 0x23, 0, 0}, {0x5801, 0x14, 0, 0},
> > -     {0x5802, 0x0f, 0, 0}, {0x5803, 0x0f, 0, 0}, {0x5804, 0x12, 0, 0},
> > -     {0x5805, 0x26, 0, 0}, {0x5806, 0x0c, 0, 0}, {0x5807, 0x08, 0, 0},
> > -     {0x5808, 0x05, 0, 0}, {0x5809, 0x05, 0, 0}, {0x580a, 0x08, 0, 0},
> > -     {0x580b, 0x0d, 0, 0}, {0x580c, 0x08, 0, 0}, {0x580d, 0x03, 0, 0},
> > -     {0x580e, 0x00, 0, 0}, {0x580f, 0x00, 0, 0}, {0x5810, 0x03, 0, 0},
> > -     {0x5811, 0x09, 0, 0}, {0x5812, 0x07, 0, 0}, {0x5813, 0x03, 0, 0},
> > -     {0x5814, 0x00, 0, 0}, {0x5815, 0x01, 0, 0}, {0x5816, 0x03, 0, 0},
> > -     {0x5817, 0x08, 0, 0}, {0x5818, 0x0d, 0, 0}, {0x5819, 0x08, 0, 0},
> > -     {0x581a, 0x05, 0, 0}, {0x581b, 0x06, 0, 0}, {0x581c, 0x08, 0, 0},
> > -     {0x581d, 0x0e, 0, 0}, {0x581e, 0x29, 0, 0}, {0x581f, 0x17, 0, 0},
> > -     {0x5820, 0x11, 0, 0}, {0x5821, 0x11, 0, 0}, {0x5822, 0x15, 0, 0},
> > -     {0x5823, 0x28, 0, 0}, {0x5824, 0x46, 0, 0}, {0x5825, 0x26, 0, 0},
> > -     {0x5826, 0x08, 0, 0}, {0x5827, 0x26, 0, 0}, {0x5828, 0x64, 0, 0},
> > -     {0x5829, 0x26, 0, 0}, {0x582a, 0x24, 0, 0}, {0x582b, 0x22, 0, 0},
> > -     {0x582c, 0x24, 0, 0}, {0x582d, 0x24, 0, 0}, {0x582e, 0x06, 0, 0},
> > -     {0x582f, 0x22, 0, 0}, {0x5830, 0x40, 0, 0}, {0x5831, 0x42, 0, 0},
> > -     {0x5832, 0x24, 0, 0}, {0x5833, 0x26, 0, 0}, {0x5834, 0x24, 0, 0},
> > -     {0x5835, 0x22, 0, 0}, {0x5836, 0x22, 0, 0}, {0x5837, 0x26, 0, 0},
> > -     {0x5838, 0x44, 0, 0}, {0x5839, 0x24, 0, 0}, {0x583a, 0x26, 0, 0},
> > -     {0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
> > +     {0x558b, 0xf8, 0, 0},
> > +
> 
> I'm wondering if the empty line here is intentional.

Yes, to start separating the functional blocks. I think we can work a
lot more to break out this whole register table with the full datasheet
being 'public'.


> Regardless:
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> >       {0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
> >       {0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> >       {0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> > @@ -2396,6 +2402,9 @@ static int ov5640_restore_mode(struct ov5640_dev *sensor)
> >       ov5640_load_regs(sensor, ov5640_init_setting,
> >                        ARRAY_SIZE(ov5640_init_setting));
> >
> > +     /* Load the Lens Shading Correction Table */
> > +     ov5640_load_regs(sensor, ov5640_lsc, ARRAY_SIZE(ov5640_lsc));
> > +
> >       ret = ov5640_mod_reg(sensor, OV5640_REG_SYS_ROOT_DIVIDER, 0x3f,
> >                            (ilog2(OV5640_SCLK2X_ROOT_DIV) << 2) |
> >                            ilog2(OV5640_SCLK_ROOT_DIV));
> >
> > --
> > 2.52.0
> >
> >