| Message ID | 20240823102731.2240857-7-vladimir.zapolskiy@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: og01a1b: Add OF support to OmniVision OG01A1B | expand |
Hi Vladimir, Thanks for the update. On Fri, Aug 23, 2024 at 01:27:31PM +0300, Vladimir Zapolskiy wrote: > Omnivision OG01A1B camera sensor is supplied by three power rails, > if supplies are present as device properties, include them into > the sensor power up sequence. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 85 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c > index 90a68201f43f..0150fdd2f424 100644 > --- a/drivers/media/i2c/og01a1b.c > +++ b/drivers/media/i2c/og01a1b.c > @@ -9,6 +9,7 @@ > #include <linux/i2c.h> > #include <linux/module.h> > #include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = { > struct og01a1b { > struct clk *xvclk; > struct gpio_desc *reset_gpio; > + struct regulator *avdd; > + struct regulator *dovdd; > + struct regulator *dvdd; > > struct v4l2_subdev sd; > struct media_pad pad; > @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev) > { > struct v4l2_subdev *sd = dev_get_drvdata(dev); > struct og01a1b *og01a1b = to_og01a1b(sd); > + int ret; > + > + if (og01a1b->avdd) { > + ret = regulator_enable(og01a1b->avdd); > + if (ret) > + return ret; > + } > + > + if (og01a1b->dovdd) { > + ret = regulator_enable(og01a1b->dovdd); > + if (ret) > + goto avdd_disable; > + } > + > + if (og01a1b->dvdd) { > + ret = regulator_enable(og01a1b->dvdd); > + if (ret) > + goto dovdd_disable; > + } > > gpiod_set_value_cansleep(og01a1b->reset_gpio, 0); > usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); > > - return clk_prepare_enable(og01a1b->xvclk); > + ret = clk_prepare_enable(og01a1b->xvclk); > + if (ret) > + goto dvdd_disable; Virtually all sensors require some delay between lifting the reset (which typically comes after enabling the regulators and the clock) and the first I²C access. This one appears to require 8192 XCLK cycles. > + > + return 0; > + > +dvdd_disable: > + if (og01a1b->dvdd) > + regulator_disable(og01a1b->dvdd); > +dovdd_disable: > + if (og01a1b->dovdd) > + regulator_disable(og01a1b->dovdd); > +avdd_disable: > + if (og01a1b->avdd) > + regulator_disable(og01a1b->avdd); > + > + return ret; > } > > static int og01a1b_power_off(struct device *dev) > @@ -998,6 +1037,15 @@ static int og01a1b_power_off(struct device *dev) > > gpiod_set_value_cansleep(og01a1b->reset_gpio, 1); > > + if (og01a1b->dvdd) > + regulator_disable(og01a1b->dvdd); > + > + if (og01a1b->dovdd) > + regulator_disable(og01a1b->dovdd); > + > + if (og01a1b->avdd) > + regulator_disable(og01a1b->avdd); > + > return 0; > } > > @@ -1045,6 +1093,42 @@ static int og01a1b_probe(struct i2c_client *client) > return PTR_ERR(og01a1b->reset_gpio); > } > > + og01a1b->avdd = devm_regulator_get_optional(&client->dev, "avdd"); > + if (IS_ERR(og01a1b->avdd)) { > + ret = PTR_ERR(og01a1b->avdd); > + if (ret != -ENODEV) { > + dev_err_probe(&client->dev, ret, > + "Failed to get 'avdd' regulator\n"); > + return ret; > + } > + > + og01a1b->avdd = NULL; > + } > + > + og01a1b->dovdd = devm_regulator_get_optional(&client->dev, "dovdd"); > + if (IS_ERR(og01a1b->dovdd)) { > + ret = PTR_ERR(og01a1b->dovdd); > + if (ret != -ENODEV) { > + dev_err_probe(&client->dev, ret, > + "Failed to get 'dovdd' regulator\n"); > + return ret; > + } > + > + og01a1b->dovdd = NULL; > + } > + > + og01a1b->dvdd = devm_regulator_get_optional(&client->dev, "dvdd"); > + if (IS_ERR(og01a1b->dvdd)) { > + ret = PTR_ERR(og01a1b->dvdd); > + if (ret != -ENODEV) { > + dev_err_probe(&client->dev, ret, > + "Failed to get 'dvdd' regulator\n"); > + return ret; > + } > + > + og01a1b->dvdd = NULL; > + } > + > /* The sensor must be powered on to read the CHIP_ID register */ > ret = og01a1b_power_on(&client->dev); > if (ret)
Hi Sakari, thank you for review. On 8/28/24 11:09, Sakari Ailus wrote: > Hi Vladimir, > > Thanks for the update. > > On Fri, Aug 23, 2024 at 01:27:31PM +0300, Vladimir Zapolskiy wrote: >> Omnivision OG01A1B camera sensor is supplied by three power rails, >> if supplies are present as device properties, include them into >> the sensor power up sequence. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> >> --- >> drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++- >> 1 file changed, 85 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c >> index 90a68201f43f..0150fdd2f424 100644 >> --- a/drivers/media/i2c/og01a1b.c >> +++ b/drivers/media/i2c/og01a1b.c >> @@ -9,6 +9,7 @@ >> #include <linux/i2c.h> >> #include <linux/module.h> >> #include <linux/pm_runtime.h> >> +#include <linux/regulator/consumer.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-device.h> >> #include <media/v4l2-fwnode.h> >> @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = { >> struct og01a1b { >> struct clk *xvclk; >> struct gpio_desc *reset_gpio; >> + struct regulator *avdd; >> + struct regulator *dovdd; >> + struct regulator *dvdd; >> >> struct v4l2_subdev sd; >> struct media_pad pad; >> @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev) >> { >> struct v4l2_subdev *sd = dev_get_drvdata(dev); >> struct og01a1b *og01a1b = to_og01a1b(sd); >> + int ret; >> + >> + if (og01a1b->avdd) { >> + ret = regulator_enable(og01a1b->avdd); >> + if (ret) >> + return ret; >> + } >> + >> + if (og01a1b->dovdd) { >> + ret = regulator_enable(og01a1b->dovdd); >> + if (ret) >> + goto avdd_disable; >> + } >> + >> + if (og01a1b->dvdd) { >> + ret = regulator_enable(og01a1b->dvdd); >> + if (ret) >> + goto dovdd_disable; >> + } >> >> gpiod_set_value_cansleep(og01a1b->reset_gpio, 0); >> usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); >> >> - return clk_prepare_enable(og01a1b->xvclk); >> + ret = clk_prepare_enable(og01a1b->xvclk); >> + if (ret) >> + goto dvdd_disable; > > Virtually all sensors require some delay between lifting the reset (which > typically comes after enabling the regulators and the clock) and the first > I²C access. This one appears to require 8192 XCLK cycles. According to the spec in case of a non-free running clock the delay mentioned by you is needed after enabling the clock, and in turn the clock is enabled after a release on a reset/shutdown GPIO line. The only supported by the driver clock frequency OG01A1B_MCLK is 19.2 MHz, so the expected delay after start of the clock is less than 1/10 of a delay after releasing the shutdown line and before entering the software standby state. I believe it would make sense to rearrange the power up sequence in this way: - enable clock, - if there is a reset GPIO, then release it and wait for 5ms (1ms is too short), - otherwise if there is no reset GPIO, then wait for 8192 XCLK cycles (0.5ms). -- Best wishes, Vladimir
diff --git a/drivers/media/i2c/og01a1b.c b/drivers/media/i2c/og01a1b.c index 90a68201f43f..0150fdd2f424 100644 --- a/drivers/media/i2c/og01a1b.c +++ b/drivers/media/i2c/og01a1b.c @@ -9,6 +9,7 @@ #include <linux/i2c.h> #include <linux/module.h> #include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> #include <media/v4l2-fwnode.h> @@ -422,6 +423,9 @@ static const struct og01a1b_mode supported_modes[] = { struct og01a1b { struct clk *xvclk; struct gpio_desc *reset_gpio; + struct regulator *avdd; + struct regulator *dovdd; + struct regulator *dvdd; struct v4l2_subdev sd; struct media_pad pad; @@ -982,11 +986,46 @@ static int og01a1b_power_on(struct device *dev) { struct v4l2_subdev *sd = dev_get_drvdata(dev); struct og01a1b *og01a1b = to_og01a1b(sd); + int ret; + + if (og01a1b->avdd) { + ret = regulator_enable(og01a1b->avdd); + if (ret) + return ret; + } + + if (og01a1b->dovdd) { + ret = regulator_enable(og01a1b->dovdd); + if (ret) + goto avdd_disable; + } + + if (og01a1b->dvdd) { + ret = regulator_enable(og01a1b->dvdd); + if (ret) + goto dovdd_disable; + } gpiod_set_value_cansleep(og01a1b->reset_gpio, 0); usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); - return clk_prepare_enable(og01a1b->xvclk); + ret = clk_prepare_enable(og01a1b->xvclk); + if (ret) + goto dvdd_disable; + + return 0; + +dvdd_disable: + if (og01a1b->dvdd) + regulator_disable(og01a1b->dvdd); +dovdd_disable: + if (og01a1b->dovdd) + regulator_disable(og01a1b->dovdd); +avdd_disable: + if (og01a1b->avdd) + regulator_disable(og01a1b->avdd); + + return ret; } static int og01a1b_power_off(struct device *dev) @@ -998,6 +1037,15 @@ static int og01a1b_power_off(struct device *dev) gpiod_set_value_cansleep(og01a1b->reset_gpio, 1); + if (og01a1b->dvdd) + regulator_disable(og01a1b->dvdd); + + if (og01a1b->dovdd) + regulator_disable(og01a1b->dovdd); + + if (og01a1b->avdd) + regulator_disable(og01a1b->avdd); + return 0; } @@ -1045,6 +1093,42 @@ static int og01a1b_probe(struct i2c_client *client) return PTR_ERR(og01a1b->reset_gpio); } + og01a1b->avdd = devm_regulator_get_optional(&client->dev, "avdd"); + if (IS_ERR(og01a1b->avdd)) { + ret = PTR_ERR(og01a1b->avdd); + if (ret != -ENODEV) { + dev_err_probe(&client->dev, ret, + "Failed to get 'avdd' regulator\n"); + return ret; + } + + og01a1b->avdd = NULL; + } + + og01a1b->dovdd = devm_regulator_get_optional(&client->dev, "dovdd"); + if (IS_ERR(og01a1b->dovdd)) { + ret = PTR_ERR(og01a1b->dovdd); + if (ret != -ENODEV) { + dev_err_probe(&client->dev, ret, + "Failed to get 'dovdd' regulator\n"); + return ret; + } + + og01a1b->dovdd = NULL; + } + + og01a1b->dvdd = devm_regulator_get_optional(&client->dev, "dvdd"); + if (IS_ERR(og01a1b->dvdd)) { + ret = PTR_ERR(og01a1b->dvdd); + if (ret != -ENODEV) { + dev_err_probe(&client->dev, ret, + "Failed to get 'dvdd' regulator\n"); + return ret; + } + + og01a1b->dvdd = NULL; + } + /* The sensor must be powered on to read the CHIP_ID register */ ret = og01a1b_power_on(&client->dev); if (ret)
Omnivision OG01A1B camera sensor is supplied by three power rails, if supplies are present as device properties, include them into the sensor power up sequence. Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> --- drivers/media/i2c/og01a1b.c | 86 ++++++++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 1 deletion(-)