linux-kernel - Re: [PATCH v7 08/24] mfd: max77686: Add Dynamic Voltage Scaling (DVS) support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <53BF351C.8060008@collabora.co.uk>
Date:	Fri, 11 Jul 2014 02:51:40 +0200
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Lee Jones <lee.jones@...aro.org>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Mike Turquette <mturquette@...aro.org>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Yadwinder Singh Brar <yadi.brar01@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Doug Anderson <dianders@...omium.org>,
	Tushar Behera <trblinux@...il.com>,
	Mark Brown <broonie@...nel.org>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	Olof Johansson <olof@...om.net>,
	Andreas Farber <afaerber@...e.de>,
	Alexandre Courbot <gnurou@...il.com>,
	Rob Jones <rob.jones@...ethink.co.uk>
Subject: Re: [PATCH v7 08/24] mfd: max77686: Add Dynamic Voltage Scaling (DVS)
 support

Hello Linus,

On 07/10/2014 11:46 AM, Linus Walleij wrote:
> On Fri, Jul 4, 2014 at 10:24 PM, Javier Martinez Canillas
> <javier.martinez@...labora.co.uk> 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.martinez@...labora.co.uk>
>> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> 
> (...)
>> +#include <linux/gpio/consumer.h>
> 
> THANKS for using modern interfaces!
> 

Thanks to you and Alexandre for keep improving the GPIO subsystem!

>> +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);
>> +}
> 
> Rob Jones has a patch cooking that adds gpio_get_array() so this thing
> merits also adding devm_gpiod_get_array() I think?
> 

Yes, I just asked [0] Rob on the other thread if he is already implementing the
descriptor-based version of his devm_request_gpio_array() or if I should go and
implement it.

Now, I wonder if that can be done in a follow-up patch (e.g: use the new
devm_gpiod_get_array once it lands in Torvalds tree)  since this series already
touches several subsystems (mfd, regulators, clk and rtc) so if possible I would
prefer to not add another cross-subsystem dependency :)

>> +/**
>> + * max77686_setup_gpios() - init DVS-related GPIOs
>> + * @dev: device whose platform data contains the dvs GPIOs information
>> + *
>> + * 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);
> 
> Why does this have to be raw? Usually that is not to be used.
> 

Right, I can't think of a good reason why this has to be raw and not just use
gpiod_direction_output() which will check the active-low flag and set the value
accordingly. I'll change it on the next revision.

> Apart from this it looks OK.
> 

Great, thanks a lot for your feedback.

> Yours,
> Linus Walleij
> 

Best regards,
Javier

[0]: https://lkml.org/lkml/2014/7/10/722
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/