Hello Krzysztof,

Again, thanks a lot for taking the time to review the series.

On 07/04/2014 01:15 PM, Krzysztof Kozlowski wrote:
> On piÄ…, 2014-07-04 at 11:55 +0200, Javier Martinez Canillas wrote:
>> Some regulators on the MAX77686 PMIC have Dynamic Voltage Scaling
>> (DVS) support that allows output voltage to change dynamically.
>> 
>> For MAX77686, these regulators are Buck regulators 2, 3 and 4.
>> 
>> Each Buck output voltage is selected using a set of external
>> inputs: DVS1-3 and SELB2-4.
>> 
>> DVS registers can be used to configure the output voltages for each
>> Buck regulator and which one is active is controled by DVSx lines.
>> 
>> SELBx lines are used to control if individual Buck lines are ON or OFF.
>> 
>> This patch adds support to configure the DVSx and SELBx lines
>> from DT and to setup and read the GPIO lines connected to them.
>> 
>> Signed-off-by: Javier Martinez Canillas <javier.marti...@collabora.co.uk>
>> ---
>>  drivers/mfd/max77686.c       | 115 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/max77686.h |  18 ++++---
>>  2 files changed, 125 insertions(+), 8 deletions(-)
> 
> One minor comment below, but overall looks good to me:
> Reviewed-by: Krzysztof Kozlowski <k.kozlow...@samsung.com>
> 
> 
>> diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c
>> index 8650832..648d564 100644
>> --- a/drivers/mfd/max77686.c
>> +++ b/drivers/mfd/max77686.c
>> @@ -32,8 +32,10 @@
>>  #include <linux/mfd/core.h>
>>  #include <linux/mfd/max77686.h>
>>  #include <linux/mfd/max77686-private.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/err.h>
>>  #include <linux/of.h>
>> +#include <linux/export.h>
>>  
>>  #define I2C_ADDR_RTC        (0x0C >> 1)
>>  
>> @@ -101,9 +103,115 @@ static const struct of_device_id 
>> max77686_pmic_dt_match[] = {
>>      {},
>>  };
>>  
>> +static void max77686_dt_parse_dvs_gpio(struct device *dev)
>> +{
>> +    struct max77686_platform_data *pd = dev_get_platdata(dev);
>> +    int i;
>> +
>> +    /*
>> +     * NOTE: we don't consider GPIO errors fatal; board may have some lines
>> +     * directly pulled high or low and thus doesn't specify them.
>> +     */
>> +    for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++)
>> +            pd->buck_gpio_dvs[i] =
>> +                    devm_gpiod_get_index(dev, "max77686,pmic-buck-dvs", i);
>> +
>> +    for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++)
>> +            pd->buck_gpio_selb[i] =
>> +                    devm_gpiod_get_index(dev, "max77686,pmic-buck-selb", i);
>> +}
>> +
>> +/**
>> + * max77686_setup_gpios - init DVS-related GPIOs
>> + *
>> + * This function claims / initalizations GPIOs related to DVS if they are
>> + * defined. This may have the effect of switching voltages if the
>> + * pdata->buck_default_idx does not match the boot time state of pins.
>> + */
>> +int max77686_setup_gpios(struct device *dev)
>> +{
>> +    struct max77686_platform_data *pd = dev_get_platdata(dev);
>> +    int buck_default_idx = pd->buck_default_idx;
>> +    int ret;
>> +    int i;
>> +
>> +    /* Set all SELB high to avoid glitching while DVS is changing */
>> +    for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) {
>> +            struct gpio_desc *gpio = pd->buck_gpio_selb[i];
>> +
>> +            /* OK if some GPIOs aren't defined */
>> +            if (IS_ERR(gpio))
>> +                    continue;
>> +
>> +            ret = gpiod_direction_output_raw(gpio, 1);
>> +            if (ret) {
>> +                    dev_err(dev, "can't set gpio[%d] dir: %d\n", i, ret);
>> +                    return ret;
>> +            }
>> +    }
>> +
>> +    /* Set our initial setting */
>> +    for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_dvs); i++) {
>> +            struct gpio_desc *gpio = pd->buck_gpio_dvs[i];
>> +
>> +            /* OK if some GPIOs aren't defined */
>> +            if (IS_ERR(gpio))
>> +                    continue;
>> +
>> +            /* If a GPIO is valid, set it */
>> +            gpiod_direction_output(gpio, (buck_default_idx >> i) & 1);
>> +            if (ret) {
>> +                    dev_err(dev, "can't set gpio[%d]: dir %d\n", i, ret);
>> +                    return ret;
>> +            }
>> +    }
>> +
>> +    /* Now set SELB low to take effect */
>> +    for (i = 0; i < ARRAY_SIZE(pd->buck_gpio_selb); i++) {
>> +            struct gpio_desc *gpio = pd->buck_gpio_selb[i];
>> +
>> +            if (!IS_ERR(gpio))
>> +                    gpiod_set_value(gpio, 0);
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(max77686_setup_gpios);
>> +
>> +/**
>> + * max77686_read_gpios - read the current state of the dvs GPIOs
>> + *
>> + * We call this function at bootup to detect what slot the firmware was
>> + * using for the DVS GPIOs.  That way we can properly preserve the 
>> firmware's
>> + * voltage settings
>> + */
>> +int max77686_read_gpios(struct max77686_platform_data *pdata)
>> +{
> 
> Can you document that this function can sleep? It is quite obvious but
> the function is exported so maybe it is worth noting.
> 

Sure, I'll add that to the comment.

> Best regards,
> Krzysztof
> 

Best regards,
Javier

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to