| Message ID | 20191220130800.61589-1-tfiga@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5695: Fix power on and off sequences | expand |
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;
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
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 --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;