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

[Keerthy]

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 at 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 at 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

>