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

15 Sep 2016

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

    