[PATCH 0/5] regulator: Fix core behaviour for gpio 0

[PATCH 0/5] regulator: Fix core behaviour for gpio 0

[Markus Pargmann]

Hi,

currently it is not possible to have a gpio enabled regulator that uses gpio 0.
0 is a valid gpio identifier so it has to be supported.

This series tries to fix this issue by adding a regulator_config field
'ena_gpio_valid' which should be true if ena_gpio is a valid gpio identifier.
Otherwise ena_gpio is ignored.

Patch 1 introduces 'ena_gpio_valid'. Patch 2 updates all drivers that set
ena_gpio. Patch 3 changes the regulator core check for ena_gpio.

Patch 4 and 5 are minor changes to regulator drivers unrelated to the gpio-0
issue.

Best regards,

Markus


Markus Pargmann (5):
  regulator: Add ena_gpio_valid config
  regulator: Set ena_gpio_valid in regulator drivers
  regulator: Fix ena_gpio check
  regulator: Use gpio_is_valid
  regulator: Remove unnecessary ena_gpio initializations

 drivers/regulator/arizona-ldo1.c       | 1 +
 drivers/regulator/core.c               | 2 +-
 drivers/regulator/da9055-regulator.c   | 1 +
 drivers/regulator/fixed.c              | 4 +++-
 drivers/regulator/gpio-regulator.c     | 4 +++-
 drivers/regulator/lp8788-ldo.c         | 2 ++
 drivers/regulator/max8952.c            | 1 +
 drivers/regulator/pfuze100-regulator.c | 1 -
 drivers/regulator/s2mps11.c            | 1 +
 drivers/regulator/s5m8767.c            | 3 +--
 drivers/regulator/tps65090-regulator.c | 1 +
 drivers/regulator/wm8994-regulator.c   | 3 ++-
 include/linux/regulator/driver.h       | 3 +++
 13 files changed, 20 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH 1/5] regulator: Add ena_gpio_valid config

[Markus Pargmann]

Most drivers do not set the ena_gpio field of struct regulator_config
before passing it to the regulator core. This is fine as long as the
gpio identifier that is passed is a positive integer. But the gpio
identifier 0 is also valid. So we are not able to decide wether we got a
real gpio identifier or not.

To be able to decide if it is a valid gpio that got passed, this patch
adds a ena_gpio_valid field that should be set if ena_gpio is a valid
gpio and should be used. It is a preperation patch for multiple patches
that adapt the drivers and fix the regulator core checks for this field.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 include/linux/regulator/driver.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index bbe03a1924c0..c561f0faaf61 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -292,6 +292,8 @@ struct regulator_desc {
  *           NULL).
  * @regmap: regmap to use for core regmap helpers if dev_get_regulator() is
  *          insufficient.
+ * @ena_gpio_valid: GPIO controlling regulator enable is valid and should be
+ *                  used.
  * @ena_gpio: GPIO controlling regulator enable.
  * @ena_gpio_invert: Sense for GPIO enable control.
  * @ena_gpio_flags: Flags to use when calling gpio_request_one()
@@ -303,6 +305,7 @@ struct regulator_config {
 	struct device_node *of_node;
 	struct regmap *regmap;
 
+	bool ena_gpio_valid;
 	int ena_gpio;
 	unsigned int ena_gpio_invert:1;
 	unsigned int ena_gpio_flags;
-- 
2.1.0

[PATCH 2/5] regulator: Set ena_gpio_valid in regulator drivers

[Markus Pargmann]

This patch sets the new field ena_gpio_valid for all drivers which set a
valid ena_gpio.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/arizona-ldo1.c       | 1 +
 drivers/regulator/da9055-regulator.c   | 1 +
 drivers/regulator/fixed.c              | 4 +++-
 drivers/regulator/gpio-regulator.c     | 4 +++-
 drivers/regulator/lp8788-ldo.c         | 2 ++
 drivers/regulator/max8952.c            | 1 +
 drivers/regulator/s2mps11.c            | 1 +
 drivers/regulator/s5m8767.c            | 1 +
 drivers/regulator/tps65090-regulator.c | 1 +
 drivers/regulator/wm8994-regulator.c   | 1 +
 10 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 4c9db589f6c1..48c33be3f297 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -264,6 +264,7 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 	}
 
 	config.ena_gpio = arizona->pdata.ldoena;
+	config.ena_gpio_valid = true;
 
 	if (arizona->pdata.ldo1)
 		config.init_data = arizona->pdata.ldo1;
