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@xxxxxx> 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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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.
But that's up to you. I guess lane configuration etc. is part of the big
register lists.
>
> I'll move it and add a comment before the register tables.
I think it's unnecessary.
--
Kind regards,
Sakari Ailus
