| Message ID | 20241128152338.4583-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov2740: Various improvements | expand |
On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On some designs the regulators for the AVDD / DOVDD / DVDD power rails > are controlled by Linux. > > Add support to the driver for getting regulators for these 3 rails and > for enabling these regulators when necessary. > > The datasheet specifies a delay of 0ns between enabling the regulators, > IOW they can all 3 be enabled at the same time. This allows using the bulk > regulator API. > > The regulator core will provide dummy regulators for the 3 power-rails > when necessary. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Ricardo Ribalda <ribalda@chromium.org> Do we need to update the device tree with this change? (honest question :), I have no idea) > --- > drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 14d0a0588cc2..c766c1f83e12 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -11,6 +11,7 @@ > #include <linux/pm_runtime.h> > #include <linux/nvmem-provider.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -76,6 +77,14 @@ > /* OTP registers from sensor */ > #define OV2740_REG_OTP_CUSTOMER 0x7010 > > +static const char * const ov2740_supply_name[] = { > + "AVDD", > + "DOVDD", > + "DVDD", > +}; > + > +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name) > + > struct nvm_data { > struct nvmem_device *nvmem; > struct regmap *regmap; > @@ -523,10 +532,11 @@ struct ov2740 { > struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > > - /* GPIOs, clocks */ > + /* GPIOs, clocks, regulators */ > struct gpio_desc *reset_gpio; > struct gpio_desc *powerdown_gpio; > struct clk *clk; > + struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES]; > > /* Current mode */ > const struct ov2740_mode *cur_mode; > @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev) > gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > clk_disable_unprepare(ov2740->clk); > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > return 0; > } > > @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev) > struct ov2740 *ov2740 = to_ov2740(sd); > int ret; > > - ret = clk_prepare_enable(ov2740->clk); > + ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies); > if (ret) > return ret; > > + ret = clk_prepare_enable(ov2740->clk); > + if (ret) { > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > + return ret; > + } > + > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > msleep(20); > @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct ov2740 *ov2740; > bool full_power; > - int ret; > + int i, ret; > > ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL); > if (!ov2740) > @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client) > return dev_err_probe(dev, PTR_ERR(ov2740->clk), > "failed to get clock\n"); > > + for (i = 0; i < OV2740_NUM_SUPPLIES; i++) > + ov2740->supplies[i].supply = ov2740_supply_name[i]; > + > + ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies); > + if (ret) > + return dev_err_probe(dev, ret, "failed to get regulators\n"); > + > full_power = acpi_dev_state_d0(&client->dev); > if (full_power) { > /* ACPI does not always clear the reset GPIO / enable the clock */ > -- > 2.47.0 > >
On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote: > On some designs the regulators for the AVDD / DOVDD / DVDD power rails > are controlled by Linux. > > Add support to the driver for getting regulators for these 3 rails and > for enabling these regulators when necessary. > > The datasheet specifies a delay of 0ns between enabling the regulators, > IOW they can all 3 be enabled at the same time. This allows using the bulk > regulator API. > > The regulator core will provide dummy regulators for the 3 power-rails > when necessary. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 14d0a0588cc2..c766c1f83e12 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -11,6 +11,7 @@ > #include <linux/pm_runtime.h> > #include <linux/nvmem-provider.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -76,6 +77,14 @@ > /* OTP registers from sensor */ > #define OV2740_REG_OTP_CUSTOMER 0x7010 > > +static const char * const ov2740_supply_name[] = { > + "AVDD", > + "DOVDD", > + "DVDD", > +}; > + > +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name) > + > struct nvm_data { > struct nvmem_device *nvmem; > struct regmap *regmap; > @@ -523,10 +532,11 @@ struct ov2740 { > struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > > - /* GPIOs, clocks */ > + /* GPIOs, clocks, regulators */ > struct gpio_desc *reset_gpio; > struct gpio_desc *powerdown_gpio; > struct clk *clk; > + struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES]; > > /* Current mode */ > const struct ov2740_mode *cur_mode; > @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev) > gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > clk_disable_unprepare(ov2740->clk); > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > return 0; > } > > @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev) > struct ov2740 *ov2740 = to_ov2740(sd); > int ret; > > - ret = clk_prepare_enable(ov2740->clk); > + ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies); > if (ret) > return ret; > > + ret = clk_prepare_enable(ov2740->clk); > + if (ret) { > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > + return ret; > + } > + > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > msleep(20); > @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct ov2740 *ov2740; > bool full_power; > - int ret; > + int i, ret; > > ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL); > if (!ov2740) > @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client) > return dev_err_probe(dev, PTR_ERR(ov2740->clk), > "failed to get clock\n"); > > + for (i = 0; i < OV2740_NUM_SUPPLIES; i++) > + ov2740->supplies[i].supply = ov2740_supply_name[i]; > + > + ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies); > + if (ret) > + return dev_err_probe(dev, ret, "failed to get regulators\n"); > + > full_power = acpi_dev_state_d0(&client->dev); > if (full_power) { > /* ACPI does not always clear the reset GPIO / enable the clock */ > -- > 2.47.0 > >
Hi Hans, Thanks for the set. On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote: > On some designs the regulators for the AVDD / DOVDD / DVDD power rails > are controlled by Linux. > > Add support to the driver for getting regulators for these 3 rails and > for enabling these regulators when necessary. > > The datasheet specifies a delay of 0ns between enabling the regulators, > IOW they can all 3 be enabled at the same time. This allows using the bulk > regulator API. > > The regulator core will provide dummy regulators for the 3 power-rails > when necessary. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 14d0a0588cc2..c766c1f83e12 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -11,6 +11,7 @@ > #include <linux/pm_runtime.h> > #include <linux/nvmem-provider.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -76,6 +77,14 @@ > /* OTP registers from sensor */ > #define OV2740_REG_OTP_CUSTOMER 0x7010 > > +static const char * const ov2740_supply_name[] = { > + "AVDD", > + "DOVDD", > + "DVDD", > +}; > + > +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name) > + > struct nvm_data { > struct nvmem_device *nvmem; > struct regmap *regmap; > @@ -523,10 +532,11 @@ struct ov2740 { > struct v4l2_ctrl *hblank; > struct v4l2_ctrl *exposure; > > - /* GPIOs, clocks */ > + /* GPIOs, clocks, regulators */ > struct gpio_desc *reset_gpio; > struct gpio_desc *powerdown_gpio; > struct clk *clk; > + struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES]; This would be cleaner with plain ARRAY_SIZE(). Same below. I.e. the macro is redundant. > > /* Current mode */ > const struct ov2740_mode *cur_mode; > @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev) > gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > clk_disable_unprepare(ov2740->clk); > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > return 0; > } > > @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev) > struct ov2740 *ov2740 = to_ov2740(sd); > int ret; > > - ret = clk_prepare_enable(ov2740->clk); > + ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies); > if (ret) > return ret; > > + ret = clk_prepare_enable(ov2740->clk); > + if (ret) { > + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); > + return ret; > + } > + > gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > msleep(20); > @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client) > struct device *dev = &client->dev; > struct ov2740 *ov2740; > bool full_power; > - int ret; > + int i, ret; unsigned int i > > ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL); > if (!ov2740) > @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client) > return dev_err_probe(dev, PTR_ERR(ov2740->clk), > "failed to get clock\n"); > > + for (i = 0; i < OV2740_NUM_SUPPLIES; i++) > + ov2740->supplies[i].supply = ov2740_supply_name[i]; > + > + ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies); > + if (ret) > + return dev_err_probe(dev, ret, "failed to get regulators\n"); > + > full_power = acpi_dev_state_d0(&client->dev); > if (full_power) { > /* ACPI does not always clear the reset GPIO / enable the clock */
Hi Ricardo, On Thu, Nov 28, 2024 at 05:50:33PM +0100, Ricardo Ribalda Delgado wrote: > On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > > On some designs the regulators for the AVDD / DOVDD / DVDD power rails > > are controlled by Linux. > > > > Add support to the driver for getting regulators for these 3 rails and > > for enabling these regulators when necessary. > > > > The datasheet specifies a delay of 0ns between enabling the regulators, > > IOW they can all 3 be enabled at the same time. This allows using the bulk > > regulator API. > > > > The regulator core will provide dummy regulators for the 3 power-rails > > when necessary. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Ricardo Ribalda <ribalda@chromium.org> > > Do we need to update the device tree with this change? (honest > question :), I have no idea) This driver seems to be ACPI-only.
Hi, On 13-Dec-24 10:04 AM, Sakari Ailus wrote: > Hi Hans, > > Thanks for the set. > > On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote: >> On some designs the regulators for the AVDD / DOVDD / DVDD power rails >> are controlled by Linux. >> >> Add support to the driver for getting regulators for these 3 rails and >> for enabling these regulators when necessary. >> >> The datasheet specifies a delay of 0ns between enabling the regulators, >> IOW they can all 3 be enabled at the same time. This allows using the bulk >> regulator API. >> >> The regulator core will provide dummy regulators for the 3 power-rails >> when necessary. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++--- >> 1 file changed, 27 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c >> index 14d0a0588cc2..c766c1f83e12 100644 >> --- a/drivers/media/i2c/ov2740.c >> +++ b/drivers/media/i2c/ov2740.c >> @@ -11,6 +11,7 @@ >> #include <linux/pm_runtime.h> >> #include <linux/nvmem-provider.h> >> #include <linux/regmap.h> >> +#include <linux/regulator/consumer.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-device.h> >> #include <media/v4l2-fwnode.h> >> @@ -76,6 +77,14 @@ >> /* OTP registers from sensor */ >> #define OV2740_REG_OTP_CUSTOMER 0x7010 >> >> +static const char * const ov2740_supply_name[] = { >> + "AVDD", >> + "DOVDD", >> + "DVDD", >> +}; >> + >> +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name) >> + >> struct nvm_data { >> struct nvmem_device *nvmem; >> struct regmap *regmap; >> @@ -523,10 +532,11 @@ struct ov2740 { >> struct v4l2_ctrl *hblank; >> struct v4l2_ctrl *exposure; >> >> - /* GPIOs, clocks */ >> + /* GPIOs, clocks, regulators */ >> struct gpio_desc *reset_gpio; >> struct gpio_desc *powerdown_gpio; >> struct clk *clk; >> + struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES]; > > This would be cleaner with plain ARRAY_SIZE(). Same below. I.e. the macro > is redundant. Ok, I will change this for v2. > >> >> /* Current mode */ >> const struct ov2740_mode *cur_mode; >> @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev) >> gpiod_set_value_cansleep(ov2740->reset_gpio, 1); >> gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); >> clk_disable_unprepare(ov2740->clk); >> + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); >> return 0; >> } >> >> @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev) >> struct ov2740 *ov2740 = to_ov2740(sd); >> int ret; >> >> - ret = clk_prepare_enable(ov2740->clk); >> + ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies); >> if (ret) >> return ret; >> >> + ret = clk_prepare_enable(ov2740->clk); >> + if (ret) { >> + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); >> + return ret; >> + } >> + >> gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); >> gpiod_set_value_cansleep(ov2740->reset_gpio, 0); >> msleep(20); >> @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client) >> struct device *dev = &client->dev; >> struct ov2740 *ov2740; >> bool full_power; >> - int ret; >> + int i, ret; > > unsigned int i ack. Regards, Hans
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index 14d0a0588cc2..c766c1f83e12 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -11,6 +11,7 @@ #include <linux/pm_runtime.h> #include <linux/nvmem-provider.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> #include <media/v4l2-fwnode.h> @@ -76,6 +77,14 @@ /* OTP registers from sensor */ #define OV2740_REG_OTP_CUSTOMER 0x7010 +static const char * const ov2740_supply_name[] = { + "AVDD", + "DOVDD", + "DVDD", +}; + +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name) + struct nvm_data { struct nvmem_device *nvmem; struct regmap *regmap; @@ -523,10 +532,11 @@ struct ov2740 { struct v4l2_ctrl *hblank; struct v4l2_ctrl *exposure; - /* GPIOs, clocks */ + /* GPIOs, clocks, regulators */ struct gpio_desc *reset_gpio; struct gpio_desc *powerdown_gpio; struct clk *clk; + struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES]; /* Current mode */ const struct ov2740_mode *cur_mode; @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev) gpiod_set_value_cansleep(ov2740->reset_gpio, 1); gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); clk_disable_unprepare(ov2740->clk); + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); return 0; } @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev) struct ov2740 *ov2740 = to_ov2740(sd); int ret; - ret = clk_prepare_enable(ov2740->clk); + ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies); if (ret) return ret; + ret = clk_prepare_enable(ov2740->clk); + if (ret) { + regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies); + return ret; + } + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); gpiod_set_value_cansleep(ov2740->reset_gpio, 0); msleep(20); @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client) struct device *dev = &client->dev; struct ov2740 *ov2740; bool full_power; - int ret; + int i, ret; ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL); if (!ov2740) @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client) return dev_err_probe(dev, PTR_ERR(ov2740->clk), "failed to get clock\n"); + for (i = 0; i < OV2740_NUM_SUPPLIES; i++) + ov2740->supplies[i].supply = ov2740_supply_name[i]; + + ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies); + if (ret) + return dev_err_probe(dev, ret, "failed to get regulators\n"); + full_power = acpi_dev_state_d0(&client->dev); if (full_power) { /* ACPI does not always clear the reset GPIO / enable the clock */
On some designs the regulators for the AVDD / DOVDD / DVDD power rails are controlled by Linux. Add support to the driver for getting regulators for these 3 rails and for enabling these regulators when necessary. The datasheet specifies a delay of 0ns between enabling the regulators, IOW they can all 3 be enabled at the same time. This allows using the bulk regulator API. The regulator core will provide dummy regulators for the 3 power-rails when necessary. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)