| Message ID | 1473927905-7679-1-git-send-email-j-keerthy@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
Hello Keerthy, On 09/15/2016 10:25 AM, Keerthy wrote: > Add support for gpio regulators. As of now this driver caters > to gpio regulators with one gpio. Supports setting voltage values to gpio > regulators and retrieving the values. > > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > drivers/power/regulator/Kconfig | 8 ++ > drivers/power/regulator/Makefile | 1 + > drivers/power/regulator/gpio-regulator.c | 136 +++++++++++++++++++++++++++++++ > include/power/regulator.h | 1 + > 4 files changed, 146 insertions(+) > create mode 100644 drivers/power/regulator/gpio-regulator.c > > diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig > index 2510474..4776011 100644 > --- a/drivers/power/regulator/Kconfig > +++ b/drivers/power/regulator/Kconfig > @@ -58,6 +58,14 @@ config DM_REGULATOR_FIXED > features for fixed value regulators. The driver implements get/set api > for enable and get only for voltage value. > > +config DM_REGULATOR_GPIO > + bool "Enable Driver Model for GPIO REGULATOR" > + depends on DM_REGULATOR > + ---help--- > + This config enables implementation of driver-model regulator uclass > + features for gpio regulators. The driver implements get/set for > + voltage value. > + > config REGULATOR_RK808 > bool "Enable driver for RK808 regulators" > depends on DM_REGULATOR && PMIC_RK808 > diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile > index 2093048..2d350cb 100644 > --- a/drivers/power/regulator/Makefile > +++ b/drivers/power/regulator/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o > obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o > obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o > obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o > +obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o > obj-$(CONFIG_REGULATOR_RK808) += rk808.o > obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o > obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o > diff --git a/drivers/power/regulator/gpio-regulator.c b/drivers/power/regulator/gpio-regulator.c > new file mode 100644 > index 0000000..9c8832e > --- /dev/null > +++ b/drivers/power/regulator/gpio-regulator.c > @@ -0,0 +1,136 @@ > +/* > + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com> > + * Keerthy <j-keerthy@ti.com> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <fdtdec.h> > +#include <errno.h> > +#include <dm.h> > +#include <i2c.h> > +#include <asm/gpio.h> > +#include <power/pmic.h> > +#include <power/regulator.h> > + > +DECLARE_GLOBAL_DATA_PTR; > + > +struct gpio_regulator_platdata { > + struct gpio_desc gpio; /* GPIO for regulator voltage control */ > + int gpio_low_uV; > + int gpio_high_uV; > +}; The low/high values can be provided by regulator's uclass driver in struct of type "dm_regulator_uclass_platdata", as for other regulators. But as I can see in the Linux, this driver should provide a structure for the gpio states. > + > +static int gpio_regulator_ofdata_to_platdata(struct udevice *dev) > +{ > + struct dm_regulator_uclass_platdata *uc_pdata; > + struct gpio_regulator_platdata *dev_pdata; > + struct gpio_desc *gpio; > + const void *blob = gd->fdt_blob; > + int node = dev->of_offset; > + int ret, count, i; > + u32 states_array[8]; > + > + dev_pdata = dev_get_platdata(dev); > + uc_pdata = dev_get_uclass_platdata(dev); > + if (!uc_pdata) > + return -ENXIO; > + > + /* Set type to gpio */ > + uc_pdata->type = REGULATOR_TYPE_GPIO; > + > + /* > + * Get gpio regulator gpio desc > + * Assuming one GPIO per regulator. > + * Can be extended later to multiple GPIOs > + * per gpio-regulator. As of now no instance with multiple > + * gpios is presnt > + */ > + gpio = &dev_pdata->gpio; > + ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); > + if (ret) > + debug("regulator gpio - not found! Error: %d", ret); > + > + count = fdtdec_get_int_array_count(blob, node, "states", > + states_array, 8); > + > + if (!count) > + return -EINVAL; > + > + for (i = 0; i < count; i += 2) { The below condition is valid for most devices. In Linux we can find dts for some ARMs in which I can find at least two boards, with inverted meaning of state/voltage, so "0", can be also valid for the high uV. I think, this driver should keep possible states in platdata. > + if (states_array[i + 1] == 0) > + dev_pdata->gpio_low_uV = states_array[i]; > + else > + dev_pdata->gpio_high_uV = states_array[i]; > + } > + > + return 0; > +} > + > +static int gpio_regulator_get_value(struct udevice *dev) > +{ > + struct dm_regulator_uclass_platdata *uc_pdata; > + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); > + int enable; > + > + if (!dev_pdata->gpio.dev) > + return -ENOSYS; > + > + uc_pdata = dev_get_uclass_platdata(dev); > + if (uc_pdata->min_uV > uc_pdata->max_uV) { > + debug("Invalid constraints for: %s\n", uc_pdata->name); > + return -EINVAL; > + } > + > + enable = dm_gpio_get_value(&dev_pdata->gpio); The returned value (enable) should be compared to the states kept in platdata. > + if (enable) > + return dev_pdata->gpio_high_uV; > + else > + return dev_pdata->gpio_low_uV; > +} > + > +static int gpio_regulator_set_value(struct udevice *dev, int uV) > +{ > + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); > + int ret; > + bool enable; > + > + if (!dev_pdata->gpio.dev) > + return -ENOSYS; > + > + if (uV == dev_pdata->gpio_high_uV) > + enable = 1; > + else if (uV == dev_pdata->gpio_low_uV) > + enable = 0; > + else > + return -EINVAL; > + And also here you should get the "enable" value from states kept in platdata. > + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); > + if (ret) { > + error("Can't set regulator : %s gpio to: %d\n", dev->name, > + enable); > + return ret; > + } > + > + return 0; > +} > + > +static const struct dm_regulator_ops gpio_regulator_ops = { > + .get_value = gpio_regulator_get_value, > + .set_value = gpio_regulator_set_value, > +}; > + > +static const struct udevice_id gpio_regulator_ids[] = { > + { .compatible = "regulator-gpio" }, > + { }, > +}; > + > +U_BOOT_DRIVER(gpio_regulator) = { > + .name = "gpio regulator", > + .id = UCLASS_REGULATOR, > + .ops = &gpio_regulator_ops, > + .of_match = gpio_regulator_ids, > + .ofdata_to_platdata = gpio_regulator_ofdata_to_platdata, > + .platdata_auto_alloc_size = sizeof(struct gpio_regulator_platdata), > +}; > diff --git a/include/power/regulator.h b/include/power/regulator.h > index 8ae6b14..5d327e6 100644 > --- a/include/power/regulator.h > +++ b/include/power/regulator.h > @@ -108,6 +108,7 @@ enum regulator_type { > REGULATOR_TYPE_BUCK, > REGULATOR_TYPE_DVS, > REGULATOR_TYPE_FIXED, > + REGULATOR_TYPE_GPIO, > REGULATOR_TYPE_OTHER, > }; > Best regards,
On Thursday 15 September 2016 03:04 PM, Przemyslaw Marczak wrote: > Hello Keerthy, > > On 09/15/2016 10:25 AM, Keerthy wrote: >> Add support for gpio regulators. As of now this driver caters >> to gpio regulators with one gpio. Supports setting voltage values to gpio >> regulators and retrieving the values. >> >> Signed-off-by: Keerthy <j-keerthy@ti.com> >> --- >> drivers/power/regulator/Kconfig | 8 ++ >> drivers/power/regulator/Makefile | 1 + >> drivers/power/regulator/gpio-regulator.c | 136 >> +++++++++++++++++++++++++++++++ >> include/power/regulator.h | 1 + >> 4 files changed, 146 insertions(+) >> create mode 100644 drivers/power/regulator/gpio-regulator.c >> >> diff --git a/drivers/power/regulator/Kconfig >> b/drivers/power/regulator/Kconfig >> index 2510474..4776011 100644 >> --- a/drivers/power/regulator/Kconfig >> +++ b/drivers/power/regulator/Kconfig >> @@ -58,6 +58,14 @@ config DM_REGULATOR_FIXED >> features for fixed value regulators. The driver implements >> get/set api >> for enable and get only for voltage value. >> +config DM_REGULATOR_GPIO >> + bool "Enable Driver Model for GPIO REGULATOR" >> + depends on DM_REGULATOR >> + ---help--- >> + This config enables implementation of driver-model regulator uclass >> + features for gpio regulators. The driver implements get/set for >> + voltage value. >> + >> config REGULATOR_RK808 >> bool "Enable driver for RK808 regulators" >> depends on DM_REGULATOR && PMIC_RK808 >> diff --git a/drivers/power/regulator/Makefile >> b/drivers/power/regulator/Makefile >> index 2093048..2d350cb 100644 >> --- a/drivers/power/regulator/Makefile >> +++ b/drivers/power/regulator/Makefile >> @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o >> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >> obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o >> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o >> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o >> obj-$(CONFIG_REGULATOR_RK808) += rk808.o >> obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o >> obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o >> diff --git a/drivers/power/regulator/gpio-regulator.c >> b/drivers/power/regulator/gpio-regulator.c >> new file mode 100644 >> index 0000000..9c8832e >> --- /dev/null >> +++ b/drivers/power/regulator/gpio-regulator.c >> @@ -0,0 +1,136 @@ >> +/* >> + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com> >> + * Keerthy <j-keerthy@ti.com> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <fdtdec.h> >> +#include <errno.h> >> +#include <dm.h> >> +#include <i2c.h> >> +#include <asm/gpio.h> >> +#include <power/pmic.h> >> +#include <power/regulator.h> >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +struct gpio_regulator_platdata { >> + struct gpio_desc gpio; /* GPIO for regulator voltage control */ >> + int gpio_low_uV; >> + int gpio_high_uV; >> +}; > > The low/high values can be provided by regulator's uclass driver in > struct of type "dm_regulator_uclass_platdata", as for other regulators. min_uV, max_uV represent the minimum and maximum voltages in dm_regulator_uclass_platdata. I would not use them here. > > But as I can see in the Linux, this driver should provide a structure > for the gpio states. Yes so i am keeping the voltage values for 0 and 1 states in a driver specific struct. > >> + >> +static int gpio_regulator_ofdata_to_platdata(struct udevice *dev) >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + struct gpio_regulator_platdata *dev_pdata; >> + struct gpio_desc *gpio; >> + const void *blob = gd->fdt_blob; >> + int node = dev->of_offset; >> + int ret, count, i; >> + u32 states_array[8]; >> + >> + dev_pdata = dev_get_platdata(dev); >> + uc_pdata = dev_get_uclass_platdata(dev); >> + if (!uc_pdata) >> + return -ENXIO; >> + >> + /* Set type to gpio */ >> + uc_pdata->type = REGULATOR_TYPE_GPIO; >> + >> + /* >> + * Get gpio regulator gpio desc >> + * Assuming one GPIO per regulator. >> + * Can be extended later to multiple GPIOs >> + * per gpio-regulator. As of now no instance with multiple >> + * gpios is presnt >> + */ >> + gpio = &dev_pdata->gpio; >> + ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); >> + if (ret) >> + debug("regulator gpio - not found! Error: %d", ret); >> + >> + count = fdtdec_get_int_array_count(blob, node, "states", >> + states_array, 8); >> + >> + if (!count) >> + return -EINVAL; >> + >> + for (i = 0; i < count; i += 2) { > The below condition is valid for most devices. > > In Linux we can find dts for some ARMs in which I can find at least two > boards, > with inverted meaning of state/voltage, so "0", can be also valid for > the high uV. > > I think, this driver should keep possible states in platdata. Okay. I get the point. So i will have both states and corresponding states voltages stored in gpio_regulator_platdata structure and set states corresponding to requested voltages. > >> + if (states_array[i + 1] == 0) >> + dev_pdata->gpio_low_uV = states_array[i]; >> + else >> + dev_pdata->gpio_high_uV = states_array[i]; >> + } >> + >> + return 0; >> +} >> + >> +static int gpio_regulator_get_value(struct udevice *dev) >> +{ >> + struct dm_regulator_uclass_platdata *uc_pdata; >> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >> + int enable; >> + >> + if (!dev_pdata->gpio.dev) >> + return -ENOSYS; >> + >> + uc_pdata = dev_get_uclass_platdata(dev); >> + if (uc_pdata->min_uV > uc_pdata->max_uV) { >> + debug("Invalid constraints for: %s\n", uc_pdata->name); >> + return -EINVAL; >> + } >> + >> + enable = dm_gpio_get_value(&dev_pdata->gpio); > The returned value (enable) should be compared to the states kept in > platdata. Sure. > >> + if (enable) >> + return dev_pdata->gpio_high_uV; >> + else >> + return dev_pdata->gpio_low_uV; >> +} >> + >> +static int gpio_regulator_set_value(struct udevice *dev, int uV) >> +{ >> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >> + int ret; >> + bool enable; >> + >> + if (!dev_pdata->gpio.dev) >> + return -ENOSYS; >> + >> + if (uV == dev_pdata->gpio_high_uV) >> + enable = 1; >> + else if (uV == dev_pdata->gpio_low_uV) >> + enable = 0; >> + else >> + return -EINVAL; >> + > > And also here you should get the "enable" value from states kept in > platdata. Okay. > >> + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); >> + if (ret) { >> + error("Can't set regulator : %s gpio to: %d\n", dev->name, >> + enable); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static const struct dm_regulator_ops gpio_regulator_ops = { >> + .get_value = gpio_regulator_get_value, >> + .set_value = gpio_regulator_set_value, >> +}; >> + >> +static const struct udevice_id gpio_regulator_ids[] = { >> + { .compatible = "regulator-gpio" }, >> + { }, >> +}; >> + >> +U_BOOT_DRIVER(gpio_regulator) = { >> + .name = "gpio regulator", >> + .id = UCLASS_REGULATOR, >> + .ops = &gpio_regulator_ops, >> + .of_match = gpio_regulator_ids, >> + .ofdata_to_platdata = gpio_regulator_ofdata_to_platdata, >> + .platdata_auto_alloc_size = sizeof(struct gpio_regulator_platdata), >> +}; >> diff --git a/include/power/regulator.h b/include/power/regulator.h >> index 8ae6b14..5d327e6 100644 >> --- a/include/power/regulator.h >> +++ b/include/power/regulator.h >> @@ -108,6 +108,7 @@ enum regulator_type { >> REGULATOR_TYPE_BUCK, >> REGULATOR_TYPE_DVS, >> REGULATOR_TYPE_FIXED, >> + REGULATOR_TYPE_GPIO, >> REGULATOR_TYPE_OTHER, >> }; >> > > Best regards, Thanks for the review, - Keerthy >
On 09/15/2016 12:34 PM, Keerthy wrote: > > > On Thursday 15 September 2016 03:04 PM, Przemyslaw Marczak wrote: >> Hello Keerthy, >> >> On 09/15/2016 10:25 AM, Keerthy wrote: >>> Add support for gpio regulators. As of now this driver caters >>> to gpio regulators with one gpio. Supports setting voltage values to >>> gpio >>> regulators and retrieving the values. >>> >>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>> --- >>> drivers/power/regulator/Kconfig | 8 ++ >>> drivers/power/regulator/Makefile | 1 + >>> drivers/power/regulator/gpio-regulator.c | 136 >>> +++++++++++++++++++++++++++++++ >>> include/power/regulator.h | 1 + >>> 4 files changed, 146 insertions(+) >>> create mode 100644 drivers/power/regulator/gpio-regulator.c >>> >>> diff --git a/drivers/power/regulator/Kconfig >>> b/drivers/power/regulator/Kconfig >>> index 2510474..4776011 100644 >>> --- a/drivers/power/regulator/Kconfig >>> +++ b/drivers/power/regulator/Kconfig >>> @@ -58,6 +58,14 @@ config DM_REGULATOR_FIXED >>> features for fixed value regulators. The driver implements >>> get/set api >>> for enable and get only for voltage value. >>> +config DM_REGULATOR_GPIO >>> + bool "Enable Driver Model for GPIO REGULATOR" >>> + depends on DM_REGULATOR >>> + ---help--- >>> + This config enables implementation of driver-model regulator >>> uclass >>> + features for gpio regulators. The driver implements get/set for >>> + voltage value. >>> + >>> config REGULATOR_RK808 >>> bool "Enable driver for RK808 regulators" >>> depends on DM_REGULATOR && PMIC_RK808 >>> diff --git a/drivers/power/regulator/Makefile >>> b/drivers/power/regulator/Makefile >>> index 2093048..2d350cb 100644 >>> --- a/drivers/power/regulator/Makefile >>> +++ b/drivers/power/regulator/Makefile >>> @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o >>> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >>> obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o >>> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o >>> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o >>> obj-$(CONFIG_REGULATOR_RK808) += rk808.o >>> obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o >>> obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o >>> diff --git a/drivers/power/regulator/gpio-regulator.c >>> b/drivers/power/regulator/gpio-regulator.c >>> new file mode 100644 >>> index 0000000..9c8832e >>> --- /dev/null >>> +++ b/drivers/power/regulator/gpio-regulator.c >>> @@ -0,0 +1,136 @@ >>> +/* >>> + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com> >>> + * Keerthy <j-keerthy@ti.com> >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include <common.h> >>> +#include <fdtdec.h> >>> +#include <errno.h> >>> +#include <dm.h> >>> +#include <i2c.h> >>> +#include <asm/gpio.h> >>> +#include <power/pmic.h> >>> +#include <power/regulator.h> >>> + >>> +DECLARE_GLOBAL_DATA_PTR; >>> + >>> +struct gpio_regulator_platdata { >>> + struct gpio_desc gpio; /* GPIO for regulator voltage control */ >>> + int gpio_low_uV; >>> + int gpio_high_uV; >>> +}; >> >> The low/high values can be provided by regulator's uclass driver in >> struct of type "dm_regulator_uclass_platdata", as for other regulators. > > min_uV, max_uV represent the minimum and maximum voltages in > dm_regulator_uclass_platdata. I would not use them here. > Ah right, sorry I just get wrong the name of those variables above - as min/max uV, but your point was about the GPIO lo/hi state. >> >> But as I can see in the Linux, this driver should provide a structure >> for the gpio states. > > Yes so i am keeping the voltage values for 0 and 1 states in a driver > specific struct. > >> >>> + >>> +static int gpio_regulator_ofdata_to_platdata(struct udevice *dev) >>> +{ >>> + struct dm_regulator_uclass_platdata *uc_pdata; >>> + struct gpio_regulator_platdata *dev_pdata; >>> + struct gpio_desc *gpio; >>> + const void *blob = gd->fdt_blob; >>> + int node = dev->of_offset; >>> + int ret, count, i; >>> + u32 states_array[8]; >>> + >>> + dev_pdata = dev_get_platdata(dev); >>> + uc_pdata = dev_get_uclass_platdata(dev); >>> + if (!uc_pdata) >>> + return -ENXIO; >>> + >>> + /* Set type to gpio */ >>> + uc_pdata->type = REGULATOR_TYPE_GPIO; >>> + >>> + /* >>> + * Get gpio regulator gpio desc >>> + * Assuming one GPIO per regulator. >>> + * Can be extended later to multiple GPIOs >>> + * per gpio-regulator. As of now no instance with multiple >>> + * gpios is presnt >>> + */ >>> + gpio = &dev_pdata->gpio; >>> + ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); >>> + if (ret) >>> + debug("regulator gpio - not found! Error: %d", ret); >>> + >>> + count = fdtdec_get_int_array_count(blob, node, "states", >>> + states_array, 8); >>> + >>> + if (!count) >>> + return -EINVAL; >>> + >>> + for (i = 0; i < count; i += 2) { >> The below condition is valid for most devices. >> >> In Linux we can find dts for some ARMs in which I can find at least two >> boards, >> with inverted meaning of state/voltage, so "0", can be also valid for >> the high uV. >> >> I think, this driver should keep possible states in platdata. > > Okay. I get the point. So i will have both states and corresponding > states voltages stored in gpio_regulator_platdata structure and set > states corresponding to requested voltages. > Yes, so actually the only issue is low/high interpretation. Maybe the easiest way is a simple array with the voltage, since there are only two states. But maybe the name of gpio_low/hi.. is quite misleading. What about an array gpio_state_uV[2] or something like that? Then you can use GPIO state's true/false as array index for get method. >> >>> + if (states_array[i + 1] == 0) >>> + dev_pdata->gpio_low_uV = states_array[i]; >>> + else >>> + dev_pdata->gpio_high_uV = states_array[i]; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int gpio_regulator_get_value(struct udevice *dev) >>> +{ >>> + struct dm_regulator_uclass_platdata *uc_pdata; >>> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >>> + int enable; >>> + >>> + if (!dev_pdata->gpio.dev) >>> + return -ENOSYS; >>> + >>> + uc_pdata = dev_get_uclass_platdata(dev); >>> + if (uc_pdata->min_uV > uc_pdata->max_uV) { >>> + debug("Invalid constraints for: %s\n", uc_pdata->name); >>> + return -EINVAL; >>> + } >>> + >>> + enable = dm_gpio_get_value(&dev_pdata->gpio); >> The returned value (enable) should be compared to the states kept in >> platdata. > > Sure. > >> >>> + if (enable) >>> + return dev_pdata->gpio_high_uV; >>> + else >>> + return dev_pdata->gpio_low_uV; >>> +} >>> + >>> +static int gpio_regulator_set_value(struct udevice *dev, int uV) >>> +{ >>> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >>> + int ret; >>> + bool enable; >>> + >>> + if (!dev_pdata->gpio.dev) >>> + return -ENOSYS; >>> + >>> + if (uV == dev_pdata->gpio_high_uV) >>> + enable = 1; >>> + else if (uV == dev_pdata->gpio_low_uV) >>> + enable = 0; >>> + else >>> + return -EINVAL; >>> + >> >> And also here you should get the "enable" value from states kept in >> platdata. > > Okay. > >> >>> + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); >>> + if (ret) { >>> + error("Can't set regulator : %s gpio to: %d\n", dev->name, >>> + enable); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static const struct dm_regulator_ops gpio_regulator_ops = { >>> + .get_value = gpio_regulator_get_value, >>> + .set_value = gpio_regulator_set_value, >>> +}; >>> + >>> +static const struct udevice_id gpio_regulator_ids[] = { >>> + { .compatible = "regulator-gpio" }, >>> + { }, >>> +}; >>> + >>> +U_BOOT_DRIVER(gpio_regulator) = { >>> + .name = "gpio regulator", >>> + .id = UCLASS_REGULATOR, >>> + .ops = &gpio_regulator_ops, >>> + .of_match = gpio_regulator_ids, >>> + .ofdata_to_platdata = gpio_regulator_ofdata_to_platdata, >>> + .platdata_auto_alloc_size = sizeof(struct >>> gpio_regulator_platdata), >>> +}; >>> diff --git a/include/power/regulator.h b/include/power/regulator.h >>> index 8ae6b14..5d327e6 100644 >>> --- a/include/power/regulator.h >>> +++ b/include/power/regulator.h >>> @@ -108,6 +108,7 @@ enum regulator_type { >>> REGULATOR_TYPE_BUCK, >>> REGULATOR_TYPE_DVS, >>> REGULATOR_TYPE_FIXED, >>> + REGULATOR_TYPE_GPIO, >>> REGULATOR_TYPE_OTHER, >>> }; >>> >> >> Best regards, > > Thanks for the review, > - Keerthy > You are welcome
On Thursday 15 September 2016 05:12 PM, Przemyslaw Marczak wrote: > > > On 09/15/2016 12:34 PM, Keerthy wrote: >> >> >> On Thursday 15 September 2016 03:04 PM, Przemyslaw Marczak wrote: >>> Hello Keerthy, >>> >>> On 09/15/2016 10:25 AM, Keerthy wrote: >>>> Add support for gpio regulators. As of now this driver caters >>>> to gpio regulators with one gpio. Supports setting voltage values to >>>> gpio >>>> regulators and retrieving the values. >>>> >>>> Signed-off-by: Keerthy <j-keerthy@ti.com> >>>> --- >>>> drivers/power/regulator/Kconfig | 8 ++ >>>> drivers/power/regulator/Makefile | 1 + >>>> drivers/power/regulator/gpio-regulator.c | 136 >>>> +++++++++++++++++++++++++++++++ >>>> include/power/regulator.h | 1 + >>>> 4 files changed, 146 insertions(+) >>>> create mode 100644 drivers/power/regulator/gpio-regulator.c >>>> >>>> diff --git a/drivers/power/regulator/Kconfig >>>> b/drivers/power/regulator/Kconfig >>>> index 2510474..4776011 100644 >>>> --- a/drivers/power/regulator/Kconfig >>>> +++ b/drivers/power/regulator/Kconfig >>>> @@ -58,6 +58,14 @@ config DM_REGULATOR_FIXED >>>> features for fixed value regulators. The driver implements >>>> get/set api >>>> for enable and get only for voltage value. >>>> +config DM_REGULATOR_GPIO >>>> + bool "Enable Driver Model for GPIO REGULATOR" >>>> + depends on DM_REGULATOR >>>> + ---help--- >>>> + This config enables implementation of driver-model regulator >>>> uclass >>>> + features for gpio regulators. The driver implements get/set for >>>> + voltage value. >>>> + >>>> config REGULATOR_RK808 >>>> bool "Enable driver for RK808 regulators" >>>> depends on DM_REGULATOR && PMIC_RK808 >>>> diff --git a/drivers/power/regulator/Makefile >>>> b/drivers/power/regulator/Makefile >>>> index 2093048..2d350cb 100644 >>>> --- a/drivers/power/regulator/Makefile >>>> +++ b/drivers/power/regulator/Makefile >>>> @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o >>>> obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o >>>> obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o >>>> obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o >>>> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o >>>> obj-$(CONFIG_REGULATOR_RK808) += rk808.o >>>> obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o >>>> obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o >>>> diff --git a/drivers/power/regulator/gpio-regulator.c >>>> b/drivers/power/regulator/gpio-regulator.c >>>> new file mode 100644 >>>> index 0000000..9c8832e >>>> --- /dev/null >>>> +++ b/drivers/power/regulator/gpio-regulator.c >>>> @@ -0,0 +1,136 @@ >>>> +/* >>>> + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com> >>>> + * Keerthy <j-keerthy@ti.com> >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0+ >>>> + */ >>>> + >>>> +#include <common.h> >>>> +#include <fdtdec.h> >>>> +#include <errno.h> >>>> +#include <dm.h> >>>> +#include <i2c.h> >>>> +#include <asm/gpio.h> >>>> +#include <power/pmic.h> >>>> +#include <power/regulator.h> >>>> + >>>> +DECLARE_GLOBAL_DATA_PTR; >>>> + >>>> +struct gpio_regulator_platdata { >>>> + struct gpio_desc gpio; /* GPIO for regulator voltage control */ >>>> + int gpio_low_uV; >>>> + int gpio_high_uV; >>>> +}; >>> >>> The low/high values can be provided by regulator's uclass driver in >>> struct of type "dm_regulator_uclass_platdata", as for other regulators. >> >> min_uV, max_uV represent the minimum and maximum voltages in >> dm_regulator_uclass_platdata. I would not use them here. >> > > Ah right, sorry I just get wrong the name of those variables above - as > min/max uV, but your point was about the GPIO lo/hi state. > >>> >>> But as I can see in the Linux, this driver should provide a structure >>> for the gpio states. >> >> Yes so i am keeping the voltage values for 0 and 1 states in a driver >> specific struct. >> >>> >>>> + >>>> +static int gpio_regulator_ofdata_to_platdata(struct udevice *dev) >>>> +{ >>>> + struct dm_regulator_uclass_platdata *uc_pdata; >>>> + struct gpio_regulator_platdata *dev_pdata; >>>> + struct gpio_desc *gpio; >>>> + const void *blob = gd->fdt_blob; >>>> + int node = dev->of_offset; >>>> + int ret, count, i; >>>> + u32 states_array[8]; >>>> + >>>> + dev_pdata = dev_get_platdata(dev); >>>> + uc_pdata = dev_get_uclass_platdata(dev); >>>> + if (!uc_pdata) >>>> + return -ENXIO; >>>> + >>>> + /* Set type to gpio */ >>>> + uc_pdata->type = REGULATOR_TYPE_GPIO; >>>> + >>>> + /* >>>> + * Get gpio regulator gpio desc >>>> + * Assuming one GPIO per regulator. >>>> + * Can be extended later to multiple GPIOs >>>> + * per gpio-regulator. As of now no instance with multiple >>>> + * gpios is presnt >>>> + */ >>>> + gpio = &dev_pdata->gpio; >>>> + ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); >>>> + if (ret) >>>> + debug("regulator gpio - not found! Error: %d", ret); >>>> + >>>> + count = fdtdec_get_int_array_count(blob, node, "states", >>>> + states_array, 8); >>>> + >>>> + if (!count) >>>> + return -EINVAL; >>>> + >>>> + for (i = 0; i < count; i += 2) { >>> The below condition is valid for most devices. >>> >>> In Linux we can find dts for some ARMs in which I can find at least two >>> boards, >>> with inverted meaning of state/voltage, so "0", can be also valid for >>> the high uV. >>> >>> I think, this driver should keep possible states in platdata. >> >> Okay. I get the point. So i will have both states and corresponding >> states voltages stored in gpio_regulator_platdata structure and set >> states corresponding to requested voltages. >> > > Yes, so actually the only issue is low/high interpretation. > > Maybe the easiest way is a simple array with the voltage, > since there are only two states. But maybe the name of gpio_low/hi.. > is quite misleading. What about an array gpio_state_uV[2] or something > like that? > > Then you can use GPIO state's true/false as array index for get method. Yes. I sent a V2 :-) with your comments fixed. > >>> >>>> + if (states_array[i + 1] == 0) >>>> + dev_pdata->gpio_low_uV = states_array[i]; >>>> + else >>>> + dev_pdata->gpio_high_uV = states_array[i]; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int gpio_regulator_get_value(struct udevice *dev) >>>> +{ >>>> + struct dm_regulator_uclass_platdata *uc_pdata; >>>> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >>>> + int enable; >>>> + >>>> + if (!dev_pdata->gpio.dev) >>>> + return -ENOSYS; >>>> + >>>> + uc_pdata = dev_get_uclass_platdata(dev); >>>> + if (uc_pdata->min_uV > uc_pdata->max_uV) { >>>> + debug("Invalid constraints for: %s\n", uc_pdata->name); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + enable = dm_gpio_get_value(&dev_pdata->gpio); >>> The returned value (enable) should be compared to the states kept in >>> platdata. >> >> Sure. >> >>> >>>> + if (enable) >>>> + return dev_pdata->gpio_high_uV; >>>> + else >>>> + return dev_pdata->gpio_low_uV; >>>> +} >>>> + >>>> +static int gpio_regulator_set_value(struct udevice *dev, int uV) >>>> +{ >>>> + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); >>>> + int ret; >>>> + bool enable; >>>> + >>>> + if (!dev_pdata->gpio.dev) >>>> + return -ENOSYS; >>>> + >>>> + if (uV == dev_pdata->gpio_high_uV) >>>> + enable = 1; >>>> + else if (uV == dev_pdata->gpio_low_uV) >>>> + enable = 0; >>>> + else >>>> + return -EINVAL; >>>> + >>> >>> And also here you should get the "enable" value from states kept in >>> platdata. >> >> Okay. >> >>> >>>> + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); >>>> + if (ret) { >>>> + error("Can't set regulator : %s gpio to: %d\n", dev->name, >>>> + enable); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static const struct dm_regulator_ops gpio_regulator_ops = { >>>> + .get_value = gpio_regulator_get_value, >>>> + .set_value = gpio_regulator_set_value, >>>> +}; >>>> + >>>> +static const struct udevice_id gpio_regulator_ids[] = { >>>> + { .compatible = "regulator-gpio" }, >>>> + { }, >>>> +}; >>>> + >>>> +U_BOOT_DRIVER(gpio_regulator) = { >>>> + .name = "gpio regulator", >>>> + .id = UCLASS_REGULATOR, >>>> + .ops = &gpio_regulator_ops, >>>> + .of_match = gpio_regulator_ids, >>>> + .ofdata_to_platdata = gpio_regulator_ofdata_to_platdata, >>>> + .platdata_auto_alloc_size = sizeof(struct >>>> gpio_regulator_platdata), >>>> +}; >>>> diff --git a/include/power/regulator.h b/include/power/regulator.h >>>> index 8ae6b14..5d327e6 100644 >>>> --- a/include/power/regulator.h >>>> +++ b/include/power/regulator.h >>>> @@ -108,6 +108,7 @@ enum regulator_type { >>>> REGULATOR_TYPE_BUCK, >>>> REGULATOR_TYPE_DVS, >>>> REGULATOR_TYPE_FIXED, >>>> + REGULATOR_TYPE_GPIO, >>>> REGULATOR_TYPE_OTHER, >>>> }; >>>> >>> >>> Best regards, >> >> Thanks for the review, >> - Keerthy >> > > You are welcome >
diff --git a/drivers/power/regulator/Kconfig b/drivers/power/regulator/Kconfig index 2510474..4776011 100644 --- a/drivers/power/regulator/Kconfig +++ b/drivers/power/regulator/Kconfig @@ -58,6 +58,14 @@ config DM_REGULATOR_FIXED features for fixed value regulators. The driver implements get/set api for enable and get only for voltage value. +config DM_REGULATOR_GPIO + bool "Enable Driver Model for GPIO REGULATOR" + depends on DM_REGULATOR + ---help--- + This config enables implementation of driver-model regulator uclass + features for gpio regulators. The driver implements get/set for + voltage value. + config REGULATOR_RK808 bool "Enable driver for RK808 regulators" depends on DM_REGULATOR && PMIC_RK808 diff --git a/drivers/power/regulator/Makefile b/drivers/power/regulator/Makefile index 2093048..2d350cb 100644 --- a/drivers/power/regulator/Makefile +++ b/drivers/power/regulator/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_REGULATOR_ACT8846) += act8846.o obj-$(CONFIG_DM_REGULATOR_MAX77686) += max77686.o obj-$(CONFIG_DM_REGULATOR_PFUZE100) += pfuze100.o obj-$(CONFIG_$(SPL_)DM_REGULATOR_FIXED) += fixed.o +obj-$(CONFIG_$(SPL_)DM_REGULATOR_GPIO) += gpio-regulator.o obj-$(CONFIG_REGULATOR_RK808) += rk808.o obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o diff --git a/drivers/power/regulator/gpio-regulator.c b/drivers/power/regulator/gpio-regulator.c new file mode 100644 index 0000000..9c8832e --- /dev/null +++ b/drivers/power/regulator/gpio-regulator.c @@ -0,0 +1,136 @@ +/* + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com> + * Keerthy <j-keerthy@ti.com> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <fdtdec.h> +#include <errno.h> +#include <dm.h> +#include <i2c.h> +#include <asm/gpio.h> +#include <power/pmic.h> +#include <power/regulator.h> + +DECLARE_GLOBAL_DATA_PTR; + +struct gpio_regulator_platdata { + struct gpio_desc gpio; /* GPIO for regulator voltage control */ + int gpio_low_uV; + int gpio_high_uV; +}; + +static int gpio_regulator_ofdata_to_platdata(struct udevice *dev) +{ + struct dm_regulator_uclass_platdata *uc_pdata; + struct gpio_regulator_platdata *dev_pdata; + struct gpio_desc *gpio; + const void *blob = gd->fdt_blob; + int node = dev->of_offset; + int ret, count, i; + u32 states_array[8]; + + dev_pdata = dev_get_platdata(dev); + uc_pdata = dev_get_uclass_platdata(dev); + if (!uc_pdata) + return -ENXIO; + + /* Set type to gpio */ + uc_pdata->type = REGULATOR_TYPE_GPIO; + + /* + * Get gpio regulator gpio desc + * Assuming one GPIO per regulator. + * Can be extended later to multiple GPIOs + * per gpio-regulator. As of now no instance with multiple + * gpios is presnt + */ + gpio = &dev_pdata->gpio; + ret = gpio_request_by_name(dev, "gpios", 0, gpio, GPIOD_IS_OUT); + if (ret) + debug("regulator gpio - not found! Error: %d", ret); + + count = fdtdec_get_int_array_count(blob, node, "states", + states_array, 8); + + if (!count) + return -EINVAL; + + for (i = 0; i < count; i += 2) { + if (states_array[i + 1] == 0) + dev_pdata->gpio_low_uV = states_array[i]; + else + dev_pdata->gpio_high_uV = states_array[i]; + } + + return 0; +} + +static int gpio_regulator_get_value(struct udevice *dev) +{ + struct dm_regulator_uclass_platdata *uc_pdata; + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); + int enable; + + if (!dev_pdata->gpio.dev) + return -ENOSYS; + + uc_pdata = dev_get_uclass_platdata(dev); + if (uc_pdata->min_uV > uc_pdata->max_uV) { + debug("Invalid constraints for: %s\n", uc_pdata->name); + return -EINVAL; + } + + enable = dm_gpio_get_value(&dev_pdata->gpio); + if (enable) + return dev_pdata->gpio_high_uV; + else + return dev_pdata->gpio_low_uV; +} + +static int gpio_regulator_set_value(struct udevice *dev, int uV) +{ + struct gpio_regulator_platdata *dev_pdata = dev_get_platdata(dev); + int ret; + bool enable; + + if (!dev_pdata->gpio.dev) + return -ENOSYS; + + if (uV == dev_pdata->gpio_high_uV) + enable = 1; + else if (uV == dev_pdata->gpio_low_uV) + enable = 0; + else + return -EINVAL; + + ret = dm_gpio_set_value(&dev_pdata->gpio, enable); + if (ret) { + error("Can't set regulator : %s gpio to: %d\n", dev->name, + enable); + return ret; + } + + return 0; +} + +static const struct dm_regulator_ops gpio_regulator_ops = { + .get_value = gpio_regulator_get_value, + .set_value = gpio_regulator_set_value, +}; + +static const struct udevice_id gpio_regulator_ids[] = { + { .compatible = "regulator-gpio" }, + { }, +}; + +U_BOOT_DRIVER(gpio_regulator) = { + .name = "gpio regulator", + .id = UCLASS_REGULATOR, + .ops = &gpio_regulator_ops, + .of_match = gpio_regulator_ids, + .ofdata_to_platdata = gpio_regulator_ofdata_to_platdata, + .platdata_auto_alloc_size = sizeof(struct gpio_regulator_platdata), +}; diff --git a/include/power/regulator.h b/include/power/regulator.h index 8ae6b14..5d327e6 100644 --- a/include/power/regulator.h +++ b/include/power/regulator.h @@ -108,6 +108,7 @@ enum regulator_type { REGULATOR_TYPE_BUCK, REGULATOR_TYPE_DVS, REGULATOR_TYPE_FIXED, + REGULATOR_TYPE_GPIO, REGULATOR_TYPE_OTHER, };
Add support for gpio regulators. As of now this driver caters to gpio regulators with one gpio. Supports setting voltage values to gpio regulators and retrieving the values. Signed-off-by: Keerthy <j-keerthy@ti.com> --- drivers/power/regulator/Kconfig | 8 ++ drivers/power/regulator/Makefile | 1 + drivers/power/regulator/gpio-regulator.c | 136 +++++++++++++++++++++++++++++++ include/power/regulator.h | 1 + 4 files changed, 146 insertions(+) create mode 100644 drivers/power/regulator/gpio-regulator.c