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
[off-list ref] wrote:

quoted

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 <redacted>
Reviewed-by: Krzysztof Kozlowski <redacted>

(...)

quoted

+#include <linux/gpio/consumer.h>

THANKS for using modern interfaces!

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

quoted

+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 :)

quoted

+/**
+ * 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