diff mbox series

media: i2c: ov5695: Fix power on and off sequences

Message ID 20191220130800.61589-1-tfiga@chromium.org (mailing list archive)
State Changes Requested, archived
Delegated to: Sakari Ailus
Headers
Series media: i2c: ov5695: Fix power on and off sequences |

Commit Message

Tomasz Figa Dec. 20, 2019, 1:08 p.m. UTC 
  From: Dongchun Zhu <dongchun.zhu@mediatek.com>

From the measured hardware signal, OV5695 reset pin goes high for a
short period of time during boot-up. From the sensor specification, the
reset pin is active low and the DT binding defines the pin as active
low, which means that the values set by the driver are inverted and thus
the value requested in probe ends up high.

Fix it by changing probe to request the reset GPIO initialized to high,
which makes the initial state of the physical signal low.

In addition, DOVDD rising must occur before DVDD rising from spec., but
regulator_bulk_enable() API enables all the regulators asynchronously.
Use an explicit loops of regulator_enable() instead.

For power off sequence, it is required that DVDD falls first. Given the
bulk API does not give any guarantee about the order of regulators,
change the driver to use regulator_disable() instead.

The sensor also requires a delay between reset high and first I2C
transaction, which was assumed to be 8192 XVCLK cycles, but 1ms is
recommended by the vendor. Fix this as well.

Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/i2c/ov5695.c | 41 +++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 18 deletions(-)

Comments

Sakari Ailus Dec. 20, 2019, 3:19 p.m. UTC | #1 
Hi Tomasz,

