From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Thu, 15 Sep 2016 13:42:02 +0200 Subject: [U-Boot] [PATCH] power: regulator: Add support for gpio regulators In-Reply-To: References: <1473927905-7679-1-git-send-email-j-keerthy@ti.com> <57DA6B1D.5010709@samsung.com> Message-ID: <57DA890A.4050504@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> --- >>> 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, >>> + * Keerthy >>> + * >>> + * SPDX-License-Identifier: GPL-2.0+ >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +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 -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com