linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0
@ 2014-10-08 13:47 Markus Pargmann
  2014-10-08 13:47 ` [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config Markus Pargmann
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-10-08 13:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel, Markus Pargmann

Hi,

In v2 of this series I renamed the added regulator_config field
'ena_gpio_valid' to 'ena_gpio_initialized'. We have a gpio_is_valid check in
the regulator core code, so no need to require a valid ena_gpio. It should
avoid a lot of gpio_is_valid checks in regulator drivers.

v2 is also backward compatible now, so if ena_gpio_initialized is false, it
will still handle gpios > 0. There are some more changes on the regulator
driver patches, which now do not set ena_gpio_initialized for pdata code.

This series fixes the handling of gpio 0 in the regulator framework.

Best regards,

Markus


Markus Pargmann (4):
  regulator: Add ena_gpio_initialized to regulator_config
  regulator: Set ena_gpio_initialized in regulator drivers
  regulator: fixed: Use gpio_is_valid
  regulator: gpio: Use gpio_is_valid

 drivers/regulator/arizona-ldo1.c       | 1 +
 drivers/regulator/core.c               | 3 ++-
 drivers/regulator/fixed.c              | 4 +++-
 drivers/regulator/gpio-regulator.c     | 4 +++-
 drivers/regulator/max8952.c            | 2 ++
 drivers/regulator/s2mps11.c            | 1 +
 drivers/regulator/s5m8767.c            | 1 +
 drivers/regulator/tps65090-regulator.c | 1 +
 drivers/regulator/wm8994-regulator.c   | 6 ++++--
 include/linux/regulator/driver.h       | 4 ++++
 10 files changed, 22 insertions(+), 5 deletions(-)

-- 
2.1.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config
  2014-10-08 13:47 [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0 Markus Pargmann
@ 2014-10-08 13:47 ` Markus Pargmann
  2014-10-13 14:49   ` Mark Brown
  2014-10-08 13:47 ` [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers Markus Pargmann
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Markus Pargmann @ 2014-10-08 13:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel, 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 based on a 0 in ena_gpio.

To be able to decide if it is a valid gpio that got passed, this patch
adds a ena_gpio_initialized field that should be set if was initialized
with a correct value, either a gpio >= 0 or a negative error number. The
core then checks if ena_gpio or ena_gpio_initialized before handling it
as a gpio. This way we maintain backwards compatibility and fix the
behaviour for gpio number 0.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---

Notes:
    Changes in v2:
     - Squash all regulator core changes into one patch
     - Fix the ena_gpio check to be backwards compatible

 drivers/regulator/core.c         | 3 ++-
 include/linux/regulator/driver.h | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd87c0c37034..55a87a2722d8 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3650,7 +3650,8 @@ 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 || config->ena_gpio_initialized) &&
+	    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",
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index fc0ee0ce8325..28da08e4671f 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -301,6 +301,9 @@ struct regulator_desc {
  *           NULL).
  * @regmap: regmap to use for core regmap helpers if dev_get_regulator() is
  *          insufficient.
+ * @ena_gpio_initialized: GPIO controlling regulator enable was properly
+ *                        initialized, meaning that >= 0 is a valid gpio
+ *                        identifier and < 0 is a non existent gpio.
  * @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()
