Re: [PATCH 2/5 v8] regulator: fixed/gpio: Pull inversion/OD into gpiolib

From: Marek Szyprowski
Date: Tue Jan 08 2019 - 05:54:48 EST

Next message: Thomas Hellstrom: "Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs"
Previous message: Sibi Sankar: "Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Linus,

On 2019-01-07 17:27, Linus Walleij wrote:
> This pushes the handling of inversion semantics and open drain
> settings to the GPIO descriptor and gpiolib. All affected board
> files are also augmented.
>
> This is especially nice since we don't have to have any
> confusing flags passed around to the left and right littering
> the fixed and GPIO regulator drivers and the regulator core.
> It is all just very straight-forward: the core asks the GPIO
> line to be asserted or deasserted and gpiolib deals with the
> rest depending on how the platform is configured: if the line
> is active low, it deals with that, if the line is open drain,
> it deals with that too.
>
> 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: Petr Cvek <petr.cvek@xxxxxx> # Magician
> Cc: Robert Jarzmik <robert.jarzmik@xxxxxxx> # PXA
> Cc: Paul Parsons <lost.distance@xxxxxxxxx> # hx4700
> 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
> Tested-by: Janusz Krzysztofik <jmkrzyszt@xxxxxxxxx> #OMAP1 Amstrad Delta
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>

The whole patchset works fine on various Samsung Exynos boards I have
for tests.

BTW, I've noticed 2 cases in Exynos dts (exynos4412-odroidx.dts and
exynos5250-arndale.dts), where GPIO descriptor had active low flag, but
it was overridden by 'enable-active-high' property. I will correct those
to match just in case.

