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

[Linus Walleij <linus.walleij@linaro.org>]

On Wed, Oct 3, 2018 at 8:08 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
> 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;
>         }

GPIOD_* flags nowadays but yes.

> 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.

Yep, I can't take the credit for that, I just tried to preserve the
logic that was already there.

> 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.

If the bootloader set up the line value, isn't it reasonable to assume
it also set up the direction (to output)? How else would it even work?

That said, you could just call gpiod_direction_output() after
getting the handle if you want to make sure it is set as output.

We are floating patches that will re-enable the GPIO code to call
.get_direction() on all descriptors at boot. After that you can use
gpiod_get_direction() to determine if it needs to be switched,
but that would be a bit overengineered IMO.

I guess you can make a patch using GPIOD_ASIS, but I am worried
that several systems will depend on the active driving of this since
as you describe, it has been like this for ages.

Yet again I'm not the conservative kind, as you notice, so by all means
try it! :)

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

That one was especially interesting!

Yours,
Linus Walleij