diff --git a/drivers/regulator/da9055-regulator.c b/drivers/regulator/da9055-regulator.c
index 9516317e1a9f..f5ccd27bb672 100644
--- a/drivers/regulator/da9055-regulator.c
+++ b/drivers/regulator/da9055-regulator.c
@@ -456,6 +456,7 @@ static int da9055_gpio_init(struct da9055_regulator *regulator,
 		int gpio_mux = pdata->gpio_ren[id];
 
 		config->ena_gpio = pdata->ena_gpio[id];
+		config->ena_gpio_valid = true;
 		config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
 		config->ena_gpio_invert = 1;
 
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 354105eff1f8..731f4315292c 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -157,8 +157,10 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	drvdata->desc.fixed_uV = config->microvolts;
 
-	if (config->gpio >= 0)
+	if (config->gpio >= 0) {
 		cfg.ena_gpio = config->gpio;
+		cfg.ena_gpio_valid = true;
+	}
 	cfg.ena_gpio_invert = !config->enable_high;
 	if (config->enabled_at_boot) {
 		if (config->enable_high)
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 989b23b377c0..681c604d4917 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -322,8 +322,10 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	cfg.driver_data = drvdata;
 	cfg.of_node = np;
 
-	if (config->enable_gpio >= 0)
+	if (config->enable_gpio >= 0) {
 		cfg.ena_gpio = config->enable_gpio;
+		cfg.ena_gpio_valid = true;
+	}
 	cfg.ena_gpio_invert = !config->enable_high;
 	if (config->enabled_at_boot) {
 		if (config->enable_high)
diff --git a/drivers/regulator/lp8788-ldo.c b/drivers/regulator/lp8788-ldo.c
index b9a29a29933f..3e9d027f66fb 100644
--- a/drivers/regulator/lp8788-ldo.c
+++ b/drivers/regulator/lp8788-ldo.c
@@ -535,6 +535,7 @@ static int lp8788_dldo_probe(struct platform_device *pdev)
 
 	if (ldo->en_pin) {
 		cfg.ena_gpio = ldo->en_pin->gpio;
+		cfg.ena_gpio_valid = true;
 		cfg.ena_gpio_flags = ldo->en_pin->init_state;
 	}
 
@@ -585,6 +586,7 @@ static int lp8788_aldo_probe(struct platform_device *pdev)
 
 	if (ldo->en_pin) {
 		cfg.ena_gpio = ldo->en_pin->gpio;
+		cfg.ena_gpio_valid = true;
 		cfg.ena_gpio_flags = ldo->en_pin->init_state;
 	}
 
diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
index f7f9efcfedb7..b56eec51fa10 100644
--- a/drivers/regulator/max8952.c
+++ b/drivers/regulator/max8952.c
@@ -225,6 +225,7 @@ static int max8952_pmic_probe(struct i2c_client *client,
 	config.of_node = client->dev.of_node;
 
 	config.ena_gpio = pdata->gpio_en;
+	config.ena_gpio_valid = true;
 	if (pdata->reg_data->constraints.boot_on)
 		config.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
 
diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index b16c53a8272f..4d78477b9f57 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -986,6 +986,7 @@ common_reg:
 			config.of_node = rdata[i].of_node;
 		}
 		config.ena_gpio = s2mps11->ext_control_gpio[i];
+		config.ena_gpio_valid = true;
 
 		regulator = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 0ab5cbeeb797..d258e6613831 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -466,6 +466,7 @@ static void s5m8767_regulator_config_ext_control(struct s5m8767_info *s5m8767,
 	}
 
 	config->ena_gpio = rdata->ext_control_gpio;
+	config->ena_gpio_valid = true;
 	config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
 }
 
diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index d5df1e9ad1da..ba87da6e494b 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -312,6 +312,7 @@ static void tps65090_configure_regulator_config(
 			gpio_flag = GPIOF_OUT_INIT_HIGH;
 
 		config->ena_gpio = tps_pdata->gpio;
+		config->ena_gpio_valid = true;
 		config->ena_gpio_flags = gpio_flag;
 	}
 }
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index c24346db8a71..dc7e80ab984e 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -149,6 +149,7 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 		config.ena_gpio = pdata->ldo[id].enable;
 	else if (wm8994->dev->of_node)
 		config.ena_gpio = wm8994->pdata.ldo[id].enable;
+	config.ena_gpio_valid = true;
 
 	/* Use default constraints if none set up */
 	if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {
-- 
2.1.0

[PATCH 3/5] regulator: Fix ena_gpio check

[Markus Pargmann]

Use the introduced ena_gpio_valid field to check if ena_gpio stores a
valid gpio.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a3c3785901f5..fd683d26786c 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3560,7 +3560,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 
 	dev_set_drvdata(&rdev->dev, rdev);
 
-	if (config->ena_gpio && gpio_is_valid(config->ena_gpio)) {
+	if (config->ena_gpio_valid && gpio_is_valid(config->ena_gpio)) {
 		ret = regulator_ena_gpio_request(rdev, config);
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
-- 
2.1.0

[PATCH 4/5] regulator: Use gpio_is_valid

[Markus Pargmann]

Use gpio_is_valid instead of custom check.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/fixed.c            | 2 +-
 drivers/regulator/gpio-regulator.c   | 2 +-
 drivers/regulator/wm8994-regulator.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 731f4315292c..35b00a288667 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -157,7 +157,7 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	drvdata->desc.fixed_uV = config->microvolts;
 
-	if (config->gpio >= 0) {
+	if (gpio_is_valid(config->gpio)) {
 		cfg.ena_gpio = config->gpio;
 		cfg.ena_gpio_valid = true;
 	}
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 681c604d4917..52d50aa9ab4b 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -322,7 +322,7 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	cfg.driver_data = drvdata;
 	cfg.of_node = np;
 
-	if (config->enable_gpio >= 0) {
+	if (gpio_is_valid(config->enable_gpio)) {
 		cfg.ena_gpio = config->enable_gpio;
 		cfg.ena_gpio_valid = true;
 	}
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index dc7e80ab984e..8f2df50225a9 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -158,7 +158,7 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 
 		ldo->init_data = wm8994_ldo_default[id];
 		ldo->init_data.consumer_supplies = &ldo->supply;
-		if (!config.ena_gpio)
+		if (!gpio_is_valid(config.ena_gpio))
 			ldo->init_data.constraints.valid_ops_mask = 0;
 	} else {
 		ldo->init_data = *pdata->ldo[id].init_data;
-- 
2.1.0

[PATCH 5/5] regulator: Remove unnecessary ena_gpio initializations

[Markus Pargmann]

It is not necessary to setup config.ena_gpio with -EINVAL or similar.
This patch removes these unnecessary initializations.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/regulator/pfuze100-regulator.c | 1 -
 drivers/regulator/s5m8767.c            | 2 --
 2 files changed, 3 deletions(-)

diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index c879dff597ee..d7455ee309bf 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -509,7 +509,6 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
 		config.init_data = init_data;
 		config.driver_data = pfuze_chip;
 		config.of_node = match_of_node(i);
-		config.ena_gpio = -EINVAL;
 
 		pfuze_chip->regulators[i] =
 			devm_regulator_register(&client->dev, desc, &config);
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index d258e6613831..2c7817f12516 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -949,8 +949,6 @@ static int s5m8767_pmic_probe(struct platform_device *pdev)
 		config.driver_data = s5m8767;
 		config.regmap = iodev->regmap_pmic;
 		config.of_node = pdata->regulators[i].reg_node;
-		config.ena_gpio = -EINVAL;
-		config.ena_gpio_flags = 0;
 		if (gpio_is_valid(pdata->regulators[i].ext_control_gpio))
 			s5m8767_regulator_config_ext_control(s5m8767,
 					&pdata->regulators[i], &config);
-- 
2.1.0

Re: [PATCH 2/5] regulator: Set ena_gpio_valid in regulator drivers

[Krzysztof Kozłowski]

On 06.10.2014 22:17, Markus Pargmann wrote:
> This patch sets the new field ena_gpio_valid for all drivers which set a
> valid ena_gpio.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---

(... looking only on s2m/s5m drivers)

> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> index b16c53a8272f..4d78477b9f57 100644
> --- a/drivers/regulator/s2mps11.c
> +++ b/drivers/regulator/s2mps11.c
> @@ -986,6 +986,7 @@ common_reg:
>   			config.of_node = rdata[i].of_node;
>   		}
>   		config.ena_gpio = s2mps11->ext_control_gpio[i];
> +		config.ena_gpio_valid = true;

This way you'll mark all regulators as GPIO enabled. This is won't 
produce an error (ena_gpio is initialized to -EINVAL by default) but I 
think it is misuse of the idea "ena_gpio_valid".

Instead maybe:
+		if (gpio_is_valid(s2mps11->ext_control_gpio[i]))
+			config.ena_gpio_valid = true;
?

>
>   		regulator = devm_regulator_register(&pdev->dev,
>   						&regulators[i], &config);
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index 0ab5cbeeb797..d258e6613831 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -466,6 +466,7 @@ static void s5m8767_regulator_config_ext_control(struct s5m8767_info *s5m8767,
>   	}
>
>   	config->ena_gpio = rdata->ext_control_gpio;
> +	config->ena_gpio_valid = true;
>   	config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
>   }

This looks fine.

Best regards,
Krzysztof

Re: [PATCH 5/5] regulator: Remove unnecessary ena_gpio initializations

[Krzysztof Kozłowski]

On 06.10.2014 22:17, Markus Pargmann wrote:
> It is not necessary to setup config.ena_gpio with -EINVAL or similar.
> This patch removes these unnecessary initializations.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>   drivers/regulator/pfuze100-regulator.c | 1 -
>   drivers/regulator/s5m8767.c            | 2 --
>   2 files changed, 3 deletions(-)
>
> diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
> index c879dff597ee..d7455ee309bf 100644
> --- a/drivers/regulator/pfuze100-regulator.c
> +++ b/drivers/regulator/pfuze100-regulator.c
> @@ -509,7 +509,6 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
>   		config.init_data = init_data;
>   		config.driver_data = pfuze_chip;
>   		config.of_node = match_of_node(i);
> -		config.ena_gpio = -EINVAL;
>
>   		pfuze_chip->regulators[i] =
>   			devm_regulator_register(&client->dev, desc, &config);
> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index d258e6613831..2c7817f12516 100644
> --- a/drivers/regulator/s5m8767.c
> +++ b/drivers/regulator/s5m8767.c
> @@ -949,8 +949,6 @@ static int s5m8767_pmic_probe(struct platform_device *pdev)
>   		config.driver_data = s5m8767;
>   		config.regmap = iodev->regmap_pmic;
>   		config.of_node = pdata->regulators[i].reg_node;
> -		config.ena_gpio = -EINVAL;
> -		config.ena_gpio_flags = 0;
>   		if (gpio_is_valid(pdata->regulators[i].ext_control_gpio))
>   			s5m8767_regulator_config_ext_control(s5m8767,
>   					&pdata->regulators[i], &config);

This would bring back problem with carried over ena_gpio (commit 
f4fbb3ce342bc1 "regulator: s5m8767: Fix carried over ena_gpio assignment").

The "config" is re-used in loop for next regulators. This means that the 
"config.ena_gpio" has to be set here to prevent using the same value for 
next regulator.

I think these lines shouldn't be removed.

Best regards,
Krzysztof

Re: [PATCH 2/5] regulator: Set ena_gpio_valid in regulator drivers

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

On Tue, Oct 07, 2014 at 10:46:27AM +0200, Krzysztof Kozłowski wrote:
> On 06.10.2014 22:17, Markus Pargmann wrote:
> >This patch sets the new field ena_gpio_valid for all drivers which set a
> >valid ena_gpio.
> >
> >Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> >---
> 
> (... looking only on s2m/s5m drivers)
> 
> >diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> >index b16c53a8272f..4d78477b9f57 100644
> >--- a/drivers/regulator/s2mps11.c
> >+++ b/drivers/regulator/s2mps11.c
> >@@ -986,6 +986,7 @@ common_reg:
> >  			config.of_node = rdata[i].of_node;
> >  		}
> >  		config.ena_gpio = s2mps11->ext_control_gpio[i];
> >+		config.ena_gpio_valid = true;
> 
> This way you'll mark all regulators as GPIO enabled. This is won't
> produce an error (ena_gpio is initialized to -EINVAL by default) but
> I think it is misuse of the idea "ena_gpio_valid".
> 
> Instead maybe:
> +		if (gpio_is_valid(s2mps11->ext_control_gpio[i]))
> +			config.ena_gpio_valid = true;
> ?

The regulator core also checks if the gpio is valid. Perhaps the name
'ena_gpio_valid' is a bit confusing here.
But yes, it may be better to check gpio_is_valid here to see if
there is an actual valid gpio.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/5] regulator: Remove unnecessary ena_gpio initializations

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]

On Tue, Oct 07, 2014 at 10:52:29AM +0200, Krzysztof Kozłowski wrote:
> On 06.10.2014 22:17, Markus Pargmann wrote:
> >It is not necessary to setup config.ena_gpio with -EINVAL or similar.
> >This patch removes these unnecessary initializations.
> >
> >Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> >---
> >  drivers/regulator/pfuze100-regulator.c | 1 -
> >  drivers/regulator/s5m8767.c            | 2 --
> >  2 files changed, 3 deletions(-)
> >
> >diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
> >index c879dff597ee..d7455ee309bf 100644
> >--- a/drivers/regulator/pfuze100-regulator.c
> >+++ b/drivers/regulator/pfuze100-regulator.c
> >@@ -509,7 +509,6 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
> >  		config.init_data = init_data;
> >  		config.driver_data = pfuze_chip;
> >  		config.of_node = match_of_node(i);
> >-		config.ena_gpio = -EINVAL;
> >
> >  		pfuze_chip->regulators[i] =
> >  			devm_regulator_register(&client->dev, desc, &config);
> >diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> >index d258e6613831..2c7817f12516 100644
> >--- a/drivers/regulator/s5m8767.c
> >+++ b/drivers/regulator/s5m8767.c
> >@@ -949,8 +949,6 @@ static int s5m8767_pmic_probe(struct platform_device *pdev)
> >  		config.driver_data = s5m8767;
> >  		config.regmap = iodev->regmap_pmic;
> >  		config.of_node = pdata->regulators[i].reg_node;
> >-		config.ena_gpio = -EINVAL;
> >-		config.ena_gpio_flags = 0;
> >  		if (gpio_is_valid(pdata->regulators[i].ext_control_gpio))
> >  			s5m8767_regulator_config_ext_control(s5m8767,
> >  					&pdata->regulators[i], &config);
> 
> This would bring back problem with carried over ena_gpio (commit
> f4fbb3ce342bc1 "regulator: s5m8767: Fix carried over ena_gpio
> assignment").
> 
> The "config" is re-used in loop for next regulators. This means that
> the "config.ena_gpio" has to be set here to prevent using the same
> value for next regulator.
> 
> I think these lines shouldn't be removed.

Yes right, thanks.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/5] regulator: Add ena_gpio_valid config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

On Mon, Oct 06, 2014 at 10:17:11PM +0200, Markus Pargmann wrote:
> Most drivers do not set the ena_gpio field of struct regulator_config
> before passing it to the regulator core. This is fine as long as the
> gpio identifier that is passed is a positive integer. But the gpio
> identifier 0 is also valid. So we are not able to decide wether we got a
> real gpio identifier or not.
> 
> To be able to decide if it is a valid gpio that got passed, this patch
> adds a ena_gpio_valid field that should be set if ena_gpio is a valid
> gpio and should be used. It is a preperation patch for multiple patches
> that adapt the drivers and fix the regulator core checks for this field.

This should be part of the patch adding meaningful behaviour for the
flag, it's pointless separately.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/5] regulator: Set ena_gpio_valid in regulator drivers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Mon, Oct 06, 2014 at 10:17:12PM +0200, Markus Pargmann wrote:
> This patch sets the new field ena_gpio_valid for all drivers which set a
> valid ena_gpio.

>  	config.ena_gpio = arizona->pdata.ldoena;
> +	config.ena_gpio_valid = true;

This patch just unconditionally sets the flag in all drivers which will
break any system which relies on the existing behaviour that we ignore
GPIO 0.  We can definitely set this flag in any DT only system if we get
the GPIO from DT but otherwise doing this defeats the point of having
the flag in the first place.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/5] regulator: Use gpio_is_valid

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 183 bytes --]

