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.
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).
I'll move it and add a comment before the register tables.
Dave
> > + if (ret) {
> > + dev_err(ov9282->dev, "fail to write MIPI_CTRL00");
> > + return ret;
> > + }
> > +
> > /* Write sensor mode registers */
> > reg_list = &ov9282->cur_mode->reg_list;
> > ret = ov9282_write_regs(ov9282, reg_list->regs, reg_list->num_of_regs);
> > @@ -861,6 +872,9 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> > if (ret)
> > return ret;
> >
> > + ov9282->noncontinuous_clock =
> > + bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
> > +
> > if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) {
> > dev_err(ov9282->dev,
> > "number of CSI2 data lanes %d is not supported",
>
> --
> Kind regards,
>
> Sakari Ailus
