diff mbox series

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

Message ID 20221005152809.3785786-9-dave.stevenson@raspberrypi.com (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series Updates to ov9282 sensor driver |

Commit Message

Dave Stevenson Oct. 5, 2022, 3:28 p.m. UTC 
  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(-)

Comments

Jacopo Mondi Oct. 6, 2022, 9:24 a.m. UTC | #1 
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;

Nit: I would move this down after the controls, where vblank is

>  	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);
> +	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;
> +

The patch looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	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",
> --
> 2.34.1
>
sakari.ailus@iki.fi Oct. 26, 2022, 7:21 a.m. UTC | #2 
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?

> +	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",
Dave Stevenson Oct. 28, 2022, 12:57 p.m. UTC | #3 
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.
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
sakari.ailus@iki.fi Oct. 28, 2022, 2:30 p.m. UTC | #4 
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.

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.
Dave Stevenson Oct. 28, 2022, 3:03 p.m. UTC | #5 
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

> 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.

  Dave

> >
> > I'll move it and add a comment before the register tables.
>
> I think it's unnecessary.
>
> --
> Kind regards,
>
> Sakari Ailus
sakari.ailus@iki.fi Oct. 31, 2022, 1:06 p.m. UTC | #6 
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.
diff mbox series

Patch

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);
+	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",