On Mon, Oct 06, 2014 at 10:17:14PM +0200, Markus Pargmann wrote:
> Use gpio_is_valid instead of custom check.

Patches per driver please, this isn't just a mechanical transformation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: Add ena_gpio_valid config

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

Hi,

On Tue, Oct 07, 2014 at 12:53:35PM +0100, Mark Brown wrote:
> On Mon, Oct 06, 2014 at 10:17:11PM +0200, Markus Pargmann wrote:
> > Most drivers do not set the ena_gpio field of struct regulator_config
> > before passing it to the regulator core. This is fine as long as the
> > gpio identifier that is passed is a positive integer. But the gpio
> > identifier 0 is also valid. So we are not able to decide wether we got a
> > real gpio identifier or not.
> > 
> > To be able to decide if it is a valid gpio that got passed, this patch
> > adds a ena_gpio_valid field that should be set if ena_gpio is a valid
> > gpio and should be used. It is a preperation patch for multiple patches
> > that adapt the drivers and fix the regulator core checks for this field.
> 
> This should be part of the patch adding meaningful behaviour for the
> flag, it's pointless separately.

I tried to keep the series bisectable while having different patches for
the drivers and the core. By splitting this 'ena_gpio_valid' field into
a seperate patch, the rest of the drivers will still compile and work
until the core condition was changed to ena_gpio_valid.