@@ -312,6 +315,7 @@ struct regulator_config {
 	struct device_node *of_node;
 	struct regmap *regmap;
 
+	bool ena_gpio_initialized;
 	int ena_gpio;
 	unsigned int ena_gpio_invert:1;
 	unsigned int ena_gpio_flags;
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers
  2014-10-08 13:47 [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0 Markus Pargmann
  2014-10-08 13:47 ` [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config Markus Pargmann
@ 2014-10-08 13:47 ` Markus Pargmann
  2014-10-08 14:53   ` Krzysztof Kozlowski
  2014-10-13 14:42   ` Mark Brown
  2014-10-08 13:47 ` [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid Markus Pargmann
  2014-10-08 13:47 ` [PATCH v2 4/4] regulator: gpio: " Markus Pargmann
  3 siblings, 2 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-10-08 13:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel, Markus Pargmann

This patch sets ena_gpio_initialized for all drivers which set a
ena_gpio from parsed DT properties. Drivers using pdata may get zero
initialized pdata and therefore copy a 0 into the regulator_config
ena_gpio field.

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

diff --git a/drivers/regulator/arizona-ldo1.c b/drivers/regulator/arizona-ldo1.c
index 4c9db589f6c1..f4ecf18222d3 100644
--- a/drivers/regulator/arizona-ldo1.c
+++ b/drivers/regulator/arizona-ldo1.c
@@ -260,6 +260,7 @@ static int arizona_ldo1_probe(struct platform_device *pdev)
 			ret = arizona_ldo1_of_get_pdata(arizona, &config);
 			if (ret < 0)
 				return ret;
+			config.ena_gpio_initialized = true;
 		}
 	}
 
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 354105eff1f8..441a3e90e266 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_initialized = 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..5da0125fac6a 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_initialized = true;
+	}
 	cfg.ena_gpio_invert = !config->enable_high;
 	if (config->enabled_at_boot) {
 		if (config->enable_high)
diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
index f7f9efcfedb7..6d1802b596ec 100644
--- a/drivers/regulator/max8952.c
+++ b/drivers/regulator/max8952.c
@@ -225,6 +225,8 @@ static int max8952_pmic_probe(struct i2c_client *client,
 	config.of_node = client->dev.of_node;
 
 	config.ena_gpio = pdata->gpio_en;
+	if (!pdata)
+		config.ena_gpio_initialized = 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 adab82d5279f..49b9e1ddc87e 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -897,6 +897,7 @@ common_reg:
 			config.of_node = rdata[i].of_node;
 		}
 		config.ena_gpio = s2mps11->ext_control_gpio[i];
+		config.ena_gpio_initialized = true;
 
 		regulator = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 0ab5cbeeb797..7f176cdb7e37 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_initialized = 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..2e92aa8718cc 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_initialized = true;
 		config->ena_gpio_flags = gpio_flag;
 	}
 }
diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/wm8994-regulator.c
index c24346db8a71..88f5064e412b 100644
--- a/drivers/regulator/wm8994-regulator.c
+++ b/drivers/regulator/wm8994-regulator.c
@@ -145,10 +145,12 @@ static int wm8994_ldo_probe(struct platform_device *pdev)
 	config.driver_data = ldo;
 	config.regmap = wm8994->regmap;
 	config.init_data = &ldo->init_data;
-	if (pdata)
+	if (pdata) {
 		config.ena_gpio = pdata->ldo[id].enable;
-	else if (wm8994->dev->of_node)
+	} else if (wm8994->dev->of_node) {
 		config.ena_gpio = wm8994->pdata.ldo[id].enable;
+		config.ena_gpio_initialized = true;
+	}
 
 	/* Use default constraints if none set up */
 	if (!pdata || !pdata->ldo[id].init_data || wm8994->dev->of_node) {
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid
  2014-10-08 13:47 [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0 Markus Pargmann
  2014-10-08 13:47 ` [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config Markus Pargmann
  2014-10-08 13:47 ` [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers Markus Pargmann
@ 2014-10-08 13:47 ` Markus Pargmann
  2014-10-13 14:47   ` Mark Brown
  2014-10-08 13:47 ` [PATCH v2 4/4] regulator: gpio: " Markus Pargmann
  3 siblings, 1 reply; 12+ messages in thread
From: Markus Pargmann @ 2014-10-08 13:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel, Markus Pargmann

Use gpio_is_valid instead of an explicit comparison with 0.

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

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 441a3e90e266..696f53cc1927 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_initialized = true;
 	}
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v2 4/4] regulator: gpio: Use gpio_is_valid
  2014-10-08 13:47 [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0 Markus Pargmann
                   ` (2 preceding siblings ...)
  2014-10-08 13:47 ` [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid Markus Pargmann
@ 2014-10-08 13:47 ` Markus Pargmann
  3 siblings, 0 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-10-08 13:47 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel, Markus Pargmann

Use gpio_is_valid instead of an explicit comparison with 0.

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

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 5da0125fac6a..86546c1c32c4 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_initialized = true;
 	}
-- 
2.1.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers
  2014-10-08 13:47 ` [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers Markus Pargmann
@ 2014-10-08 14:53   ` Krzysztof Kozlowski
  2014-10-10  6:00     ` Markus Pargmann
  2014-10-13 14:42   ` Mark Brown
  1 sibling, 1 reply; 12+ messages in thread
From: Krzysztof Kozlowski @ 2014-10-08 14:53 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Mark Brown, Liam Girdwood, linux-arm-kernel, linux-kernel, kernel

On śro, 2014-10-08 at 15:47 +0200, Markus Pargmann wrote:
> This patch sets ena_gpio_initialized for all drivers which set a
> ena_gpio from parsed DT properties. Drivers using pdata may get zero
> initialized pdata and therefore copy a 0 into the regulator_config
> ena_gpio field.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

(... rewind to s2m/s5m... I think Mark asked for splitting this per
driver)

> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> index adab82d5279f..49b9e1ddc87e 100644
> --- a/drivers/regulator/s2mps11.c
> +++ b/drivers/regulator/s2mps11.c
> @@ -897,6 +897,7 @@ common_reg:
>  			config.of_node = rdata[i].of_node;
>  		}
>  		config.ena_gpio = s2mps11->ext_control_gpio[i];
> +		config.ena_gpio_initialized = true;
>  
>  		regulator = devm_regulator_register(&pdev->dev,
>  						&regulators[i], &config);

Looks good.

> diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> index 0ab5cbeeb797..7f176cdb7e37 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_initialized = true;
>  	config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
>  }

