Re: [PATCH 12/21] media: i2c: imx258: Allow configuration of clock lane behaviour

[Dave Stevenson <dave.stevenson@raspberrypi.com>]

Hi Jacopo

On Fri, 2 Jun 2023 at 14:27, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Dave
>
> On Tue, May 30, 2023 at 06:29:51PM +0100, Dave Stevenson wrote:
> > The sensor supports the clock lane either remaining in HS mode
> > during frame blanking, or dropping to LP11.
> >
> > Add configuration of the mode via V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK.
>
> Assuming a follow-up patch for DT
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

It's already covered in video-interfaces.yaml, and is an optional flag
as the driver will work in either manner, so do the bindings need
updating?

In checking the default value for this register, it is 0x01, or non-continuous.
The original omission of this property from the binding and driver
therefore means that an existing binding will most likely have omitted
it and be believing the sensor is in continuous clock mode when it
isn't.
Now that we check the mode requested, it will actually adopt
continuous clock mode and may no longer work with the receiver.

Perhaps it's best to drop this patch, and add it as a note to anyone
preparing a talk on Camera Sensor Drivers Compliance ;-)

  Dave

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx258.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 1fa83fe82f27..b5c2dcb7c9e6 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -72,6 +72,8 @@
> >  /* Test Pattern Control */
> >  #define IMX258_REG_TEST_PATTERN              0x0600
> >
> > +#define IMX258_CLK_BLANK_STOP                0x4040
> > +
> >  /* Orientation */
> >  #define REG_MIRROR_FLIP_CONTROL              0x0101
> >  #define REG_CONFIG_MIRROR_FLIP               0x03
> > @@ -634,6 +636,7 @@ struct imx258 {
> >       const struct imx258_link_freq_config *link_freq_configs;
> >       const s64 *link_freq_menu_items;
> >       unsigned int nlanes;
> > +     unsigned int csi2_flags;
> >
> >       /*
> >        * Mutex for serialized access:
> > @@ -1072,6 +1075,15 @@ static int imx258_start_streaming(struct imx258 *imx258)
> >               return ret;
> >       }
> >
> > +     ret = imx258_write_reg(imx258, IMX258_CLK_BLANK_STOP,
> > +                            IMX258_REG_VALUE_08BIT,
> > +                            imx258->csi2_flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK ?
> > +                            1 : 0);
> > +     if (ret) {
> > +             dev_err(&client->dev, "%s failed to set clock lane mode\n", __func__);
> > +             return ret;
> > +     }
> > +
> >       /* Apply default values of current mode */
> >       reg_list = &imx258->cur_mode->reg_list;
> >       ret = imx258_write_regs(imx258, reg_list->regs, reg_list->num_of_regs);
> > @@ -1486,6 +1498,8 @@ static int imx258_probe(struct i2c_client *client)
> >               goto error_endpoint_poweron;
> >       }
> >
> > +     imx258->csi2_flags = ep.bus.mipi_csi2.flags;
> > +
> >       /* Initialize subdev */
> >       v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
> >
> > --
> > 2.25.1
> >