But I can squash the three patches into one.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/5] regulator: Set ena_gpio_valid in regulator drivers

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1130 bytes --]

Hi,

On Tue, Oct 07, 2014 at 12:58:17PM +0100, Mark Brown wrote:
> On Mon, Oct 06, 2014 at 10:17:12PM +0200, Markus Pargmann wrote:
> > This patch sets the new field ena_gpio_valid for all drivers which set a
> > valid ena_gpio.
> 
> >  	config.ena_gpio = arizona->pdata.ldoena;
> > +	config.ena_gpio_valid = true;
> 
> This patch just unconditionally sets the flag in all drivers which will
> break any system which relies on the existing behaviour that we ignore
> GPIO 0.  We can definitely set this flag in any DT only system if we get
> the GPIO from DT but otherwise doing this defeats the point of having
> the flag in the first place.

Right, I didn't thought about the old platform data which may pass 0
gpios indicating that there is no gpio.
I will rework this patch.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/5] regulator: Use gpio_is_valid

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Tue, Oct 07, 2014 at 01:01:06PM +0100, Mark Brown wrote:
> On Mon, Oct 06, 2014 at 10:17:14PM +0200, Markus Pargmann wrote:
> > Use gpio_is_valid instead of custom check.
> 
> Patches per driver please, this isn't just a mechanical transformation.

