Re: [PATCH v1 07/15] media: i2c: imx290: Factor out black level setting to a function

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dave,

On Wed, Nov 23, 2022 at 10:08:19AM +0000, Dave Stevenson wrote:
> On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote:
> >
> > The black level programmed in the BLKLEVEL register depends on the
> > output format. The black level value computation is currently performed
> > in imx290_set_ctrl(), in addition to having different black level values
> > in the output-specific register value tables. Move it to a separate
> > function to simplify the imx290_set_ctrl() code.
> 
> Does this actually help?
> The sensor only supports 10 and 12 bpp readout, so making it generic
> for any bpp is rather redundant.
> If you are looking at having different on-sensor black level values
> kicking around, then tuning data goes out the window.

This may be needed for some use cases, but that's not what I'm after
here. By the way, could you explain why we can't just set it to 0 in the
sensor and avoid subtracting it in the ISP (at least for the data
pedestal value that is added on purpose in the sensor, there can also be
an additional black level due to other reasons) ? I'm sure there's a
reason :-)

> OK we do appear to have duplication between imx290_[10|12]bit_settings
> and imx290_set_ctrl, but I don't get the need to compute it.

The idea was to centralize black level setting in a single place, to
prepare for the removal of he imx290->bpp field. It's really in
imx290_set_ctrl() that it matters, I could keep the value in the
imx290_10bit_settings and imx290_12bit_settings arrays, but I thought I
could as well do it the same way everywhere.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx290.c | 53 +++++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 135ed55592a1..d9fc6c44b0f0 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -152,6 +152,9 @@
> >  #define IMX290_PIXEL_ARRAY_RECORDING_WIDTH             1920
> >  #define IMX290_PIXEL_ARRAY_RECORDING_HEIGHT            1080
> >
> > +/* Equivalent value for 16bpp */
> > +#define IMX290_BLACK_LEVEL_DEFAULT                     3840
> > +
> >  #define IMX290_NUM_SUPPLIES                            3
> >
> >  struct imx290_regval {
> > @@ -315,7 +318,6 @@ static const struct imx290_regval imx290_10bit_settings[] = {
> >         { IMX290_ADBIT2, IMX290_ADBIT2_10BIT },
> >         { IMX290_ADBIT3, IMX290_ADBIT3_10BIT },
> >         { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW10 },
> > -       { IMX290_BLKLEVEL, 60 },
> >  };
> >
> >  static const struct imx290_regval imx290_12bit_settings[] = {
> > @@ -325,7 +327,6 @@ static const struct imx290_regval imx290_12bit_settings[] = {
> >         { IMX290_ADBIT2, IMX290_ADBIT2_12BIT },
> >         { IMX290_ADBIT3, IMX290_ADBIT3_12BIT },
> >         { IMX290_CSI_DT_FMT, IMX290_CSI_DT_FMT_RAW12 },
> > -       { IMX290_BLKLEVEL, 240 },
> >  };
> >
> >  /* supported link frequencies */
> > @@ -516,35 +517,43 @@ static int imx290_set_data_lanes(struct imx290 *imx290)
> >         return ret;
> >  }
> >
> > +static int imx290_set_black_level(struct imx290 *imx290,
> > +                                 unsigned int black_level, int *err)
> > +{
> > +       return imx290_write(imx290, IMX290_BLKLEVEL,
> > +                           black_level >> (16 - imx290->bpp), err);
> > +}
> > +
> >  static int imx290_write_current_format(struct imx290 *imx290)
> >  {
> > +       const struct imx290_regval *regs;
> > +       unsigned int num_regs;
> > +       unsigned int bpp;
> >         int ret;
> >
> >         switch (imx290->current_format.code) {
> >         case MEDIA_BUS_FMT_SRGGB10_1X10:
> > -               ret = imx290_set_register_array(imx290, imx290_10bit_settings,
> > -                                               ARRAY_SIZE(
> > -                                                       imx290_10bit_settings));
> > -               if (ret < 0) {
> > -                       dev_err(imx290->dev, "Could not set format registers\n");
> > -                       return ret;
> > -               }
> > +               regs = imx290_10bit_settings;
> > +               num_regs = ARRAY_SIZE(imx290_10bit_settings);
> > +               bpp = 10;
> >                 break;
> >         case MEDIA_BUS_FMT_SRGGB12_1X12:
> > -               ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> > -                                               ARRAY_SIZE(
> > -                                                       imx290_12bit_settings));
> > -               if (ret < 0) {
> > -                       dev_err(imx290->dev, "Could not set format registers\n");
> > -                       return ret;
> > -               }
> > +               regs = imx290_12bit_settings;
> > +               num_regs = ARRAY_SIZE(imx290_12bit_settings);
> > +               bpp = 12;
> >                 break;
> >         default:
> >                 dev_err(imx290->dev, "Unknown pixel format\n");
> >                 return -EINVAL;
> >         }
> >
> > -       return 0;
> > +       ret = imx290_set_register_array(imx290, regs, num_regs);
> > +       if (ret < 0) {
> > +               dev_err(imx290->dev, "Could not set format registers\n");
> > +               return ret;
> > +       }
> > +
> > +       return imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT, &ret);
> >  }
> >
> >  /* ----------------------------------------------------------------------------
> > @@ -573,7 +582,7 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >         case V4L2_CID_TEST_PATTERN:
> >                 if (ctrl->val) {
> > -                       imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
> > +                       imx290_set_black_level(imx290, 0, &ret);
> >                         usleep_range(10000, 11000);
> >                         imx290_write(imx290, IMX290_PGCTRL,
> >                                      (u8)(IMX290_PGCTRL_REGEN |
> > @@ -582,12 +591,8 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >                 } else {
> >                         imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
> >                         usleep_range(10000, 11000);
> > -                       if (imx290->bpp == 10)
> > -                               imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
> > -                                            &ret);
> > -                       else /* 12 bits per pixel */
> > -                               imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
> > -                                            &ret);
> > +                       imx290_set_black_level(imx290, IMX290_BLACK_LEVEL_DEFAULT,
> > +                                              &ret);
> >                 }
> >                 break;
> >         default:

-- 
Regards,

Laurent Pinchart