[PATCH 0/3] regulator: max8973: cleanups and add DT parsing for platform data

[PATCH 0/3] regulator: max8973: cleanups and add DT parsing for platform data

[Laxman Dewangan]

Remove redundant variables for dvs gpio validity, handle unset/default
dvs gpio and add DT parsing for platform data.

Laxman Dewangan (3):
  regulator: max8973: get rid of extra variable for gpio validity
  regulator: max8973: make default/unset dvs gpio as invalid gpio
  regulator: max8973: add DT parsing of platform specific parameter

 .../bindings/regulator/max8973-regulator.txt       | 14 ++++
 drivers/regulator/max8973-regulator.c              | 93 ++++++++++++++++------
 2 files changed, 81 insertions(+), 26 deletions(-)

-- 
1.8.1.5

[PATCH 1/3] regulator: max8973: get rid of extra variable for gpio validity

[Laxman Dewangan]

To find that dvs-gpio is valid or not, gpio API gpio_is_valid()
can be directly used instead of intermediate variable.

Removing the extra variable and using the gpio_is_valid().

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
This is same as earlier patch. Making this as part of this series.

 drivers/regulator/max8973-regulator.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index c3d55c2..1890808 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -100,7 +100,6 @@ struct max8973_chip {
 	int curr_vout_val[MAX8973_MAX_VOUT_REG];
 	int curr_vout_reg;
 	int curr_gpio_val;
-	bool valid_dvs_gpio;
 	struct regulator_ops ops;
 };
 
