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

[Thierry Reding]

On Fri, Jun 01, 2018 at 11:35:11AM +0200, Thierry Reding wrote:
> 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

Forwarding this to linux-tegra for visibility.

Thierry

Attachment: signature.asc
Description: PGP signature