Re: [PATCH v2 03/15] media: i2c: os05b10: add register definitions and use them in init table

[Tarang Raval]

Hi, Mehdi.

> On Wed, Mar 25, 2026 at 05:13:49PM +0530, Tarang Raval wrote:
> > Define named register macros for OS05B10 and replace raw register
> > addresses in the common initialization array with the new definitions.
> > This improves readability and maintainability without changing
> > functionality.
> >
> > Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/os05b10.c | 111 +++++++++++++++++++++++-------------
> >  1 file changed, 71 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/media/i2c/os05b10.c b/drivers/media/i2c/os05b10.c
> > index 62fb856cbdea..751494fdba6d 100644
> > --- a/drivers/media/i2c/os05b10.c
> > +++ b/drivers/media/i2c/os05b10.c
> > @@ -38,6 +38,20 @@
> >  #define OS05B10_MODE_STANDBY         0x00
> >  #define OS05B10_MODE_STREAMING               0x01
> >
> > +#define OS05B10_REG_PLL_CTRL_01              CCI_REG8(0x0301)
> > +#define OS05B10_REG_PLL_CTRL_03              CCI_REG8(0x0303)
> > +#define OS05B10_REG_PLL_CTRL_05              CCI_REG8(0x0305)
> > +#define OS05B10_REG_PLL_CTRL_06              CCI_REG8(0x0306)
> > +#define OS05B10_REG_PLL_CTRL_25              CCI_REG8(0x0325)
> > +
> > +#define OS05B10_REG_MIPI_SC_CTRL     CCI_REG8(0x3016)
> > +#define OS05B10_4_LANE_MODE          0x72
> > +#define OS05B10_2_LANE_MODE          0x32
> > +
> > +#define OS05B10_REG_MIPI_SC_CTRL_1   CCI_REG8(0x3022)
> > +#define OS05B10_10BIT_MODE           0x01
> > +#define OS05B10_12BIT_MODE           0x61
> > +
> >  #define OS05B10_REG_EXPOSURE         CCI_REG24(0x3500)
> >  #define OS05B10_EXPOSURE_MIN         2
> >  #define OS05B10_EXPOSURE_STEP                1
> > @@ -49,11 +63,42 @@
> >  #define OS05B10_ANALOG_GAIN_STEP     1
> >  #define OS05B10_ANALOG_GAIN_DEFAULT  0x80
> >
> > +#define OS05B10_REG_DIGITAL_GAIN     CCI_REG16(0x350a)
> > +#define OS05B10_DIGITAL_GAIN_MIN     0x400
> > +#define OS05B10_DIGITAL_GAIN_MAX     0x3fff
> > +#define OS05B10_DIGITAL_GAIN_STEP    16
> > +#define OS05B10_DIGITAL_GAIN_DEFAULT 0x400
> > +
> > +#define OS05B10_REG_ANALOG_GAIN_SHORT        CCI_REG16(0x350c)
> > +#define OS05B10_REG_DIGITAL_GAIN_SHORT       CCI_REG16(0x350e)
> > +#define OS05B10_REG_EXPOSURE_SHORT   CCI_REG24(0x3510)
> > +
> > +#define OS05B10_REG_X_ADDR_START     CCI_REG16(0x3800)
> > +#define OS05B10_REG_Y_ADDR_START     CCI_REG16(0x3802)
> > +#define OS05B10_REG_X_ADDR_END               CCI_REG16(0x3804)
> > +#define OS05B10_REG_Y_ADDR_END               CCI_REG16(0x3806)
> > +#define OS05B10_REG_X_OUTPUT_SIZE    CCI_REG16(0x3808)
> > +#define OS05B10_REG_Y_OUTPUT_SIZE    CCI_REG16(0x380a)
> > +
> >  #define OS05B10_REG_HTS                      CCI_REG16(0x380c)
> >
> >  #define OS05B10_REG_VTS                      CCI_REG16(0x380e)
> >  #define OS05B10_VTS_MAX                      0x7fff
> >
> > +#define OS05B10_REG_ISP_X_WIN                CCI_REG16(0x3810)
> > +#define OS05B10_REG_ISP_Y_WIN                CCI_REG16(0x3812)
> > +#define OS05B10_REG_X_INC_ODD                CCI_REG8(0x3814)
> > +#define OS05B10_REG_X_INC_EVEN               CCI_REG8(0x3815)
> > +#define OS05B10_REG_Y_INC_ODD                CCI_REG8(0x3816)
> > +#define OS05B10_REG_Y_INC_EVEN               CCI_REG8(0x3817)
> > +
> > +#define OS05B10_REG_FORMAT1          CCI_REG8(0x3820)
> > +#define OS05B10_MIRROR                       BIT(3)
> > +#define OS05B10_FLIP                 GENMASK(5, 4)
> > +
> > +#define OS05B10_REG_FORMAT2          CCI_REG8(0x3821)
> > +#define OS05B10_HDR_ENABLE           0x04
>
> Is this BIT(2) ? The define is also not used.

