Re: [PATCH 10/11] media: i2c: ov5640: Disable ISP for raw output

[Kieran Bingham]

Quoting Jacopo Mondi (2026-05-14 09:39:31)
> Hi Kieran
> 
> On Fri, May 01, 2026 at 04:39:12PM +0100, Kieran Bingham wrote:
> > The OV5640 has ISP operations that can run even when outputting RAW
> > bayer data.  These include the Lens Shading Correction, Gamma and Auto
> > white balance which need to be disabled when performing module
> > calibration using RAW data.
> >
> > Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/ov5640.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index fd369a13463e..f63d81640f54 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -280,28 +280,28 @@ static const struct ov5640_pixfmt ov5640_dvp_formats[] = {
> >       }, {
> >               /* Raw, BGBG... / GRGR... */
> >               .code           = MEDIA_BUS_FMT_SBGGR8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x00,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, GBGB... / RGRG... */
> >               .code           = MEDIA_BUS_FMT_SGBRG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x01,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, GRGR... / BGBG... */
> >               .code           = MEDIA_BUS_FMT_SGRBG8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x02,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> >       }, {
> >               /* Raw bayer, RGRG... / GBGB... */
> >               .code           = MEDIA_BUS_FMT_SRGGB8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x03,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > @@ -348,7 +348,7 @@ static const struct ov5640_pixfmt ov5640_csi2_formats[] = {
> >       }, {
> >               /* Raw, BGBG... / GRGR... */
> >               .code           = MEDIA_BUS_FMT_SBGGR8_1X8,
> > -             .colorspace     = V4L2_COLORSPACE_SRGB,
> > +             .colorspace     = V4L2_COLORSPACE_RAW,
> >               .bpp            = 8,
> >               .ctrl00         = 0x00,
> >               .mux            = OV5640_FMT_MUX_RAW_DPC,
> > @@ -473,6 +473,7 @@ struct ov5640_dev {
> >
> >       struct v4l2_mbus_framefmt fmt;
> >       bool pending_fmt_change;
> > +     bool is_raw;
> >
> >       const struct ov5640_mode_info *current_mode;
> >       const struct ov5640_mode_info *last_mode;
> > @@ -618,8 +619,13 @@ static const struct reg_value ov5640_init_setting[] = {
> >       {0x501f, 0x00, 0, 0}, {0x440e, 0x00, 0, 0}, {0x4837, 0x0a, 0, 0},
> >
> >       /* ISP Control */
> > -     {OV5640_REG_ISP_CTRL00, 0xa7, 0, 0},
> > -     {OV5640_REG_ISP_CTRL01, 0xa3, 0, 0},
> > +     {OV5640_REG_ISP_CTRL00, OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> > +                             OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> > +                             OV5640_ISP_00_CIP_ENABLE, 0, 0},
> > +
> > +     /* OV5640_ISP_01_UVA_ENABLE is not enabled */
> > +     {OV5640_REG_ISP_CTRL01, OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> > +                             OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE, 0, 0},
> 
> Should we avoid writing these registers at all in the init sequence ?

Oh probably. I haven't tested that yet, but indeed.

> >
> >       /* AWB Control */
> >       {OV5640_REG_AWB_CONTROL_00, 0xff, 0, 0}, /* AWB B Block */
> > @@ -3116,6 +3122,31 @@ static int ov5640_set_framefmt(struct ov5640_dev *sensor,
> >       if (ret)
> >               return ret;
> >
> > +     /*
> > +      * Disable all ISP image processing (Lens Shading, Gamma, AWB...) for
> > +      * RAW modes to facilitate module tuning.
> > +      */
> > +     sensor->is_raw = pixfmt->colorspace == V4L2_COLORSPACE_RAW;
> 
> Unless it is used later on, the 'is_raw' flag can be kept a local
> variable.

Haha yes, no idea why I put it in the main dev structure.

Will move. Thanks.

> 
> > +     if (sensor->is_raw) {
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00, 0);
> > +             if (ret)
> > +                     return ret;
> > +     } else {
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL00,
> > +                                    OV5640_ISP_00_LENC_ENABLE | OV5640_ISP_00_GMA_ENABLE |
> > +                                    OV5640_ISP_00_BPC_ENABLE | OV5640_ISP_00_WPC_ENABLE |
> > +                                    OV5640_ISP_00_CIP_ENABLE);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             /* OV5640_ISP_01_UVA_ENABLE is not enabled */
> > +             ret = ov5640_write_reg(sensor, OV5640_REG_ISP_CTRL01,
> > +                                    OV5640_ISP_01_SDE_ENABLE | OV5640_ISP_01_SCL_ENABLE |
> > +                                    OV5640_ISP_01_CMX_ENABLE | OV5640_ISP_01_AWB_ENABLE);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       /*
> >        * TIMING TC REG21:
> >        * - [5]:       JPEG enable
> >
> > --
> > 2.52.0
> >
> >