On Fri, Dec 20, 2019 at 10:08:00PM +0900, Tomasz Figa wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> 
> From the measured hardware signal, OV5695 reset pin goes high for a
> short period of time during boot-up. From the sensor specification, the
> reset pin is active low and the DT binding defines the pin as active
> low, which means that the values set by the driver are inverted and thus
> the value requested in probe ends up high.
> 
> Fix it by changing probe to request the reset GPIO initialized to high,
> which makes the initial state of the physical signal low.
> 
> In addition, DOVDD rising must occur before DVDD rising from spec., but
> regulator_bulk_enable() API enables all the regulators asynchronously.
> Use an explicit loops of regulator_enable() instead.
> 
> For power off sequence, it is required that DVDD falls first. Given the
> bulk API does not give any guarantee about the order of regulators,
> change the driver to use regulator_disable() instead.
> 
> The sensor also requires a delay between reset high and first I2C
> transaction, which was assumed to be 8192 XVCLK cycles, but 1ms is
> recommended by the vendor. Fix this as well.
> 
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  drivers/media/i2c/ov5695.c | 41 +++++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> index d6cd15bb699ac..8d0cc3893fcfc 100644
> --- a/drivers/media/i2c/ov5695.c
> +++ b/drivers/media/i2c/ov5695.c
> @@ -971,16 +971,9 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
>  	return ret;
>  }
>  
> -/* Calculate the delay in us by clock rate and clock cycles */
> -static inline u32 ov5695_cal_delay(u32 cycles)
> -{
> -	return DIV_ROUND_UP(cycles, OV5695_XVCLK_FREQ / 1000 / 1000);
> -}
> -
>  static int __ov5695_power_on(struct ov5695 *ov5695)
>  {
> -	int ret;
> -	u32 delay_us;
> +	int i, ret;
>  	struct device *dev = &ov5695->client->dev;
>  
>  	ret = clk_prepare_enable(ov5695->xvclk);
> @@ -991,21 +984,24 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
>  
>  	gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
>  
> -	ret = regulator_bulk_enable(OV5695_NUM_SUPPLIES, ov5695->supplies);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to enable regulators\n");
> -		goto disable_clk;
> +	for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> +		ret = regulator_enable(ov5695->supplies[i].consumer);

The regulator voltage takes some time before it settles. If the hardware
requires a particular order, then presumably there should be a small delay
to ensure that. 1 ms should be plenty.

I also think it'd be necessary to add a comment here explaining the
requirements for enabling regulators, as otherwise I expect someone to
"fix" this sooner or later.

Same for powering off.

> +		if (ret) {
> +			dev_err(dev, "Failed to enable %s: %d\n",
> +				ov5695->supplies[i].supply, ret);
> +			goto disable_reg_clk;
> +		}
>  	}
>  
>  	gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
>  
> -	/* 8192 cycles prior to first SCCB transaction */
> -	delay_us = ov5695_cal_delay(8192);
> -	usleep_range(delay_us, delay_us * 2);
> +	usleep_range(1000, 1200);
>  
>  	return 0;
>  
> -disable_clk:
> +disable_reg_clk:
> +	for (--i; i >= 0; i--)
> +		regulator_disable(ov5695->supplies[i].consumer);
>  	clk_disable_unprepare(ov5695->xvclk);
>  
>  	return ret;
> @@ -1013,9 +1009,18 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
>  
>  static void __ov5695_power_off(struct ov5695 *ov5695)
>  {
> +	struct device *dev = &ov5695->client->dev;
> +	int i, ret;
> +
>  	clk_disable_unprepare(ov5695->xvclk);
>  	gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> -	regulator_bulk_disable(OV5695_NUM_SUPPLIES, ov5695->supplies);
> +
> +	for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
> +		ret = regulator_disable(ov5695->supplies[i].consumer);
> +		if (ret)
> +			dev_err(dev, "Failed to disable %s: %d\n",
> +				ov5695->supplies[i].supply, ret);
> +	}
>  }
>  
>  static int __maybe_unused ov5695_runtime_resume(struct device *dev)
> @@ -1285,7 +1290,7 @@ static int ov5695_probe(struct i2c_client *client,
>  	if (clk_get_rate(ov5695->xvclk) != OV5695_XVCLK_FREQ)
>  		dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
>  
> -	ov5695->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	ov5695->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(ov5695->reset_gpio)) {
>  		dev_err(dev, "Failed to get reset-gpios\n");
>  		return -EINVAL;
Tomasz Figa March 11, 2020, 10:39 a.m. UTC | #2 
Hi Sakari,

On Sat, Dec 21, 2019 at 12:19 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz,
>
> On Fri, Dec 20, 2019 at 10:08:00PM +0900, Tomasz Figa wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > From the measured hardware signal, OV5695 reset pin goes high for a
> > short period of time during boot-up. From the sensor specification, the
> > reset pin is active low and the DT binding defines the pin as active
> > low, which means that the values set by the driver are inverted and thus
> > the value requested in probe ends up high.
> >
> > Fix it by changing probe to request the reset GPIO initialized to high,
> > which makes the initial state of the physical signal low.
> >
> > In addition, DOVDD rising must occur before DVDD rising from spec., but
> > regulator_bulk_enable() API enables all the regulators asynchronously.
> > Use an explicit loops of regulator_enable() instead.
> >
> > For power off sequence, it is required that DVDD falls first. Given the
> > bulk API does not give any guarantee about the order of regulators,
> > change the driver to use regulator_disable() instead.
> >
> > The sensor also requires a delay between reset high and first I2C
> > transaction, which was assumed to be 8192 XVCLK cycles, but 1ms is
> > recommended by the vendor. Fix this as well.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> > ---
> >  drivers/media/i2c/ov5695.c | 41 +++++++++++++++++++++-----------------
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> > index d6cd15bb699ac..8d0cc3893fcfc 100644
> > --- a/drivers/media/i2c/ov5695.c
> > +++ b/drivers/media/i2c/ov5695.c
> > @@ -971,16 +971,9 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
> >       return ret;
> >  }
> >
> > -/* Calculate the delay in us by clock rate and clock cycles */
> > -static inline u32 ov5695_cal_delay(u32 cycles)
> > -{
> > -     return DIV_ROUND_UP(cycles, OV5695_XVCLK_FREQ / 1000 / 1000);
> > -}
> > -
> >  static int __ov5695_power_on(struct ov5695 *ov5695)
> >  {
> > -     int ret;
> > -     u32 delay_us;
> > +     int i, ret;
> >       struct device *dev = &ov5695->client->dev;
> >
> >       ret = clk_prepare_enable(ov5695->xvclk);
> > @@ -991,21 +984,24 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> >
> >       gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> >
> > -     ret = regulator_bulk_enable(OV5695_NUM_SUPPLIES, ov5695->supplies);
> > -     if (ret < 0) {
> > -             dev_err(dev, "Failed to enable regulators\n");
> > -             goto disable_clk;
> > +     for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> > +             ret = regulator_enable(ov5695->supplies[i].consumer);
>
> The regulator voltage takes some time before it settles. If the hardware
> requires a particular order, then presumably there should be a small delay
> to ensure that. 1 ms should be plenty.

The regulator API guarantees that when regulator_enable() returns, the
voltage is stable. Regulator ramp up delays can be also configured via
DT to take care for per-platform variability.

>
> I also think it'd be necessary to add a comment here explaining the
> requirements for enabling regulators, as otherwise I expect someone to
> "fix" this sooner or later.

True. Let me add a comment.

>
> Same for powering off.
>

Same as above.

Best regards,
Tomasz
Sakari Ailus March 11, 2020, 10:45 a.m. UTC | #3 
Hi Tomasz,

On Wed, Mar 11, 2020 at 07:39:15PM +0900, Tomasz Figa wrote:
> Hi Sakari,
> 
> On Sat, Dec 21, 2019 at 12:19 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Fri, Dec 20, 2019 at 10:08:00PM +0900, Tomasz Figa wrote:
> > > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > >
> > > From the measured hardware signal, OV5695 reset pin goes high for a
> > > short period of time during boot-up. From the sensor specification, the
> > > reset pin is active low and the DT binding defines the pin as active
> > > low, which means that the values set by the driver are inverted and thus
> > > the value requested in probe ends up high.
> > >
> > > Fix it by changing probe to request the reset GPIO initialized to high,
> > > which makes the initial state of the physical signal low.
> > >
> > > In addition, DOVDD rising must occur before DVDD rising from spec., but
> > > regulator_bulk_enable() API enables all the regulators asynchronously.
> > > Use an explicit loops of regulator_enable() instead.
> > >
> > > For power off sequence, it is required that DVDD falls first. Given the
> > > bulk API does not give any guarantee about the order of regulators,
> > > change the driver to use regulator_disable() instead.
> > >
> > > The sensor also requires a delay between reset high and first I2C
> > > transaction, which was assumed to be 8192 XVCLK cycles, but 1ms is
> > > recommended by the vendor. Fix this as well.
> > >
> > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > Signed-off-by: Tomasz Figa <tfiga@chromium.org>
> > > ---
> > >  drivers/media/i2c/ov5695.c | 41 +++++++++++++++++++++-----------------
> > >  1 file changed, 23 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> > > index d6cd15bb699ac..8d0cc3893fcfc 100644
> > > --- a/drivers/media/i2c/ov5695.c
> > > +++ b/drivers/media/i2c/ov5695.c
> > > @@ -971,16 +971,9 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
> > >       return ret;
> > >  }
> > >
> > > -/* Calculate the delay in us by clock rate and clock cycles */
> > > -static inline u32 ov5695_cal_delay(u32 cycles)
> > > -{
> > > -     return DIV_ROUND_UP(cycles, OV5695_XVCLK_FREQ / 1000 / 1000);
> > > -}
> > > -
> > >  static int __ov5695_power_on(struct ov5695 *ov5695)
> > >  {
> > > -     int ret;
> > > -     u32 delay_us;
> > > +     int i, ret;
> > >       struct device *dev = &ov5695->client->dev;
> > >
> > >       ret = clk_prepare_enable(ov5695->xvclk);
> > > @@ -991,21 +984,24 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > >
> > >       gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> > >
> > > -     ret = regulator_bulk_enable(OV5695_NUM_SUPPLIES, ov5695->supplies);
> > > -     if (ret < 0) {
> > > -             dev_err(dev, "Failed to enable regulators\n");
> > > -             goto disable_clk;
> > > +     for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> > > +             ret = regulator_enable(ov5695->supplies[i].consumer);
> >
> > The regulator voltage takes some time before it settles. If the hardware
> > requires a particular order, then presumably there should be a small delay
> > to ensure that. 1 ms should be plenty.
> 
> The regulator API guarantees that when regulator_enable() returns, the
> voltage is stable. Regulator ramp up delays can be also configured via
> DT to take care for per-platform variability.

Ack. In practice not many drivers do that still. But that should probably
be seen as a driver bug indeed.
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
index d6cd15bb699ac..8d0cc3893fcfc 100644
--- a/drivers/media/i2c/ov5695.c
+++ b/drivers/media/i2c/ov5695.c
@@ -971,16 +971,9 @@  static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
 	return ret;
 }
 
-/* Calculate the delay in us by clock rate and clock cycles */
-static inline u32 ov5695_cal_delay(u32 cycles)
-{
-	return DIV_ROUND_UP(cycles, OV5695_XVCLK_FREQ / 1000 / 1000);
-}
-
 static int __ov5695_power_on(struct ov5695 *ov5695)
 {
-	int ret;
-	u32 delay_us;
+	int i, ret;
 	struct device *dev = &ov5695->client->dev;
 
 	ret = clk_prepare_enable(ov5695->xvclk);
@@ -991,21 +984,24 @@  static int __ov5695_power_on(struct ov5695 *ov5695)
 
 	gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
 
-	ret = regulator_bulk_enable(OV5695_NUM_SUPPLIES, ov5695->supplies);
-	if (ret < 0) {
-		dev_err(dev, "Failed to enable regulators\n");
-		goto disable_clk;
+	for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
+		ret = regulator_enable(ov5695->supplies[i].consumer);
+		if (ret) {
+			dev_err(dev, "Failed to enable %s: %d\n",
+				ov5695->supplies[i].supply, ret);
+			goto disable_reg_clk;
+		}
 	}
 
 	gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
 
-	/* 8192 cycles prior to first SCCB transaction */
-	delay_us = ov5695_cal_delay(8192);
-	usleep_range(delay_us, delay_us * 2);
+	usleep_range(1000, 1200);
 
 	return 0;
 
-disable_clk:
+disable_reg_clk:
+	for (--i; i >= 0; i--)
+		regulator_disable(ov5695->supplies[i].consumer);
 	clk_disable_unprepare(ov5695->xvclk);
 
 	return ret;
@@ -1013,9 +1009,18 @@  static int __ov5695_power_on(struct ov5695 *ov5695)
 
 static void __ov5695_power_off(struct ov5695 *ov5695)
 {
+	struct device *dev = &ov5695->client->dev;
+	int i, ret;
+
 	clk_disable_unprepare(ov5695->xvclk);
 	gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
-	regulator_bulk_disable(OV5695_NUM_SUPPLIES, ov5695->supplies);
+
+	for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
+		ret = regulator_disable(ov5695->supplies[i].consumer);
+		if (ret)
+			dev_err(dev, "Failed to disable %s: %d\n",
+				ov5695->supplies[i].supply, ret);
+	}
 }
 
 static int __maybe_unused ov5695_runtime_resume(struct device *dev)
@@ -1285,7 +1290,7 @@  static int ov5695_probe(struct i2c_client *client,
 	if (clk_get_rate(ov5695->xvclk) != OV5695_XVCLK_FREQ)
 		dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
 
-	ov5695->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	ov5695->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(ov5695->reset_gpio)) {
 		dev_err(dev, "Failed to get reset-gpios\n");
 		return -EINVAL;