You are right. It could be written as BIT(2), but since it's currently
unused, I will remove it in the next revision.

> > +
> >  #define OS05B10_LINK_FREQ_600MHZ     (600 * HZ_PER_MHZ)
> >
> >  static const struct v4l2_rect os05b10_native_area = {
> > @@ -77,30 +122,25 @@ static const char * const os05b10_supply_name[] = {
> >  };
> >
> >  static const struct cci_reg_sequence os05b10_common_regs[] = {
> > -     { CCI_REG8(0x0301), 0x44 },
> > -     { CCI_REG8(0x0303), 0x02 },
> > -     { CCI_REG8(0x0305), 0x32 },
> > -     { CCI_REG8(0x0306), 0x00 },
> > -     { CCI_REG8(0x0325), 0x3b },
> > +     { OS05B10_REG_PLL_CTRL_01, 0x44 },
> > +     { OS05B10_REG_PLL_CTRL_03, 0x02 },
> > +     { OS05B10_REG_PLL_CTRL_05, 0x32 },
> > +     { OS05B10_REG_PLL_CTRL_06, 0x00 },
> > +     { OS05B10_REG_PLL_CTRL_25, 0x3b },
> >       { CCI_REG8(0x3002), 0x21 },
> > -     { CCI_REG8(0x3016), 0x72 },
> > +     { OS05B10_REG_MIPI_SC_CTRL, 0x72 },
> >       { CCI_REG8(0x301e), 0xb4 },
> >       { CCI_REG8(0x301f), 0xd0 },
> >       { CCI_REG8(0x3021), 0x03 },
> > -     { CCI_REG8(0x3022), 0x01 },
> > +     { OS05B10_REG_MIPI_SC_CTRL_1, 0x01 },
> >       { CCI_REG8(0x3107), 0xa1 },
> >       { CCI_REG8(0x3108), 0x7d },
> >       { CCI_REG8(0x3109), 0xfc },
> >       { CCI_REG8(0x3503), 0x88 },
> > -     { CCI_REG8(0x350a), 0x04 },
> > -     { CCI_REG8(0x350b), 0x00 },
> > -     { CCI_REG8(0x350c), 0x00 },
> > -     { CCI_REG8(0x350d), 0x80 },
> > -     { CCI_REG8(0x350e), 0x04 },
> > -     { CCI_REG8(0x350f), 0x00 },
> > -     { CCI_REG8(0x3510), 0x00 },
> > -     { CCI_REG8(0x3511), 0x00 },
> > -     { CCI_REG8(0x3512), 0x20 },
> > +     { OS05B10_REG_DIGITAL_GAIN, 0x0400 },
>
> Are you setting the register to OS05B10_REG_DIGITAL_GAIN_DEFAULT ? It is
> defined above

In patch 4/15, this entry will be removed.

However, you are right. For consistency, I will use
OS05B10_REG_DIGITAL_GAIN_DEFAULT here.

Best Regards,
Tarang