> ---
> ChangeLog v7->v8:
> - Rebase on v5.0-rc1.
> - Collected Janusz Tested-by tag for OMAP1.
> ChangeLog v6->v7:
> - Fix a missed .enable_high on OMAP1.
> ChangeLog v4->v6:
> - Split out parts relation to GPIO regulator descriptor conversion
> to the right patch.
> - Renumber to fit the rest of the series.
> - Daniel Mack says he will probably delete the Raumfeld board file
> and replace it with a device tree, I suggest we just deal with that
> conflict upstream.
> ChangeLog v3->v4:
> - Rebase on fixed regulator changes.
> ChangeLog v2->v3:
> - Resending.
> ChangeLog v1->v2:
> - Rebase the patch series
> - Cover the new user added in sa1100
> ---
> arch/arm/mach-imx/mach-mx21ads.c | 1 -
> arch/arm/mach-imx/mach-mx27ads.c | 2 +-
> arch/arm/mach-mmp/brownstone.c | 1 -
> arch/arm/mach-omap1/board-ams-delta.c | 2 --
> arch/arm/mach-omap2/pdata-quirks.c | 1 -
> arch/arm/mach-pxa/em-x270.c | 1 -
> arch/arm/mach-pxa/ezx.c | 3 +-
> arch/arm/mach-pxa/raumfeld.c | 1 -
> arch/arm/mach-pxa/zeus.c | 3 +-
> arch/arm/mach-sa1100/assabet.c | 1 -
> arch/sh/boards/mach-ecovec24/setup.c | 2 --
> .../intel-mid/device_libs/platform_bcm43xx.c | 1 -
> drivers/regulator/core.c | 8 ++---
> drivers/regulator/da9055-regulator.c | 1 -
> drivers/regulator/fixed.c | 35 +++++--------------
> include/linux/regulator/fixed.h | 10 ------
> include/linux/regulator/gpio-regulator.h | 6 ----
> 17 files changed, 13 insertions(+), 66 deletions(-)
>
> diff --git a/arch/arm/mach-imx/mach-mx21ads.c b/arch/arm/mach-imx/mach-mx21ads.c
> index 2e1e540f2e5a..d278fb672d40 100644
> --- a/arch/arm/mach-imx/mach-mx21ads.c
> +++ b/arch/arm/mach-imx/mach-mx21ads.c
> @@ -205,7 +205,6 @@ static struct regulator_init_data mx21ads_lcd_regulator_init_data = {
> static struct fixed_voltage_config mx21ads_lcd_regulator_pdata = {
> .supply_name = "LCD",
> .microvolts = 3300000,
> - .enable_high = 1,
> .init_data = &mx21ads_lcd_regulator_init_data,
> };
>
> diff --git a/arch/arm/mach-imx/mach-mx27ads.c b/arch/arm/mach-imx/mach-mx27ads.c
> index f5e04047ed13..6dd7f57c332f 100644
> --- a/arch/arm/mach-imx/mach-mx27ads.c
> +++ b/arch/arm/mach-imx/mach-mx27ads.c
> @@ -237,7 +237,7 @@ static struct fixed_voltage_config mx27ads_lcd_regulator_pdata = {
> static struct gpiod_lookup_table mx27ads_lcd_regulator_gpiod_table = {
> .dev_id = "reg-fixed-voltage.0", /* Let's hope ID 0 is what we get */
> .table = {
> - GPIO_LOOKUP("LCD", 0, NULL, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP("LCD", 0, NULL, GPIO_ACTIVE_LOW),
> { },
> },
> };
> diff --git a/arch/arm/mach-mmp/brownstone.c b/arch/arm/mach-mmp/brownstone.c
> index a04e249c654b..d2560fb1e835 100644
> --- a/arch/arm/mach-mmp/brownstone.c
> +++ b/arch/arm/mach-mmp/brownstone.c
> @@ -149,7 +149,6 @@ static struct regulator_init_data brownstone_v_5vp_data = {
> static struct fixed_voltage_config brownstone_v_5vp = {
> .supply_name = "v_5vp",
> .microvolts = 5000000,
> - .enable_high = 1,
> .enabled_at_boot = 1,
> .init_data = &brownstone_v_5vp_data,
> };
> diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
> index c4c0a8ea11e4..be30c3c061b4 100644
> --- a/arch/arm/mach-omap1/board-ams-delta.c
> +++ b/arch/arm/mach-omap1/board-ams-delta.c
> @@ -267,7 +267,6 @@ static struct fixed_voltage_config modem_nreset_config = {
> .supply_name = "modem_nreset",
> .microvolts = 3300000,
> .startup_delay = 25000,
> - .enable_high = 1,
> .enabled_at_boot = 1,
> .init_data = &modem_nreset_data,
> };
> @@ -533,7 +532,6 @@ static struct regulator_init_data keybrd_pwr_initdata = {
> static struct fixed_voltage_config keybrd_pwr_config = {
> .supply_name = "keybrd_pwr",
> .microvolts = 5000000,
> - .enable_high = 1,
> .init_data = &keybrd_pwr_initdata,
> };
>
> diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
> index 8a5b6ed4ec36..a2ecc5e69abb 100644
> --- a/arch/arm/mach-omap2/pdata-quirks.c
> +++ b/arch/arm/mach-omap2/pdata-quirks.c
> @@ -330,7 +330,6 @@ static struct fixed_voltage_config pandora_vwlan = {
> .supply_name = "vwlan",
> .microvolts = 1800000, /* 1.8V */
> .startup_delay = 50000, /* 50ms */
> - .enable_high = 1,
> .init_data = &pandora_vmmc3,
> };
>
> diff --git a/arch/arm/mach-pxa/em-x270.c b/arch/arm/mach-pxa/em-x270.c
> index 32c1edeb3f14..5ba7bb7f7d51 100644
> --- a/arch/arm/mach-pxa/em-x270.c
> +++ b/arch/arm/mach-pxa/em-x270.c
> @@ -976,7 +976,6 @@ static struct fixed_voltage_config camera_dummy_config = {
> .supply_name = "camera_vdd",
> .input_supply = "vcc cam",
> .microvolts = 2800000,
> - .enable_high = 0,
> .init_data = &camera_dummy_initdata,
> };
>
> diff --git a/arch/arm/mach-pxa/ezx.c b/arch/arm/mach-pxa/ezx.c
> index 565965e9acc7..5e110e70ce5a 100644
> --- a/arch/arm/mach-pxa/ezx.c
> +++ b/arch/arm/mach-pxa/ezx.c
> @@ -714,7 +714,6 @@ static struct regulator_init_data camera_regulator_initdata = {
> static struct fixed_voltage_config camera_regulator_config = {
> .supply_name = "camera_vdd",
> .microvolts = 2800000,
> - .enable_high = 0,
> .init_data = &camera_regulator_initdata,
> };
>
> @@ -730,7 +729,7 @@ static struct gpiod_lookup_table camera_supply_gpiod_table = {
> .dev_id = "reg-fixed-voltage.1",
> .table = {
> GPIO_LOOKUP("gpio-pxa", GPIO50_nCAM_EN,
> - NULL, GPIO_ACTIVE_HIGH),
> + NULL, GPIO_ACTIVE_LOW),
> { },
> },
> };
> diff --git a/arch/arm/mach-pxa/raumfeld.c b/arch/arm/mach-pxa/raumfeld.c
> index e1db072756f2..e13bfc9b01d2 100644
> --- a/arch/arm/mach-pxa/raumfeld.c
> +++ b/arch/arm/mach-pxa/raumfeld.c
> @@ -883,7 +883,6 @@ static struct regulator_init_data audio_va_initdata = {
> static struct fixed_voltage_config audio_va_config = {
> .supply_name = "audio_va",
> .microvolts = 5000000,
> - .enable_high = 1,
> .enabled_at_boot = 0,
> .init_data = &audio_va_initdata,
> };
> diff --git a/arch/arm/mach-pxa/zeus.c b/arch/arm/mach-pxa/zeus.c
> index c411f79d4cb5..ebd654302387 100644
> --- a/arch/arm/mach-pxa/zeus.c
> +++ b/arch/arm/mach-pxa/zeus.c
> @@ -426,7 +426,7 @@ static struct gpiod_lookup_table can_regulator_gpiod_table = {
> .dev_id = "reg-fixed-voltage.0",
> .table = {
> GPIO_LOOKUP("gpio-pxa", ZEUS_CAN_SHDN_GPIO,
> - NULL, GPIO_ACTIVE_HIGH),
> + NULL, GPIO_ACTIVE_LOW),
> { },
> },
> };
> @@ -547,7 +547,6 @@ static struct regulator_init_data zeus_ohci_regulator_data = {
> static struct fixed_voltage_config zeus_ohci_regulator_config = {
> .supply_name = "vbus2",
> .microvolts = 5000000, /* 5.0V */
> - .enable_high = 1,
> .startup_delay = 0,
> .init_data = &zeus_ohci_regulator_data,
> };
> diff --git a/arch/arm/mach-sa1100/assabet.c b/arch/arm/mach-sa1100/assabet.c
> index dfa42496ec27..d09c3f236186 100644
> --- a/arch/arm/mach-sa1100/assabet.c
> +++ b/arch/arm/mach-sa1100/assabet.c
> @@ -469,7 +469,6 @@ static struct regulator_consumer_supply assabet_cf_vcc_consumers[] = {
> static struct fixed_voltage_config assabet_cf_vcc_pdata __initdata = {
> .supply_name = "cf-power",
> .microvolts = 3300000,
> - .enable_high = 1,
> };
>
> static struct gpiod_lookup_table assabet_cf_vcc_gpio_table = {
> diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
> index 22b4106b8084..5495efa07335 100644
> --- a/arch/sh/boards/mach-ecovec24/setup.c
> +++ b/arch/sh/boards/mach-ecovec24/setup.c
> @@ -630,7 +630,6 @@ static struct regulator_init_data cn12_power_init_data = {
> static struct fixed_voltage_config cn12_power_info = {
> .supply_name = "CN12 SD/MMC Vdd",
> .microvolts = 3300000,
> - .enable_high = 1,
> .init_data = &cn12_power_init_data,
> };
>
> @@ -671,7 +670,6 @@ static struct regulator_init_data sdhi0_power_init_data = {
> static struct fixed_voltage_config sdhi0_power_info = {
> .supply_name = "CN11 SD/MMC Vdd",
> .microvolts = 3300000,
> - .enable_high = 1,
> .init_data = &sdhi0_power_init_data,
> };
>
> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c b/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
> index 96f438d4b026..1421d5330b2c 100644
> --- a/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c
> @@ -44,7 +44,6 @@ static struct fixed_voltage_config bcm43xx_vmmc = {
> */
> .microvolts = 2000000, /* 1.8V */
> .startup_delay = 250 * 1000, /* 250ms */
> - .enable_high = 1, /* active high */
> .enabled_at_boot = 0, /* disabled at boot */
> .init_data = &bcm43xx_vmmc_data,
> };
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index b9d7b45c7295..48baa03ff3d8 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -82,7 +82,6 @@ struct regulator_enable_gpio {
> struct gpio_desc *gpiod;
> u32 enable_count; /* a number of enabled shared GPIO */
> u32 request_count; /* a number of requested shared GPIO */
> - unsigned int ena_gpio_invert:1;
> };
>
> /*
> @@ -2268,7 +2267,6 @@ static int regulator_ena_gpio_request(struct regulator_dev *rdev,
> }
>
> pin->gpiod = gpiod;
> - pin->ena_gpio_invert = config->ena_gpio_invert;
> list_add(&pin->list, &regulator_ena_gpio_list);
>
> update_ena_gpio_to_rdev:
> @@ -2319,8 +2317,7 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
> if (enable) {
> /* Enable GPIO at initial use */
> if (pin->enable_count == 0)
> - gpiod_set_value_cansleep(pin->gpiod,
> - !pin->ena_gpio_invert);
> + gpiod_set_value_cansleep(pin->gpiod, 1);
>
> pin->enable_count++;
> } else {
> @@ -2331,8 +2328,7 @@ static int regulator_ena_gpio_ctrl(struct regulator_dev *rdev, bool enable)
>
> /* Disable GPIO if not used */
> if (pin->enable_count <= 1) {
> - gpiod_set_value_cansleep(pin->gpiod,
> - pin->ena_gpio_invert);
> + gpiod_set_value_cansleep(pin->gpiod, 0);
> pin->enable_count = 0;
> }
> }
> diff --git a/drivers/regulator/da9055-regulator.c b/drivers/regulator/da9055-regulator.c
> index 588c3d2445cf..417cafe2aba0 100644
> --- a/drivers/regulator/da9055-regulator.c
> +++ b/drivers/regulator/da9055-regulator.c
> @@ -457,7 +457,6 @@ static int da9055_gpio_init(struct da9055_regulator *regulator,
> int gpio_mux = pdata->gpio_ren[id];
>
> config->ena_gpiod = pdata->ena_gpiods[id];
> - config->ena_gpio_invert = 1;
>
> /*
> * GPI pin is muxed with regulator to control the
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 9abdb9130766..b5afc9db2c61 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -79,15 +79,6 @@ of_get_fixed_voltage_config(struct device *dev,
>
> of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
>
> - /*
> - * FIXME: we pulled active low/high and open drain handling into
> - * gpiolib so it will be handled there. Delete this in the second
> - * step when we also remove the custom inversion handling for all
> - * legacy boardfiles.
> - */
> - config->enable_high = 1;
> - config->gpio_is_open_drain = 0;
> -
> if (of_find_property(np, "vin-supply", NULL))
> config->input_supply = "vin";
>
> @@ -151,24 +142,14 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
>
> drvdata->desc.fixed_uV = config->microvolts;
>
> - cfg.ena_gpio_invert = !config->enable_high;
> - if (config->enabled_at_boot) {
> - if (config->enable_high)
> - gflags = GPIOD_OUT_HIGH;
> - else
> - gflags = GPIOD_OUT_LOW;
> - } else {
> - if (config->enable_high)
> - gflags = GPIOD_OUT_LOW;
> - else
> - gflags = GPIOD_OUT_HIGH;
> - }
> - if (config->gpio_is_open_drain) {
> - if (gflags == GPIOD_OUT_HIGH)
> - gflags = GPIOD_OUT_HIGH_OPEN_DRAIN;
> - else
> - gflags = GPIOD_OUT_LOW_OPEN_DRAIN;
> - }
> + /*
> + * The signal will be inverted by the GPIO core if flagged so in the
> + * decriptor.
> + */
> + if (config->enabled_at_boot)
> + gflags = GPIOD_OUT_HIGH;
> + else
> + gflags = GPIOD_OUT_LOW;
>
> /*
> * Some fixed regulators share the enable line between two
> diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
> index 1a4340ed8e2b..f10140da7145 100644
> --- a/include/linux/regulator/fixed.h
> +++ b/include/linux/regulator/fixed.h
> @@ -25,14 +25,6 @@ struct regulator_init_data;
> * @input_supply: Name of the input regulator supply
> * @microvolts: Output voltage of regulator
> * @startup_delay: Start-up time in microseconds
> - * @gpio_is_open_drain: Gpio pin is open drain or normal type.
> - * If it is open drain type then HIGH will be set
> - * through PULL-UP with setting gpio as input
> - * and low will be set as gpio-output with driven
> - * to low. For non-open-drain case, the gpio will
> - * will be in output and drive to low/high accordingly.
> - * @enable_high: Polarity of enable GPIO
> - * 1 = Active high, 0 = Active low
> * @enabled_at_boot: Whether regulator has been enabled at
> * boot or not. 1 = Yes, 0 = No
> * This is used to keep the regulator at
> @@ -48,8 +40,6 @@ struct fixed_voltage_config {
> const char *input_supply;
> int microvolts;
> unsigned startup_delay;
> - unsigned gpio_is_open_drain:1;
> - unsigned enable_high:1;
> unsigned enabled_at_boot:1;
> struct regulator_init_data *init_data;
> };
> diff --git a/include/linux/regulator/gpio-regulator.h b/include/linux/regulator/gpio-regulator.h
> index 49c407afb944..11cd6375215d 100644
> --- a/include/linux/regulator/gpio-regulator.h
> +++ b/include/linux/regulator/gpio-regulator.h
> @@ -46,10 +46,6 @@ struct gpio_regulator_state {
> /**
> * struct gpio_regulator_config - config structure
> * @supply_name: Name of the regulator supply
> - * @enable_gpio: GPIO to use for enable control
> - * set to -EINVAL if not used
> - * @enable_high: Polarity of enable GPIO
> - * 1 = Active high, 0 = Active low
> * @enabled_at_boot: Whether regulator has been enabled at
> * boot or not. 1 = Yes, 0 = No
> * This is used to keep the regulator at
> @@ -71,8 +67,6 @@ struct gpio_regulator_state {
> struct gpio_regulator_config {
> const char *supply_name;
>
> - int enable_gpio;
> - unsigned enable_high:1;
> unsigned enabled_at_boot:1;
> unsigned startup_delay;
>

Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Next message: Thomas Hellstrom: "Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs"
Previous message: Sibi Sankar: "Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]