Re: [U-Boot] [PATCH] power: regulator: Add support for gpio regulators

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