Re: [PATCH 02/21] regulator: fixed: Convert to use GPIO descriptor only

[Andy Shevchenko]

On Mon, 2018-02-12 at 14:16 +0100, 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.
> 
> 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.)

Hi, Linus!

Nice patch, though I have comments below.

> --- a/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c

> @@ -43,7 +43,6 @@ static struct fixed_voltage_config bcm43xx_vmmc = {
>  	 * real voltage and signaling are still 1.8V.
>  	 */
>  	.microvolts		= 2000000,		/* 1.8V
> */
> -	.gpio			= -EINVAL,
>  	.startup_delay		= 250 * 1000,		/*
> 250ms */
>  	.enable_high		= 1,			/*
> active high */
>  	.enabled_at_boot	= 0,			/*
> disabled at boot */
> @@ -58,11 +57,25 @@ static struct platform_device
> bcm43xx_vmmc_regulator = {
>  	},
>  };
>  
> +static struct gpiod_lookup_table bcm43xx_vmmc_gpio_table = {
> +	.dev_id	= "reg-fixed-voltage.0",

I'm not sure this will be always like this.
We have DEVID_AUTO, which theoretically can be anything.

Okay, it looks like we have only one static regulator for now for Intel
MID. Though it's fragile if anything will change in the future (quite
unlikely).

> +	.table	= {

> +		/* CHECKME: is this the correct PCI address for the
> GPIO controller? */

Yes.

> +		GPIO_LOOKUP("0000:00:0c.0", -1, "enable",
> GPIO_ACTIVE_LOW),

> +		{ },

Terminator should terminate even at compile time, right?

Simple

{}

looks better to me.

> +	},
> +};
> +
>  static int __init bcm43xx_regulator_register(void)
>  {
> +	struct gpiod_lookup_table *table = &bcm43xx_vmmc_gpio_table;
> +	struct gpiod_lookup *lookup = table->table;
>  	int ret;
>  
> -	bcm43xx_vmmc.gpio =
> get_gpio_by_name(WLAN_SFI_GPIO_ENABLE_NAME);

> +	/* FIXME: convert SFI layer to use GPIO descriptors
> internally */

We already discussed this few years ago and decide not to do anything
WRT SFI. It should just die.

> +	lookup[0].chip_hwnum =
> get_gpio_by_name(WLAN_SFI_GPIO_ENABLE_NAME);
> +	gpiod_add_lookup_table(table);



> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c


>  	cfg.ena_gpio_invert = !config->enable_high;

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

Just a side note: It might make sense to split this to some kind of
generic helper, like:

static inline enum gpiod_flags gpiod_flags_output(bool value, bool
invert)
{
...
}

> +	if (config->gpio_is_open_drain) {
> +		if (gflags == GPIOD_OUT_HIGH)
> +			gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
> +		else
> +			gflags = GPIOD_OUT_LOW_OPEN_DRAIN;
> +	}
> +

> +	cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL,
> gflags);

Shouldn't we be a little bit more stricter here, i.e. require "enable"
name?

> +	if (IS_ERR(cfg.ena_gpiod))
> +		return PTR_ERR(cfg.ena_gpiod);

-- 
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy