Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only

From: Thierry Reding
Date: Fri Jun 01 2018 - 05:35:22 EST

Next message: Thierry Reding: "Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only"
Previous message: Sergey Senozhatsky: "Re: [PATCH 2/2] printk: make sure to print log on console."
Next in thread: Thierry Reding: "Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 14, 2018 at 10:06:22AM +0200, Linus Walleij wrote:
> As we augmented the regulator core to accept a GPIO descriptor instead
> of a GPIO number, we can augment the fixed GPIO regulator to look up
> and pass that descriptor directly from device tree or board GPIO
> descriptor look up tables.
>
> Some boards just auto-enumerate their fixed regulator platform devices
> and I have assumed they get names like "fixed-regulator.0" but it's
> pretty hard to guess this. I need some testing from board maintainers to
> be sure. Other boards are straight forward, using just plain
> "fixed-regulator" (ID -1) or "fixed-regulator.1" hammering down the
> device ID.
>
> The OMAP didn't have proper label names on its GPIO chips so I have fixed
> this with a separate patch to the GPIO tree, see
> commit 088413bc0bd5f5fb66ca22a19d66a49d7154ba4c
> "gpio: omap: Give unique labels to each GPIO bank/chip"
>
> It seems the da9055 and da9211 has never got around to actually passing
> any enable gpio into its platform data (not the in-tree code anyway) so we
> can just decide to simply pass a descriptor instead.
>
> The fixed GPIO-controlled regulator in mach-pxa/ezx.c was confusingly named
> "*_dummy_supply_device" while it is a very real device backed by a GPIO
> line. There is nothing dummy about it at all, so I renamed it with the
> infix *_regulator_* as part of this patch set.
>
> For the patch hunk hitting arch/blackfin I would say I do not expect
> testing, review or ACKs anymore so if it works, it works.
>
> The hunk hitting the x86 BCM43xx driver is especially tricky as the number
> comes out of SFI which is a mystery to me. I definately need someone to
> look at this. (Hi Andy.)
>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> # Check the x86 BCM stuff
> Cc: Alexander Shiyan <shc_work@xxxxxxx> # i.MX boards user
> Cc: Haojian Zhuang <haojian.zhuang@xxxxxxxxx> # MMP2 maintainer
> Cc: Aaro Koskinen <aaro.koskinen@xxxxxx> # OMAP1 maintainer
> Cc: Tony Lindgren <tony@xxxxxxxxxxx> # OMAP1,2,3 maintainer
> Cc: Mike Rapoport <rppt@xxxxxxxxxxxxxxxxxx> # EM-X270 maintainer
> Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx> # EZX maintainer
> Cc: Philipp Zabel <philipp.zabel@xxxxxxxxx> # Magician maintainer
> Cc: Daniel Mack <zonque@xxxxxxxxx> # Raumfeld maintainer
> Cc: Marc Zyngier <marc.zyngier@xxxxxxx> # Zeus maintainer
> Cc: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> # SuperH pinctrl/GPIO maintainer
> Cc: Russell King <rmk+kernel@xxxxxxxxxxxxxxx> # SA1100
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> ChangeLog v2->v3:
> - Resending.
> ChangeLog v1->v2:
> - Rebase the patch on mainline with Blackfin gone and other changes.
> - Fix up the new users that appeared in sa1100
> - Drop some suplus comments in x86.
> ---
> arch/arm/mach-imx/mach-mx21ads.c | 13 +++++++-
> arch/arm/mach-imx/mach-mx27ads.c | 12 ++++++-
> arch/arm/mach-mmp/brownstone.c | 12 ++++++-
> arch/arm/mach-omap1/board-ams-delta.c | 14 +++++++-
> arch/arm/mach-omap2/pdata-quirks.c | 16 ++++++++-
> arch/arm/mach-pxa/em-x270.c | 1 -
> arch/arm/mach-pxa/ezx.c | 33 ++++++++++++-------
> arch/arm/mach-pxa/magician.c | 2 +-
> arch/arm/mach-pxa/raumfeld.c | 12 +++++--
> arch/arm/mach-pxa/zeus.c | 23 +++++++++++--
> arch/arm/mach-s3c64xx/mach-crag6410.c | 1 -
> arch/arm/mach-s3c64xx/mach-smdk6410.c | 1 -
> arch/arm/mach-sa1100/assabet.c | 21 ++++++++----
> arch/arm/mach-sa1100/generic.c | 5 +--
> arch/arm/mach-sa1100/generic.h | 3 +-
> arch/arm/mach-sa1100/shannon.c | 4 +--
> arch/sh/boards/mach-ecovec24/setup.c | 22 +++++++++++--
> .../intel-mid/device_libs/platform_bcm43xx.c | 17 ++++++++--
> drivers/regulator/fixed-helper.c | 1 -
> drivers/regulator/fixed.c | 33 +++++++++----------
> include/linux/regulator/fixed.h | 3 --
> 21 files changed, 187 insertions(+), 62 deletions(-)