It will work fine but a little messy. The 'config' is re-used in loop
for next regulators, so:
1. regulator X with GPIO=-ENODEV, ena_gpio_initialized=false
2. regulator X+1 with real GPIO, ena_gpio_initialized=true
3. regulator X+2 with GPIO=-ENODEV, ena_gpio_initialized=true


Instead do this in probe around line 950:
 config.ena_gpio = -EINVAL;
 config.ena_gpio_flags = 0;
+config->ena_gpio_initialized = true;
 if (gpio_is_valid(pdata->regulators[i].ext_control_gpio))
 	s5m8767_regulator_config_ext_control(s5m8767,
 			&pdata->regulators[i], &config);


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers
  2014-10-08 14:53   ` Krzysztof Kozlowski
@ 2014-10-10  6:00     ` Markus Pargmann
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-10-10  6:00 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Mark Brown, Liam Girdwood, linux-arm-kernel, linux-kernel, kernel

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

On Wed, Oct 08, 2014 at 04:53:33PM +0200, Krzysztof Kozlowski wrote:
> On śro, 2014-10-08 at 15:47 +0200, Markus Pargmann wrote:
> > This patch sets ena_gpio_initialized for all drivers which set a
> > ena_gpio from parsed DT properties. Drivers using pdata may get zero
> > initialized pdata and therefore copy a 0 into the regulator_config
> > ena_gpio field.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> (... rewind to s2m/s5m... I think Mark asked for splitting this per
> driver)

Then I misunderstood Mark, I thought he just meant the fixups at the end
of the last series.

> 
> > diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> > index adab82d5279f..49b9e1ddc87e 100644
> > --- a/drivers/regulator/s2mps11.c
> > +++ b/drivers/regulator/s2mps11.c
> > @@ -897,6 +897,7 @@ common_reg:
> >  			config.of_node = rdata[i].of_node;
> >  		}
> >  		config.ena_gpio = s2mps11->ext_control_gpio[i];
> > +		config.ena_gpio_initialized = true;
> >  
> >  		regulator = devm_regulator_register(&pdev->dev,
> >  						&regulators[i], &config);
> 
> Looks good.
> 
> > diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
> > index 0ab5cbeeb797..7f176cdb7e37 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_initialized = true;
> >  	config->ena_gpio_flags = GPIOF_OUT_INIT_HIGH;
> >  }
> 
> It will work fine but a little messy. The 'config' is re-used in loop
> for next regulators, so:
> 1. regulator X with GPIO=-ENODEV, ena_gpio_initialized=false
> 2. regulator X+1 with real GPIO, ena_gpio_initialized=true
> 3. regulator X+2 with GPIO=-ENODEV, ena_gpio_initialized=true
> 
> 
> Instead do this in probe around line 950:
>  config.ena_gpio = -EINVAL;
>  config.ena_gpio_flags = 0;
> +config->ena_gpio_initialized = true;
>  if (gpio_is_valid(pdata->regulators[i].ext_control_gpio))
>  	s5m8767_regulator_config_ext_control(s5m8767,
>  			&pdata->regulators[i], &config);

I will fix that.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers
  2014-10-08 13:47 ` [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers Markus Pargmann
  2014-10-08 14:53   ` Krzysztof Kozlowski
@ 2014-10-13 14:42   ` Mark Brown
  2014-11-03 13:22     ` Markus Pargmann
  1 sibling, 1 reply; 12+ messages in thread
From: Mark Brown @ 2014-10-13 14:42 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel

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

On Wed, Oct 08, 2014 at 03:47:06PM +0200, Markus Pargmann wrote:

> --- a/drivers/regulator/max8952.c
> +++ b/drivers/regulator/max8952.c
> @@ -225,6 +225,8 @@ static int max8952_pmic_probe(struct i2c_client *client,
>  	config.of_node = client->dev.of_node;
>  
>  	config.ena_gpio = pdata->gpio_en;
> +	if (!pdata)
> +		config.ena_gpio_initialized = true;

This looks wrong, we're checking for pdata being non-NULL immediately
after dereferencing pdata.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid
  2014-10-08 13:47 ` [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid Markus Pargmann
@ 2014-10-13 14:47   ` Mark Brown
  2014-11-03 13:38     ` Markus Pargmann
  0 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2014-10-13 14:47 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel

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

On Wed, Oct 08, 2014 at 03:47:07PM +0200, Markus Pargmann wrote:
> Use gpio_is_valid instead of an explicit comparison with 0.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/regulator/fixed.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 441a3e90e266..696f53cc1927 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)) {

Have you audited all users to ensure that they don't rely on zero being
ignored?  Right now we're sharing the core behaviour here so the same
issues apply to this platform data as apply to the core.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config
  2014-10-08 13:47 ` [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config Markus Pargmann
@ 2014-10-13 14:49   ` Mark Brown
  0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2014-10-13 14:49 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel

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

On Wed, Oct 08, 2014 at 03:47:05PM +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 based on a 0 in ena_gpio.

Applied, thanks.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers
  2014-10-13 14:42   ` Mark Brown
@ 2014-11-03 13:22     ` Markus Pargmann
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-11-03 13:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel

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

Hi,

Sorry for the delay, holiday and so on.

On Mon, Oct 13, 2014 at 04:42:35PM +0200, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:47:06PM +0200, Markus Pargmann wrote:
> 
> > --- a/drivers/regulator/max8952.c
> > +++ b/drivers/regulator/max8952.c
> > @@ -225,6 +225,8 @@ static int max8952_pmic_probe(struct i2c_client *client,
> >  	config.of_node = client->dev.of_node;
> >  
> >  	config.ena_gpio = pdata->gpio_en;
> > +	if (!pdata)
> > +		config.ena_gpio_initialized = true;
> 
> This looks wrong, we're checking for pdata being non-NULL immediately
> after dereferencing pdata.

Yes this is wrong, I changed it to check for client->dev.of_node instead.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid
  2014-10-13 14:47   ` Mark Brown
@ 2014-11-03 13:38     ` Markus Pargmann
  0 siblings, 0 replies; 12+ messages in thread
From: Markus Pargmann @ 2014-11-03 13:38 UTC (permalink / raw)
  To: Mark Brown
  Cc: Liam Girdwood, Krzysztof Kozłowski, linux-arm-kernel,
	linux-kernel, kernel

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

On Mon, Oct 13, 2014 at 04:47:33PM +0200, Mark Brown wrote:
> On Wed, Oct 08, 2014 at 03:47:07PM +0200, Markus Pargmann wrote:
> > Use gpio_is_valid instead of an explicit comparison with 0.
> > 
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > ---
> >  drivers/regulator/fixed.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> > index 441a3e90e266..696f53cc1927 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)) {
> 
> Have you audited all users to ensure that they don't rely on zero being
> ignored?  Right now we're sharing the core behaviour here so the same
> issues apply to this platform data as apply to the core.

For all other drivers this should be fine (just rechecked again). But
the fixed regulator driver patch has this problem.

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2014-11-03 13:38 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-08 13:47 [PATCH v2 0/4] regulator: Fix core behaviour for gpio 0 Markus Pargmann
2014-10-08 13:47 ` [PATCH v2 1/4] regulator: Add ena_gpio_initialized to regulator_config Markus Pargmann
2014-10-13 14:49   ` Mark Brown
2014-10-08 13:47 ` [PATCH v2 2/4] regulator: Set ena_gpio_initialized in regulator drivers Markus Pargmann
2014-10-08 14:53   ` Krzysztof Kozlowski
2014-10-10  6:00     ` Markus Pargmann
2014-10-13 14:42   ` Mark Brown
2014-11-03 13:22     ` Markus Pargmann
2014-10-08 13:47 ` [PATCH v2 3/4] regulator: fixed: Use gpio_is_valid Markus Pargmann
2014-10-13 14:47   ` Mark Brown
2014-11-03 13:38     ` Markus Pargmann
2014-10-08 13:47 ` [PATCH v2 4/4] regulator: gpio: " Markus Pargmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).