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

[Keerthy]

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