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

[Przemyslaw Marczak <p.marczak@samsung.com>]

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

-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com