Re: [PATCH 08/16] media: i2c: ov9282: Add selection for CSI2 clock mode

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Dave,

On Fri, Oct 28, 2022 at 04:03:45PM +0100, Dave Stevenson wrote:
> Hi Sakari
> 
> On Fri, 28 Oct 2022 at 15:30, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > Hi Dave,
> >
> > On Fri, Oct 28, 2022 at 01:57:48PM +0100, Dave Stevenson wrote:
> > > Hi Sakari
> > >
> > > On Wed, 26 Oct 2022 at 08:21, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> > > >
> > > > Hi Dave,
> > > >
> > > > On Wed, Oct 05, 2022 at 04:28:01PM +0100, Dave Stevenson wrote:
> > > > > The sensor supports either having the CSI2 clock lane free
> > > > > running, or gated when there is no packet to transmit.
> > > > > The driver only selected gated (non-continuous) clock mode.
> > > > >
> > > > > Add code to allow fwnode to configure whether the clock is
> > > > > gated or free running.
> > > > >
> > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov9282.c | 16 +++++++++++++++-
> > > > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > > > index abb1223c0260..334b31af34a4 100644
> > > > > --- a/drivers/media/i2c/ov9282.c
> > > > > +++ b/drivers/media/i2c/ov9282.c
> > > > > @@ -46,6 +46,9 @@
> > > > >  /* Group hold register */
> > > > >  #define OV9282_REG_HOLD              0x3308
> > > > >
> > > > > +#define OV9282_REG_MIPI_CTRL00       0x4800
> > > > > +#define OV9282_GATED_CLOCK   BIT(5)
> > > > > +
> > > > >  /* Input clock rate */
> > > > >  #define OV9282_INCLK_RATE    24000000
> > > > >
> > > > > @@ -138,6 +141,7 @@ struct ov9282 {
> > > > >       struct clk *inclk;
> > > > >       struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > > > >       struct v4l2_ctrl_handler ctrl_handler;
> > > > > +     bool noncontinuous_clock;
> > > > >       struct v4l2_ctrl *link_freq_ctrl;
> > > > >       struct v4l2_ctrl *hblank_ctrl;
> > > > >       struct v4l2_ctrl *vblank_ctrl;
> > > > > @@ -211,7 +215,6 @@ static const struct ov9282_reg common_regs[] = {
> > > > >       {0x4601, 0x04},
> > > > >       {0x470f, 0x00},
> > > > >       {0x4f07, 0x00},
> > > > > -     {0x4800, 0x20},
> > > > >       {0x5000, 0x9f},
> > > > >       {0x5001, 0x00},
> > > > >       {0x5e00, 0x00},
> > > > > @@ -684,6 +687,14 @@ static int ov9282_start_streaming(struct ov9282 *ov9282)
> > > > >               return ret;
> > > > >       }
> > > > >
> > > > > +     ret = ov9282_write_reg(ov9282, OV9282_REG_MIPI_CTRL00, 1,
> > > > > +                            ov9282->noncontinuous_clock ?
> > > > > +                                     OV9282_GATED_CLOCK : 0);
> > > >
> > > > Wouldn't this better fit for power on?
> > >
> > > It can be done in ov9282_power_on, but is then totally redundantly set
> > > when powering the sensor up to read the ID during initial probe.
> >
> > This is the same also when streaming is enabled and disabled multiple times
> > while the sensor is powered on. Although without autosuspend this may be
> > unlikely.
> >
> > > Doing so also means there needs to be a great big warning never to
> > > change the driver and hit the software reset via writing 0x01 to
> > > register 0x0103 as part of any register array (very common in many
> > > other sensor drivers).
> >
> > If there's a desire to reset the sensor after powering it up, that should
> > be done as the first thing after power-up. Setting non-continuous clock
> > isn't anything special here.
> 
> I'm only looking at existing drivers in mainline as there is no clear
> documentation on do's and don't's within sensor drivers (I know
> writing good documentation is hard).
> ov7251 [1] reset in ov7251_global_init_setting
> ov8856 [2] reset in the lane config tables
> ov5695 [3] reset in ov5695_global_regs
> ov2740 [4] reset in mipi_data_rate_720mbps
> ov13858 [5] explicit reset in ov13858_start_streaming
> ov13b10 [6] explicit reset in ov13b10_start_streaming
> 
> In my book that's a common enough pattern in mainline drivers for it
> to be worth warning against introducing it when it will cause quirky
> behaviour.
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov7251.c#L238
> [2] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov8856.c#L167
> [3] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov5695.c#L128
> [4] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov2740.c#L127
> [5] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13858.c#L1421
> [6] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/ov13b10.c#L1033

Well, register list based sensor drivers are hardly exemplary in this
respect. Some drivers reset the device after resuming it, some do not.

As noted, it's up to you.

> 
> > But that's up to you. I guess lane configuration etc. is part of the big
> > register lists.
> 
> Only 2 data lanes are currently supported by the driver.
> Bit 5 of 0x3039 in theory allows you to drop to 1 lane, but I've not
> got it to work. I suspect further clock tree changes are required.
> 
> A 400MHz link freq (800Mbit/s/lane) is already required for the max
> 1280x800@120fps.
> Dropping to 1 lane therefore either requires reducing the max frame
> rate, or potentially running at 800MHz link freq, which is well in
> excess of the earlier versions of CSI-2 spec (500MHz or 1Gbit/s). IMHO
> It's not worth pursuing.

Works for me.

-- 
Kind regards,

Sakari Ailus