Okay, but you mean just patches 4 and 5 and not patch 2 right?

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/5] regulator: Add ena_gpio_valid config

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On Tue, Oct 07, 2014 at 03:03:20PM +0200, Markus Pargmann wrote:
> On Tue, Oct 07, 2014 at 12:53:35PM +0100, Mark Brown wrote:

> > This should be part of the patch adding meaningful behaviour for the
> > flag, it's pointless separately.

> I tried to keep the series bisectable while having different patches for
> the drivers and the core. By splitting this 'ena_gpio_valid' field into
> a seperate patch, the rest of the drivers will still compile and work
> until the core condition was changed to ena_gpio_valid.

> But I can squash the three patches into one.

No, I think this is missing the point a bit - if we need to introduce
this such that all drivers are instantly buggy without an update that's
probably an indication that we're introducing compatibility problems.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 4/5] regulator: Use gpio_is_valid

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Tue, Oct 07, 2014 at 03:07:37PM +0200, Markus Pargmann wrote:
> On Tue, Oct 07, 2014 at 01:01:06PM +0100, Mark Brown wrote:
> > On Mon, Oct 06, 2014 at 10:17:14PM +0200, Markus Pargmann wrote:
> > > Use gpio_is_valid instead of custom check.

> > Patches per driver please, this isn't just a mechanical transformation.

