[PATCH V1 0/4] regulator: tps62360: add cache support and settling time.

[PATCH V1 0/4] regulator: tps62360: add cache support and settling time.

[Laxman Dewangan]

This patch series is for improving the regulator tps62360 by enabling
caching, using efficient functions for reducing code size and provide
settling time.

Laxman Dewangan (4):
  regmap: add function for set/clear bits
	This add the regmap_set_bits()/regmap_clear_bits() functionlity.

  regulator: tps62360: enable register cache
	Enable the caching of register to have faster update_bits as it
	will avoid one more i2c read every time.

  regulator: tps62360: use efficient function
	This reduces code size and have better readability in the code.

  regulator: tps62360: Provide settling time for voltage change
	This will provide settlling delay when voltage output changes.

 drivers/base/regmap/regmap.c           |   32 ++++++++++
 drivers/regulator/tps62360-regulator.c |  106 +++++++++++++++++++-------------
 include/linux/regmap.h                 |   15 +++++
 3 files changed, 110 insertions(+), 43 deletions(-)

[PATCH V1 1/4] regmap: add function for set/clear bits

[Laxman Dewangan]

Add regmap_set_bits() and regmap_clear_bits() for
setting/clearing bits.
Although this can be achieve by regmap_update_bits() but
having these functions gives good readability in driver
code which uses regmap apis.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/base/regmap/regmap.c |   32 ++++++++++++++++++++++++++++++++
 include/linux/regmap.h       |   15 +++++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 8a25006..d0c8144 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -942,6 +942,38 @@ int regmap_update_bits_check(struct regmap *map, unsigned int reg,
 EXPORT_SYMBOL_GPL(regmap_update_bits_check);
 
 /**
+ * regmap_set_bits: Set bits of register.
+ *
+ * @map: Register map to update
+ * @reg: Register to update
+ * @bits: Bits to set
+ *
+ * Returns zero for success, a negative number on error.
+ */
+int regmap_set_bits(struct regmap *map, unsigned int reg, unsigned int bits)
+{
+	bool change;
+	return _regmap_update_bits(map, reg, bits, bits, &change);
+}
+EXPORT_SYMBOL_GPL(regmap_set_bits);
+
+/**
+ * regmap_clear_bits: Clear bits of register.
+ *
+ * @map: Register map to update
+ * @reg: Register to update
+ * @bits: Bits to clear
+ *
+ * Returns zero for success, a negative number on error.
+ */
+int regmap_clear_bits(struct regmap *map, unsigned int reg, unsigned int bits)
+{
+	bool change;
+	return _regmap_update_bits(map, reg, bits, 0, &change);
+}
+EXPORT_SYMBOL_GPL(regmap_clear_bits);
+
+/**
  * regmap_register_patch: Register and apply register updates to be applied
  *                        on device initialistion
  *
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index ae797b1..bc859b2 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -185,6 +185,8 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 int regmap_update_bits_check(struct regmap *map, unsigned int reg,
 			     unsigned int mask, unsigned int val,
 			     bool *change);
+int regmap_set_bits(struct regmap *map, unsigned int reg, unsigned int bits);
+int regmap_clear_bits(struct regmap *map, unsigned int reg, unsigned int bits);
 int regmap_get_val_bytes(struct regmap *map);
 
 int regcache_sync(struct regmap *map);
@@ -311,6 +313,19 @@ static inline int regmap_update_bits_check(struct regmap *map,
 	WARN_ONCE(1, "regmap API is disabled");
 	return -EINVAL;
 }
+static inline int regmap_set_bits(struct regmap *map,
+			unsigned int reg, unsigned int bits)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
+
+static inline int regmap_clear_bits(struct regmap *map,
+			unsigned int reg, unsigned int bits)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return -EINVAL;
+}
 
 static inline int regmap_get_val_bytes(struct regmap *map)
 {
-- 
1.7.1.1

Re: [PATCH V1 1/4] regmap: add function for set/clear bits

[Mark Brown]

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

On Mon, May 07, 2012 at 01:05:36PM +0530, Laxman Dewangan wrote:
> Add regmap_set_bits() and regmap_clear_bits() for
> setting/clearing bits.
> Although this can be achieve by regmap_update_bits() but
> having these functions gives good readability in driver
> code which uses regmap apis.

I'm not convinced about these for readability in the first place but...

> +int regmap_set_bits(struct regmap *map, unsigned int reg, unsigned int bits)
> +{
> +	bool change;
> +	return _regmap_update_bits(map, reg, bits, bits, &change);
> +}
> +EXPORT_SYMBOL_GPL(regmap_set_bits);

...clearly they should just be static inlines in the header file,
there's nothing in the actual function.

For readability I find moving the operation being done to the register
value into the function name tends to make it less obvious when scanning
the code what the actual change is (since with a bunch of operations
there's a lot of repeated text on the line and a fairly small amount of
text for the actual change) and can lead to the two operations following
one after another.

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

[PATCH V1 2/4] regulator: tps62360: enable register cache

[Laxman Dewangan]

Enable cache of device register using regmap cache RBTREE.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/tps62360-regulator.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index 20fef1d..375f757 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -262,8 +262,11 @@ static int tps62360_init_dcdc(struct tps62360_chip *tps,
 }
 
 static const struct regmap_config tps62360_regmap_config = {
-	.reg_bits = 8,
-	.val_bits = 8,
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= REG_CHIPID,
+	.num_reg_defaults_raw	= REG_CHIPID + 1,
+	.cache_type		= REGCACHE_RBTREE,
 };
 
 static int __devinit tps62360_probe(struct i2c_client *client,
-- 
1.7.1.1

Re: [PATCH V1 2/4] regulator: tps62360: enable register cache

[Mark Brown]

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

On Mon, May 07, 2012 at 01:05:37PM +0530, Laxman Dewangan wrote:

>  static const struct regmap_config tps62360_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> +	.reg_bits		= 8,
> +	.val_bits		= 8,
> +	.max_register		= REG_CHIPID,
> +	.num_reg_defaults_raw	= REG_CHIPID + 1,
> +	.cache_type		= REGCACHE_RBTREE,
>  };

Since you're using the rbtree cache which can fill itself gradually it's
better to not set num_reg_defaults_raw, especially not without providing
the defaults.  Doing this may lead to bugs later as the framework thinks
it has the chip defaults when it doesn't really.  If you omit this we'll
just cache gradually as we access the registers.

The ability to read the default values from the chip is intended for use
with caches which must have defaults, and should only be used when doing
a hard reset of the device during startup.

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

[PATCH V1 3/4] regulator: tps62360: use efficient function

[Laxman Dewangan]

In place of using the multiple function to achieve desire
functionality, use the function which implement that functionality,
like:
- regmap_update_bits for read-modify-write
- gpio_request_one for gpio_request and intial setting output.

This reduces code size effectively.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/tps62360-regulator.c |   53 +++++++++++---------------------
 1 files changed, 18 insertions(+), 35 deletions(-)

diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index 375f757..ad7d67a 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -203,23 +203,15 @@ static int tps62360_init_force_pwm(struct tps62360_chip *tps,
 	struct tps62360_regulator_platform_data *pdata,
 	int vset_id)
 {
-	unsigned int data;
 	int ret;
-	ret = regmap_read(tps->regmap, REG_VSET0 + vset_id, &data);
-	if (ret < 0) {
-		dev_err(tps->dev, "%s() fails in writing reg %d\n",
-			__func__, REG_VSET0 + vset_id);
-		return ret;
-	}
-	tps->curr_vset_vsel[vset_id] = data & tps->voltage_reg_mask;
+	int reg = REG_VSET0 + vset_id;
 	if (pdata->en_force_pwm)
-		data |= BIT(7);
+		ret = regmap_set_bits(tps->regmap, reg, BIT(7));
 	else
-		data &= ~BIT(7);
-	ret = regmap_write(tps->regmap, REG_VSET0 + vset_id, data);
+		ret = regmap_clear_bits(tps->regmap, reg, BIT(7));
 	if (ret < 0)
-		dev_err(tps->dev, "%s() fails in writing reg %d\n",
-				__func__, REG_VSET0 + vset_id);
+		dev_err(tps->dev, "%s() register %d configuration fails\n",
+			__func__, reg);
 	return ret;
 }
 
@@ -254,9 +246,9 @@ static int tps62360_init_dcdc(struct tps62360_chip *tps,
 	}
 
 	/* Reset output discharge path to reduce power consumption */
-	ret = regmap_update_bits(tps->regmap, REG_RAMPCTRL, BIT(2), 0);
+	ret = regmap_clear_bits(tps->regmap, REG_RAMPCTRL, BIT(2));
 	if (ret < 0)
-		dev_err(tps->dev, "%s() fails in updating reg %d\n",
+		dev_err(tps->dev, "%s() fails in clearing bits of reg %d\n",
 			__func__, REG_RAMPCTRL);
 	return ret;
 }
@@ -337,37 +329,28 @@ static int __devinit tps62360_probe(struct i2c_client *client,
 	tps->valid_gpios = false;
 
 	if (gpio_is_valid(tps->vsel0_gpio) && gpio_is_valid(tps->vsel1_gpio)) {
-		ret = gpio_request(tps->vsel0_gpio, "tps62360-vsel0");
+		int gpio_flags;
+		gpio_flags = (pdata->vsel0_def_state) ?
+				GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
+		ret = gpio_request_one(tps->vsel0_gpio,
+				gpio_flags, "tps62360-vsel0");
 		if (ret) {
 			dev_err(&client->dev,
 				"Err: Could not obtain vsel0 GPIO %d: %d\n",
 						tps->vsel0_gpio, ret);
 			goto err_gpio0;
 		}
-		ret = gpio_direction_output(tps->vsel0_gpio,
-					pdata->vsel0_def_state);
-		if (ret) {
-			dev_err(&client->dev, "Err: Could not set direction of"
-				"vsel0 GPIO %d: %d\n", tps->vsel0_gpio, ret);
-			gpio_free(tps->vsel0_gpio);
-			goto err_gpio0;
-		}
 
-		ret = gpio_request(tps->vsel1_gpio, "tps62360-vsel1");
+		gpio_flags = (pdata->vsel1_def_state) ?
+				GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
+		ret = gpio_request_one(tps->vsel1_gpio,
+				gpio_flags, "tps62360-vsel1");
 		if (ret) {
 			dev_err(&client->dev,
 				"Err: Could not obtain vsel1 GPIO %d: %d\n",
 						tps->vsel1_gpio, ret);
 			goto err_gpio1;
 		}
-		ret = gpio_direction_output(tps->vsel1_gpio,
-					pdata->vsel1_def_state);
-		if (ret) {
-			dev_err(&client->dev, "Err: Could not set direction of"
-				"vsel1 GPIO %d: %d\n", tps->vsel1_gpio, ret);
-			gpio_free(tps->vsel1_gpio);
-			goto err_gpio1;
-		}
 		tps->valid_gpios = true;
 
 		/*
@@ -442,9 +425,9 @@ static void tps62360_shutdown(struct i2c_client *client)
 		return;
 
 	/* Configure the output discharge path */
-	st = regmap_update_bits(tps->regmap, REG_RAMPCTRL, BIT(2), BIT(2));
+	st = regmap_set_bits(tps->regmap, REG_RAMPCTRL, BIT(2));
 	if (st < 0)
-		dev_err(tps->dev, "%s() fails in updating reg %d\n",
+		dev_err(tps->dev, "%s() fails in setting reg %d\n",
 			__func__, REG_RAMPCTRL);
 }
 
-- 
1.7.1.1

[PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Laxman Dewangan]

Settling time is require when there is voltage output change.
Implement set_voltage_time_sel() callback which returns delay time
for voltage change to settle down to new value.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/regulator/tps62360-regulator.c |   46 +++++++++++++++++++++++++++----
 1 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/regulator/tps62360-regulator.c b/drivers/regulator/tps62360-regulator.c
index ad7d67a..3fd7341 100644
--- a/drivers/regulator/tps62360-regulator.c
+++ b/drivers/regulator/tps62360-regulator.c
@@ -71,6 +71,7 @@ struct tps62360_chip {
 	int lru_index[4];
 	int curr_vset_vsel[4];
 	int curr_vset_id;
+	int change_uv_per_us;
 };
 
 /*
@@ -114,7 +115,7 @@ update_lru_index:
 	return found;
 }
 
-static int tps62360_dcdc_get_voltage(struct regulator_dev *dev)
+static int tps62360_dcdc_get_voltage_sel(struct regulator_dev *dev)
 {
 	struct tps62360_chip *tps = rdev_get_drvdata(dev);
 	int vsel;
@@ -128,7 +129,7 @@ static int tps62360_dcdc_get_voltage(struct regulator_dev *dev)
 		return ret;
 	}
 	vsel = (int)data & tps->voltage_reg_mask;
-	return (tps->voltage_base + vsel * 10) * 1000;
+	return vsel;
 }
 
 static int tps62360_dcdc_set_voltage(struct regulator_dev *dev,
@@ -193,10 +194,28 @@ static int tps62360_dcdc_list_voltage(struct regulator_dev *dev,
 	return (tps->voltage_base + selector * 10) * 1000;
 }
 
+static int tps62360_set_voltage_time_sel(struct regulator_dev *rdev,
+		unsigned int old_selector, unsigned int new_selector)
+{
+	struct tps62360_chip *tps = rdev_get_drvdata(rdev);
+	int old_uV, new_uV;
+
+	old_uV = tps62360_dcdc_list_voltage(rdev, old_selector);
+	if (old_uV < 0)
+		return old_uV;
+
+	new_uV = tps62360_dcdc_list_voltage(rdev, new_selector);
+	if (new_uV < 0)
+		return new_uV;
+
+	return DIV_ROUND_UP(abs(old_uV - new_uV), tps->change_uv_per_us);
+}
+
 static struct regulator_ops tps62360_dcdc_ops = {
-	.get_voltage = tps62360_dcdc_get_voltage,
-	.set_voltage = tps62360_dcdc_set_voltage,
-	.list_voltage = tps62360_dcdc_list_voltage,
+	.get_voltage_sel	= tps62360_dcdc_get_voltage_sel,
+	.set_voltage		= tps62360_dcdc_set_voltage,
+	.list_voltage		= tps62360_dcdc_list_voltage,
+	.set_voltage_time_sel	= tps62360_set_voltage_time_sel,
 };
 
 static int tps62360_init_force_pwm(struct tps62360_chip *tps,
@@ -220,6 +239,7 @@ static int tps62360_init_dcdc(struct tps62360_chip *tps,
 {
 	int ret;
 	int i;
+	unsigned int ramp_ctrl;
 
 	/* Initailize internal pull up/down control */
 	if (tps->en_internal_pulldn)
@@ -247,9 +267,23 @@ static int tps62360_init_dcdc(struct tps62360_chip *tps,
 
 	/* Reset output discharge path to reduce power consumption */
 	ret = regmap_clear_bits(tps->regmap, REG_RAMPCTRL, BIT(2));
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(tps->dev, "%s() fails in clearing bits of reg %d\n",
 			__func__, REG_RAMPCTRL);
+		return ret;
+	}
+
+	/* Get ramp value from ramp control register */
+	ret = regmap_read(tps->regmap, REG_RAMPCTRL, &ramp_ctrl);
+	if (ret < 0) {
+		dev_err(tps->dev, "%s() fails in reading reg %d\n",
+			__func__, REG_RAMPCTRL);
+		return ret;
+	}
+	ramp_ctrl = (ramp_ctrl >> 4) & 0x7;
+
+	/* ramp mV/us = 32/(2^ramp_ctrl) */
+	tps->change_uv_per_us = DIV_ROUND_UP(32000, BIT(ramp_ctrl));
 	return ret;
 }
 
-- 
1.7.1.1

Re: [PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Mark Brown]

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

On Mon, May 07, 2012 at 01:05:39PM +0530, Laxman Dewangan wrote:
> Settling time is require when there is voltage output change.
> Implement set_voltage_time_sel() callback which returns delay time
> for voltage change to settle down to new value.

This is fine but depends on your previous stylistic change so won't
apply.  In general you should always try to order things so that there
are fewer dependencies at the start of the series - there's no actual
dependency between this and the stylistic changes you've made.

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

Re: [PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Laxman Dewangan]

On Monday 07 May 2012 04:53 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, May 07, 2012 at 01:05:39PM +0530, Laxman Dewangan wrote:
>> Settling time is require when there is voltage output change.
>> Implement set_voltage_time_sel() callback which returns delay time
>> for voltage change to settle down to new value.
> This is fine but depends on your previous stylistic change so won't
> apply.  In general you should always try to order things so that there
> are fewer dependencies at the start of the series - there's no actual
> dependency between this and the stylistic changes you've made.
>
Thanks for quick review.

Yaah, I should put the stylistic change at end..
I will send next patch for enabling cache and settling time.
Will keep discussion on stylistic changes and so will remove that from 
this series.

Re: [PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Mark Brown]

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

On Mon, May 07, 2012 at 06:12:04PM +0530, Laxman Dewangan wrote:

> Yaah, I should put the stylistic change at end..
> I will send next patch for enabling cache and settling time.
> Will keep discussion on stylistic changes and so will remove that
> from this series.

TBH I'd probably apply the style change if the regmap bit were using
static inlines, though I'm not a big fan and wouldn't be happy with it
in any consumer side code that I actively work on.

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

Re: [PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Laxman Dewangan]

On Monday 07 May 2012 07:52 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, May 07, 2012 at 06:12:04PM +0530, Laxman Dewangan wrote:
>
>> Yaah, I should put the stylistic change at end..
>> I will send next patch for enabling cache and settling time.
>> Will keep discussion on stylistic changes and so will remove that
>> from this series.
> TBH I'd probably apply the style change if the regmap bit were using
> static inlines, though I'm not a big fan and wouldn't be happy with it
> in any consumer side code that I actively work on.

Actually I saw the multiple driver which uses set_bits()/clear_bits() 
and thought that this is good way of writing clean code and so developed 
multiple driver surrounding these apis.  Now seems it is easy for me to 
use these apis. May be it is my personal taste.

I will send the another patch making the function as static inline.

Thanks,
Laxman

Re: [PATCH V1 4/4] regulator: tps62360: Provide settling time for voltage change

[Mark Brown]

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

On Mon, May 07, 2012 at 08:15:13PM +0530, Laxman Dewangan wrote:

> Actually I saw the multiple driver which uses
> set_bits()/clear_bits() and thought that this is good way of writing
> clean code and so developed multiple driver surrounding these apis.
> Now seems it is easy for me to use these apis. May be it is my
> personal taste.

It's a taste thing, a lot of those drivers are doing this because they
cloned the wm8350 driver (or something else which was based off the
wm8350 driver - it was the first of these).

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