This causes an HDMI display regression on Jetson TK1. From what I can
tell, the problem is that we now get a double-inversion for low-active
GPIOs. For example, we have this in the Jetson TK1 device tree:

vdd_hdmi_pll: regulator@11 {
compatible = "regulator-fixed";
reg = <11>;
regulator-name = "+1.05V_RUN_AVDD_HDMI_PLL";
regulator-min-microvolt = <1050000>;
regulator-max-microvolt = <1050000>;
gpio = <&gpio TEGRA_GPIO(H, 7) GPIO_ACTIVE_LOW>;
vin-supply = <&vdd_1v05_run>;
};

We've got GPIO_ACTIVE_LOW for the regulator's enable GPIO and since we
don't have enable-active-high, the regulator core will treat the GPIO as
low active. The presence of the GPIO_ACTIVE_LOW flag will cause the GPIO
polarity to be inversed, transparently in gpiolib, and the lack of the
enable-active-high property causes the GPIO polarity to inversed as
well, so we effectively end up with a high-active enable GPIO for one
which should really be low-active.

This has always been a bit of an ambiguity since we've had two places
for expressing the polarity. But I think given the move to pervasively
using GPIO descriptors, it'd be reasonable to just ignore the
enable-active-high property, or perhaps warn if we stumble across a
low-active GPIO (via the flags in the specifier) if the regulator is
also marked enable-active-high.

> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 988a7472c2ab..1142f195529b 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -24,10 +24,9 @@
> #include <linux/platform_device.h>
> #include <linux/regulator/driver.h>
> #include <linux/regulator/fixed.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/slab.h>
> #include <linux/of.h>
> -#include <linux/of_gpio.h>
> #include <linux/regulator/of_regulator.h>
> #include <linux/regulator/machine.h>
>
> @@ -78,10 +77,6 @@ of_get_fixed_voltage_config(struct device *dev,
> if (init_data->constraints.boot_on)
> config->enabled_at_boot = true;
>
> - config->gpio = of_get_named_gpio(np, "gpio", 0);
> - if ((config->gpio < 0) && (config->gpio != -ENOENT))
> - return ERR_PTR(config->gpio);
> -
> of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
>
> config->enable_high = of_property_read_bool(np, "enable-active-high");
> @@ -102,6 +97,7 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
> struct fixed_voltage_config *config;
> struct fixed_voltage_data *drvdata;
> struct regulator_config cfg = { };
> + enum gpiod_flags gflags;
> int ret;
>
> drvdata = devm_kzalloc(&pdev->dev, sizeof(struct fixed_voltage_data),
> @@ -150,25 +146,28 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
>
> drvdata->desc.fixed_uV = config->microvolts;
>
> - if (gpio_is_valid(config->gpio)) {
> - cfg.ena_gpio = config->gpio;
> - if (pdev->dev.of_node)
> - cfg.ena_gpio_initialized = true;
> - }
> cfg.ena_gpio_invert = !config->enable_high;

Change this line to:

cfg.ena_gpio_invert = false;

fixes the regression and is pretty much the implementation of my above
suggestion to ignore enable-active-high, though we may eventually want
to get rid of ena_gpio_invert altogether, provided that everyone has
moved over to GPIO descriptors.

Thierry

Attachment: signature.asc
Description: PGP signature

Next message: Thierry Reding: "Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only"
Previous message: Sergey Senozhatsky: "Re: [PATCH 2/2] printk: make sure to print log on console."
Next in thread: Thierry Reding: "Re: [PATCH 01/19 v3] regulator: fixed: Convert to use GPIO descriptor only"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]