| Message ID | 20240813102035.1763559-7-vladimir.zapolskiy@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: og01a1b: Add OF support to OmniVision OG01A1B | expand |
Quoting Vladimir Zapolskiy (2024-08-13 11:20:35) > Omnivision OG01A1B camera sensor is supplied by tree power rails, three? > if supplies are present as device properties, include them into > 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; > + } Perhaps regulator_bulk_enable()/regulator_bulk_disable() would be suitable here to reduce lots of repetitive code and error handling? > > 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) > -- > 2.45.2 >
Hi Kieran, On 8/13/24 15:53, Kieran Bingham wrote: > Quoting Vladimir Zapolskiy (2024-08-13 11:20:35) >> Omnivision OG01A1B camera sensor is supplied by tree power rails, > > three? that's it, thanks for catching the typo. >> if supplies are present as device properties, include them into >> 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; >> + } > > Perhaps regulator_bulk_enable()/regulator_bulk_disable() would be > suitable here to reduce lots of repetitive code and error handling? It won't be possible to support optional regulators with bulk operations, thus likely it's not an option. Note, that in ACPI case there are no regulators at all, at least this should be functionally preserved in the driver. It might be an option to define all supply regulators as strictly required for the OF case, but since there is actually no need for it, I'm reluctant to push the limits into the device tree node schema. So, from my opinion the left option is the published one. -- 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 tree power rails, if supplies are present as device properties, include them into 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(-)