@@ -174,7 +173,7 @@ static int max8973_dcdc_set_voltage_sel(struct regulator_dev *rdev,
 	 * If gpios are available to select the VOUT register then least
 	 * recently used register for new configuration.
 	 */
-	if (max->valid_dvs_gpio)
+	if (gpio_is_valid(max->dvs_gpio))
 		found = find_voltage_set_register(max, vsel,
 					&vout_reg, &gpio_val);
 
@@ -191,7 +190,7 @@ static int max8973_dcdc_set_voltage_sel(struct regulator_dev *rdev,
 	}
 
 	/* Select proper VOUT register vio gpios */
-	if (max->valid_dvs_gpio) {
+	if (gpio_is_valid(max->dvs_gpio)) {
 		gpio_set_value_cansleep(max->dvs_gpio, gpio_val & 0x1);
 		max->curr_gpio_val = gpio_val;
 	}
@@ -434,7 +433,6 @@ static int max8973_probe(struct i2c_client *client,
 				max->dvs_gpio, ret);
 			return ret;
 		}
-		max->valid_dvs_gpio = true;
 
 		/*
 		 * Initialize the lru index with vout_reg id
@@ -444,8 +442,6 @@ static int max8973_probe(struct i2c_client *client,
 			max->lru_index[i] = i;
 		max->lru_index[0] = max->curr_vout_reg;
 		max->lru_index[max->curr_vout_reg] = 0;
-	} else {
-		max->valid_dvs_gpio = false;
 	}
 
 	if (pdata) {
-- 
1.8.1.5

Re: [PATCH 1/3] regulator: max8973: get rid of extra variable for gpio validity

[Mark Brown]

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

On Mon, Apr 20, 2015 at 06:24:15PM +0530, Laxman Dewangan wrote:
> To find that dvs-gpio is valid or not, gpio API gpio_is_valid()
> can be directly used instead of intermediate variable.

> Removing the extra variable and using the gpio_is_valid().

This still looks like it's got the same issue with not doing the right
thing with platform data that uses zero as a default which is the point
in having a flag like this.

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

Re: [PATCH 1/3] regulator: max8973: get rid of extra variable for gpio validity

[Mark Brown]

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

On Mon, Apr 20, 2015 at 06:24:15PM +0530, Laxman Dewangan wrote:
> To find that dvs-gpio is valid or not, gpio API gpio_is_valid()
> can be directly used instead of intermediate variable.

Applied, thanks.

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

Re: [PATCH 1/3] regulator: max8973: get rid of extra variable for gpio validity

[Laxman Dewangan]

On Tuesday 21 April 2015 02:49 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Apr 20, 2015 at 06:24:15PM +0530, Laxman Dewangan wrote:
>> To find that dvs-gpio is valid or not, gpio API gpio_is_valid()
>> can be directly used instead of intermediate variable.
> Applied, thanks.
>

This patch is not available on linux-next. Is it pending for any clean-up?

Re: [PATCH 1/3] regulator: max8973: get rid of extra variable for gpio validity

[Mark Brown]

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

On Wed, May 06, 2015 at 05:39:28PM +0530, Laxman Dewangan wrote:

> This patch is not available on linux-next. Is it pending for any clean-up?

Ah, sorry - forgot to follow up on this.  The patch doesn't apply
against curent code, please check and resend.

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

[PATCH 2/3] regulator: max8973: make default/unset dvs gpio as invalid gpio

[Laxman Dewangan]

If platform data has dvs-gpio value 0 as default/unset then
make this as invalid gpio so that function gpio_is_valid()
can return false on this case.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
As discussed on the patch
[PATCH] regulator: max8973: get rid of extra variable for gpio validity
Adding handling of unset/default gpio on platform data which is set as
0.

 drivers/regulator/max8973-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index 1890808..85fa2de 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -408,7 +408,7 @@ static int max8973_probe(struct i2c_client *client,
 	}
 
 	if (pdata) {
-		max->dvs_gpio = pdata->dvs_gpio;
+		max->dvs_gpio = (pdata->dvs_gpio) ? pdata->dvs_gpio : -EINVAL;
 		max->enable_external_control = pdata->enable_ext_control;
 		max->curr_gpio_val = pdata->dvs_def_state;
 		max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
-- 
1.8.1.5

Re: [PATCH 2/3] regulator: max8973: make default/unset dvs gpio as invalid gpio

[Mark Brown]

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

On Mon, Apr 20, 2015 at 06:24:16PM +0530, Laxman Dewangan wrote:
> If platform data has dvs-gpio value 0 as default/unset then
> make this as invalid gpio so that function gpio_is_valid()
> can return false on this case.

OK, so this handles the problem with the first patch - it should have
been the first patch to help with review.

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

Re: [PATCH 2/3] regulator: max8973: make default/unset dvs gpio as invalid gpio

[Laxman Dewangan]

On Tuesday 21 April 2015 02:45 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Apr 20, 2015 at 06:24:16PM +0530, Laxman Dewangan wrote:
>> If platform data has dvs-gpio value 0 as default/unset then
>> make this as invalid gpio so that function gpio_is_valid()
>> can return false on this case.
> OK, so this handles the problem with the first patch - it should have
> been the first patch to help with review.
>
Yaah, I realized after reading your comment on patch 1 that I should 
have on different sequence.

Thanks for accepting the patches.

Re: [PATCH 2/3] regulator: max8973: make default/unset dvs gpio as invalid gpio

[Mark Brown]

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

On Mon, Apr 20, 2015 at 06:24:16PM +0530, Laxman Dewangan wrote:
> If platform data has dvs-gpio value 0 as default/unset then
> make this as invalid gpio so that function gpio_is_valid()
> can return false on this case.

Applied, thanks.

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

[PATCH 3/3] regulator: max8973: add DT parsing of platform specific parameter

[Laxman Dewangan]

There are some platform specific parameter required to configure
the device like enable external control, DVS gpio etc.

Add DT parsing of such properties to make platform specific data.

Update DT binding doc accordingly.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 .../bindings/regulator/max8973-regulator.txt       | 14 ++++
 drivers/regulator/max8973-regulator.c              | 85 +++++++++++++++++-----
 2 files changed, 79 insertions(+), 20 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/max8973-regulator.txt b/Documentation/devicetree/bindings/regulator/max8973-regulator.txt
index 4f15d8a..f63de7d 100644
--- a/Documentation/devicetree/bindings/regulator/max8973-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/max8973-regulator.txt
@@ -8,6 +8,20 @@ Required properties:
 Any standard regulator properties can be used to configure the single max8973
 DCDC.
 
+Optional properties:
+
+-maxim,externally-enable: boolean, externally control the regulator output
+		enable/disable.
+-maxim,dvs-gpio: GPIO which is connected to DVS pin of device.
+-maxim,dvs-default-state: Default state of GPIO during initialisation.
+		1 for HIGH and 0 for LOW.
+-maxim,enable-remote-sense: boolean, enable reote sense.
+-maxim,enable-falling-slew-rate: boolean, enable falling slew rate.
+-maxim,enable-active-discharge: boolean: enable active discharge.
+-maxim,enable-frequency-shift: boolean, enable 9% frequency shift.
+-maxim,enable-bias-control: boolean, enable bias control. By enabling this
+		startup delay can be reduce to 20us from 220us.
+
 Example:
 
 	max8973@1b {
diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index 85fa2de..da7059c 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -33,6 +33,7 @@
 #include <linux/regulator/max8973-regulator.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/i2c.h>
 #include <linux/slab.h>
 #include <linux/regmap.h>
@@ -360,6 +361,46 @@ static const struct regmap_config max8973_regmap_config = {
 	.cache_type		= REGCACHE_RBTREE,
 };
 
+static struct max8973_regulator_platform_data *max8973_parse_dt(
+		struct device *dev)
+{
+	struct max8973_regulator_platform_data *pdata;
+	struct device_node *np = dev->of_node;
+	int ret;
+	u32 pval;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	pdata->enable_ext_control = of_property_read_bool(np,
+						"maxim,externally-enable");
+	pdata->dvs_gpio = of_get_named_gpio(np, "maxim,dvs-gpio", 0);
+
+	ret = of_property_read_u32(np, "maxim,dvs-default-state", &pval);
+	if (!ret)
+		pdata->dvs_def_state = pval;
+
+	if (of_property_read_bool(np, "maxim,enable-remote-sense"))
+		pdata->control_flags  |= MAX8973_CONTROL_REMOTE_SENSE_ENABLE;
+
+	if (of_property_read_bool(np, "maxim,enable-falling-slew-rate"))
+		pdata->control_flags  |=
+				MAX8973_CONTROL_FALLING_SLEW_RATE_ENABLE;
+
+	if (of_property_read_bool(np, "maxim,enable-active-discharge"))
+		pdata->control_flags  |=
+				MAX8973_CONTROL_OUTPUT_ACTIVE_DISCH_ENABLE;
+
+	if (of_property_read_bool(np, "maxim,enable-frequency-shift"))
+		pdata->control_flags  |= MAX8973_CONTROL_FREQ_SHIFT_9PER_ENABLE;
+
+	if (of_property_read_bool(np, "maxim,enable-bias-control"))
+		pdata->control_flags  |= MAX8973_BIAS_ENABLE;
+
+	return pdata;
+}
+
 static int max8973_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -367,15 +408,24 @@ static int max8973_probe(struct i2c_client *client,
 	struct regulator_config config = { };
 	struct regulator_dev *rdev;
 	struct max8973_chip *max;
+	bool pdata_from_dt = false;
 	int ret;
 
 	pdata = dev_get_platdata(&client->dev);
 
-	if (!pdata && !client->dev.of_node) {
+	if (!pdata && client->dev.of_node) {
+		pdata = max8973_parse_dt(&client->dev);
+		pdata_from_dt = true;
+	}
+
+	if (!pdata) {
 		dev_err(&client->dev, "No Platform data");
 		return -EIO;
 	}
 
+	if (pdata->dvs_gpio == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+
 	max = devm_kzalloc(&client->dev, sizeof(*max), GFP_KERNEL);
 	if (!max)
 		return -ENOMEM;
@@ -399,7 +449,7 @@ static int max8973_probe(struct i2c_client *client,
 	max->desc.uV_step = MAX8973_VOLATGE_STEP;
 	max->desc.n_voltages = MAX8973_BUCK_N_VOLTAGE;
 
-	if (!pdata || !pdata->enable_ext_control) {
+	if (!pdata->enable_ext_control) {
 		max->desc.enable_reg = MAX8973_VOUT;
 		max->desc.enable_mask = MAX8973_VOUT_ENABLE;
 		max->ops.enable = regulator_enable_regmap;
@@ -407,15 +457,10 @@ static int max8973_probe(struct i2c_client *client,
 		max->ops.is_enabled = regulator_is_enabled_regmap;
 	}
 
-	if (pdata) {
-		max->dvs_gpio = (pdata->dvs_gpio) ? pdata->dvs_gpio : -EINVAL;
-		max->enable_external_control = pdata->enable_ext_control;
-		max->curr_gpio_val = pdata->dvs_def_state;
-		max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
-	} else {
-		max->dvs_gpio = -EINVAL;
-		max->curr_vout_reg = MAX8973_VOUT;
-	}
+	max->dvs_gpio = (pdata->dvs_gpio) ? pdata->dvs_gpio : -EINVAL;
+	max->enable_external_control = pdata->enable_ext_control;
+	max->curr_gpio_val = pdata->dvs_def_state;
+	max->curr_vout_reg = MAX8973_VOUT + pdata->dvs_def_state;
 
 	max->lru_index[0] = max->curr_vout_reg;
 
@@ -444,18 +489,18 @@ static int max8973_probe(struct i2c_client *client,
 		max->lru_index[max->curr_vout_reg] = 0;
 	}
 
-	if (pdata) {
-		ret = max8973_init_dcdc(max, pdata);
-		if (ret < 0) {
-			dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret);
-			return ret;
-		}
+	if (pdata_from_dt)
+		pdata->reg_init_data = of_get_regulator_init_data(&client->dev,
+					client->dev.of_node, &max->desc);
+
+	ret = max8973_init_dcdc(max, pdata);
+	if (ret < 0) {
+		dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret);
+		return ret;
 	}
 
 	config.dev = &client->dev;
-	config.init_data = pdata ? pdata->reg_init_data :
-		of_get_regulator_init_data(&client->dev, client->dev.of_node,
-					   &max->desc);
+	config.init_data = pdata->reg_init_data;
 	config.driver_data = max;
 	config.of_node = client->dev.of_node;
 	config.regmap = max->regmap;
-- 
1.8.1.5

Re: [PATCH 3/3] regulator: max8973: add DT parsing of platform specific parameter

[Mark Brown]

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

On Mon, Apr 20, 2015 at 06:24:17PM +0530, Laxman Dewangan wrote:

> +Optional properties:
> +
> +-maxim,externally-enable: boolean, externally control the regulator output
> +		enable/disable.

I'd have expected this to be enabled if a GPIO is provided to do the
control rather than having a specific property for it.

> +-maxim,enable-falling-slew-rate: boolean, enable falling slew rate.
> +-maxim,enable-active-discharge: boolean: enable active discharge.
> +-maxim,enable-frequency-shift: boolean, enable 9% frequency shift.
> +-maxim,enable-bias-control: boolean, enable bias control. By enabling this
> +		startup delay can be reduce to 20us from 220us.

It looks like we should be implementing the set_ramp_delay() operation
here?

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

Re: [PATCH 3/3] regulator: max8973: add DT parsing of platform specific parameter

[Laxman Dewangan]

On Tuesday 21 April 2015 02:49 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Apr 20, 2015 at 06:24:17PM +0530, Laxman Dewangan wrote:
>
>> +Optional properties:
>> +
>> +-maxim,externally-enable: boolean, externally control the regulator output
>> +		enable/disable.
> I'd have expected this to be enabled if a GPIO is provided to do the
> control rather than having a specific property for it.
Curently there is no support for providing a GPIO for external control 
pins. I have follow on patch to support this as my platform control the 
EN pin of the device through GPIO.

There is also case that the EN pin is controlled by the control signal 
from host power management controller directly and this control signal 
is not exported as GPIO.
So we need this option to support on both cases.

There is another device from Maxim, MAX77621, which is compatible on 
register with this device but it has converted the EN pin as SHTDN pin. 
I have follow on patches to support this device through this driver 
instead of duplicating the driver.

MAX8973: Vout is enabled when EN pin OR EN bit on register is 1.
MAX77621: Vout is enabled when EN pin AND EN bit on register is 1. If EN 
pin (renamed as SHTDN pin) goes low, it reset the entire register 
configuration.

In our platform, we are using MAX77621 and SHTDN pin of MAX77621 is 
controlled through GPIO.
On this case, we will use the GPIO as always HIGH and control Vout 
enable/disable through the register EN bit write otherwise, we will need 
to restore all configuration on each regulator enable.

So on MAX77621 use, we will provide the GPIO for SHTDN pin to make it 
enable always and it will not be controlled externally, it will be 
controlled through register access.

>
>> +-maxim,enable-falling-slew-rate: boolean, enable falling slew rate.
>> +-maxim,enable-active-discharge: boolean: enable active discharge.
>> +-maxim,enable-frequency-shift: boolean, enable 9% frequency shift.
>> +-maxim,enable-bias-control: boolean, enable bias control. By enabling this
>> +		startup delay can be reduce to 20us from 220us.
> It looks like we should be implementing the set_ramp_delay() operation
> here?
>
It is actually enable_time which changes with this bias enable/disable.
So we need to provide desc->enable_time based on this flag. Currently it 
is missing on driver.

I have few more patches, not sent because did not want to bulk the 
review and started with small changes which can be discussed easily.