Re: [PATCH] regulator: fixed: Default enable high on DT regulators

[Leonard Crestez <leonard.crestez@nxp.com>]

On Wed, 2018-10-03 at 13:10 +0100, Mark Brown wrote:
> On Tue, Oct 02, 2018 at 01:42:38PM +0000, Leonard Crestez wrote:
> 
> > This turns the phy off and on again instead of leaving it up from uboot
> > and it doesn't work for some reason. However looking at
> > reg_fixed_voltage_probe introducing an edge seems to be intentional for
> > regulators which are not marked with "enabled-at-boot". Right?
> 
> No, that's definitely not desired.  We don't want to change the state of
> the regulator at all if we can avoid it unless the user explicitly asked
> for it.

That also makes sense, for a top level perspective. But
reg_fixed_voltage_probe contains the following snippet:

	if (config->enabled_at_boot) {
		if (config->enable_high)
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
		else
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
	} else {
		if (config->enable_high)
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
		else
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
	}

Unless config->enabled_at_boot the GPIO is initialized with the
opposite polarity of enabled. This means that fixed regulators which
are not marked with "enabled-at-boot" but happen to be "on" anyway are
always power cycled.

This is from before recent changes by Linus, code dates from 2012
commit 25a53dfbfbfd ("regulator: fixed: Use core GPIO enable support").
Even before that the logic was similar:

	drvdata->is_enabled = config->enabled_at_boot;
	ret = drvdata->is_enabled ?
        	config->enable_high : !config->enable_high;
	gpio_flag = ret ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;

Looking further back it seems that this behavior has always been
present in fixed-regulator code.

In theory it might be possible to request the GPIO while asking to keep
the value from the bootloader? Maybe I'm confused but I don't see an
easy way to do this through the GPIO api; functions for requesting in
output mode all seem to also ask for the initial value.

GPIOD_ASIS looks close but it doesn't even adjust the direction.

> > It's possible that you exposed an imx board-specific bug: maybe power
> > cycling the phy after uboot needs some missing fixup?
> 
> It'd probably also be good to sort this out though.

Yes, handled separately: https://lore.kernel.org/patchwork/patch/994871/