> Okay, but you mean just patches 4 and 5 and not patch 2 right?

Mostly, that was OK as it was just mechanical.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] regulator: Add ena_gpio_valid config

[Markus Pargmann]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

On Tue, Oct 07, 2014 at 05:19:33PM +0100, Mark Brown wrote:
> On Tue, Oct 07, 2014 at 03:03:20PM +0200, Markus Pargmann wrote:
> > On Tue, Oct 07, 2014 at 12:53:35PM +0100, Mark Brown wrote:
> 
> > > This should be part of the patch adding meaningful behaviour for the
> > > flag, it's pointless separately.
> 
> > I tried to keep the series bisectable while having different patches for
> > the drivers and the core. By splitting this 'ena_gpio_valid' field into
> > a seperate patch, the rest of the drivers will still compile and work
> > until the core condition was changed to ena_gpio_valid.
> 
> > But I can squash the three patches into one.
> 
> No, I think this is missing the point a bit - if we need to introduce
> this such that all drivers are instantly buggy without an update that's
> probably an indication that we're introducing compatibility problems.

Yes, it was designed to not be compatible with the old way of setting up
ena_gpio. But I think it shouldn't be a problem to get it backwads
compatible. I will fix the series and send the next version with one
core patch and another one which adds ena_gpio_valid to the drivers.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]