[PATCH] regulator/max1586: support increased V3 voltage range

[PATCH] regulator/max1586: support increased V3 voltage range

[Philipp Zabel]

The V3 regulator can be configured with an external resistor
connected to the feedback pin (R24 in the data sheet) to
increase the voltage range.

For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/regulator/max1586.c       |   75 ++++++++++++++++++++++++++++---------
 include/linux/regulator/max1586.h |    7 +++
 2 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index bbbb55f..db11099 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -27,43 +27,56 @@
 #define MAX1586_V3_MAX_VSEL 31
 #define MAX1586_V6_MAX_VSEL 3
 
-#define MAX1586_V3_MIN_UV   700000
-#define MAX1586_V3_MAX_UV  1475000
-#define MAX1586_V3_STEP_UV   25000
-
 #define MAX1586_V6_MIN_UV        0
 #define MAX1586_V6_MAX_UV  3000000
 
 #define I2C_V3_SELECT (0 << 5)
 #define I2C_V6_SELECT (1 << 5)
 
+struct max1586_data {
+	struct i2c_client *client;
+
+	/* min/max V3 voltage */
+	int min_uV;
+	int max_uV;
+};
+
 /*
  * V3 voltage
  * On I2C bus, sending a "x" byte to the max1586 means :
- *   set V3 to 0.700V + (x & 0x1f) * 0.025V
+ *   set V3 to 0.700V + (x & 0x1f) * 0.025V,
+ *   set V3 to 0.730V + (x & 0x1f) * 0.026V,
+ *   set V3 to 0.750V + (x & 0x1f) * 0.027V or
+ *   set V3 to 0.780V + (x & 0x1f) * 0.028V
+ * depending on the external resistance R24.
  */
-static int max1586_v3_calc_voltage(unsigned selector)
+static int max1586_v3_calc_voltage(struct max1586_data *max1586,
+	unsigned selector)
 {
-	return MAX1586_V3_MIN_UV + (MAX1586_V3_STEP_UV * selector);
+	unsigned range_uV = max1586->max_uV - max1586->min_uV;
+
+	return max1586->min_uV + (selector * range_uV / 31);
 }
 
 static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
 {
-	struct i2c_client *client = rdev_get_drvdata(rdev);
+	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
+	struct i2c_client *client = max1586->client;
+	unsigned range_uV = max1586->max_uV - max1586->min_uV;
 	unsigned selector;
 	u8 v3_prog;
 
-	if (min_uV < MAX1586_V3_MIN_UV || min_uV > MAX1586_V3_MAX_UV)
-		return -EINVAL;
-	if (max_uV < MAX1586_V3_MIN_UV || max_uV > MAX1586_V3_MAX_UV)
+	if (min_uV > max1586->max_uV || max_uV < max1586->min_uV)
 		return -EINVAL;
+	if (min_uV < max1586->min_uV)
+		min_uV = max1586->min_uV;
 
-	selector = (min_uV - MAX1586_V3_MIN_UV) / MAX1586_V3_STEP_UV;
-	if (max1586_v3_calc_voltage(selector) > max_uV)
+	selector = (min_uV - max1586->min_uV) * 31 / range_uV;
+	if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
 		return -EINVAL;
 
 	dev_dbg(&client->dev, "changing voltage v3 to %dmv\n",
-		max1586_v3_calc_voltage(selector) / 1000);
+		max1586_v3_calc_voltage(max1586, selector) / 1000);
 
 	v3_prog = I2C_V3_SELECT | (u8) selector;
 	return i2c_smbus_write_byte(client, v3_prog);
@@ -71,9 +84,11 @@ static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
 
 static int max1586_v3_list(struct regulator_dev *rdev, unsigned selector)
 {
+	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
+
 	if (selector > MAX1586_V3_MAX_VSEL)
 		return -EINVAL;
-	return max1586_v3_calc_voltage(selector);
+	return max1586_v3_calc_voltage(max1586, selector);
 }
 
 /*
@@ -164,12 +179,33 @@ static int max1586_pmic_probe(struct i2c_client *client,
 {
 	struct regulator_dev **rdev;
 	struct max1586_platform_data *pdata = client->dev.platform_data;
-	int i, id, ret = 0;
+	struct max1586_data *max1586;
+	int i, id, ret = -ENOMEM;
 
 	rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
 		       GFP_KERNEL);
 	if (!rdev)
-		return -ENOMEM;
+		goto out;
+
+	max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
+	if (!max1586)
+		goto out_unmap;
+
+	max1586->client = client;
+	switch (pdata->r24) {
+	case MAX1586_R24_3k32:
+		max1586->min_uV = 730000;
+		max1586->max_uV = 1550000;
+		break;
+	case MAX1586_R24_5k11:
+		max1586->min_uV = 750000;
+		max1586->max_uV = 1600000;
+		break;
+	case MAX1586_R24_7k5:
+		max1586->min_uV = 780000;
+		max1586->max_uV = 1650000;
+		break;
+	}
 
 	ret = -EINVAL;
 	for (i = 0; i < pdata->num_subdevs && i <= MAX1586_V6; i++) {
@@ -182,7 +218,7 @@ static int max1586_pmic_probe(struct i2c_client *client,
 		}
 		rdev[i] = regulator_register(&max1586_reg[id], &client->dev,
 					     pdata->subdevs[i].platform_data,
-					     client);
+					     max1586);
 		if (IS_ERR(rdev[i])) {
 			ret = PTR_ERR(rdev[i]);
 			dev_err(&client->dev, "failed to register %s\n",
@@ -198,7 +234,10 @@ static int max1586_pmic_probe(struct i2c_client *client,
 err:
 	while (--i >= 0)
 		regulator_unregister(rdev[i]);
+	kfree(max1586);
+out_unmap:
 	kfree(rdev);
+out:
 	return ret;
 }
 
diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
index 2056973..cdf2010 100644
--- a/include/linux/regulator/max1586.h
+++ b/include/linux/regulator/max1586.h
@@ -26,6 +26,10 @@
 #define MAX1586_V3 0
 #define MAX1586_V6 1
 
+#define MAX1586_R24_3k32 3320
+#define MAX1586_R24_5k11 5110
+#define MAX1586_R24_7k5  7500
+
 /**
  * max1586_subdev_data - regulator data
  * @id: regulator Id (either MAX1586_V3 or MAX1586_V6)
@@ -43,10 +47,13 @@ struct max1586_subdev_data {
  * @num_subdevs: number of regultors used (may be 1 or 2)
  * @subdevs: regulator used
  *           At most, there will be a regulator for V3 and one for V6 voltages.
+ * @r24: external regulator to increase V3 range
+ *       (0, MAX1586_R24_3k32, MAX1586_R24_5k11 or MAX1586_R24_7k5)
  */
 struct max1586_platform_data {
 	int num_subdevs;
 	struct max1586_subdev_data *subdevs;
+	int r24;
 };
 
 #endif
-- 
1.6.3.1

Re: [PATCH] regulator/max1586: support increased V3 voltage range

[Mark Brown]

On Tue, May 26, 2009 at 09:39:08PM +0200, Philipp Zabel wrote:

Please don't use my @wolfsonmicro.com address for anything kernel
related - use @opensource.wolfsonmicro.com or @sirena.org.uk.

> -#define MAX1586_V3_MIN_UV   700000
> -#define MAX1586_V3_MAX_UV  1475000
> -#define MAX1586_V3_STEP_UV   25000

I don't see these values being re-added elsewhere in the code?

> +	selector = (min_uV - max1586->min_uV) * 31 / range_uV;
> +	if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
>  		return -EINVAL;

Might be nice to have a #define for the 31 but then it's the only
reference to it.

>  	rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>  		       GFP_KERNEL);
>  	if (!rdev)
> -		return -ENOMEM;
> +		goto out;
> +
> +	max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
> +	if (!max1586)
> +		goto out_unmap;

Could perhaps make these one struct?  But either way is fine.

> +       switch (pdata->r24) {
> +       case MAX1586_R24_3k32:

This switch statement doesn't reject invalid values which I'd expect it
to and...

> + * @r24: external regulator to increase V3 range
> + *       (0, MAX1586_R24_3k32, MAX1586_R24_5k11 or MAX1586_R24_7k5)

...I think I'd be more comfortable if an explicit configuration were
required for the no feedback case since otherwise the driver will
happily start up and begin producing the wrong output voltages by
default on platforms which have this feedback resistor fitted.  This
could make the system non-obviously unstable and in the worst case
possibly lead to long term hardware damage if something is consistently
driven over voltage.

Re: [PATCH] regulator/max1586: support increased V3 voltage range

[pHilipp Zabel]

Hi Mark,

thanks for the review.

On Tue, May 26, 2009 at 10:43 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, May 26, 2009 at 09:39:08PM +0200, Philipp Zabel wrote:
>
> Please don't use my @wolfsonmicro.com address for anything kernel
> related - use @opensource.wolfsonmicro.com or @sirena.org.uk.

Copy&paste error, won't happen again.

>> -#define MAX1586_V3_MIN_UV   700000
>> -#define MAX1586_V3_MAX_UV  1475000
>> -#define MAX1586_V3_STEP_UV   25000
>
> I don't see these values being re-added elsewhere in the code?

Ouh, strange thinko. For some reason I left them out of the switch
statement because they felt like default values. And why wouldn't the
structure be initialised with default values, right? Will add them
there.

>> +     selector = (min_uV - max1586->min_uV) * 31 / range_uV;
>> +     if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
>>               return -EINVAL;
>
> Might be nice to have a #define for the 31 but then it's the only
> reference to it.

There is a "#define MAX1586_V3_MAX_VSEL 31" already, I'll use that.
(And there's a second reference in _calc_voltage)

>>       rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>>                      GFP_KERNEL);
>>       if (!rdev)
>> -             return -ENOMEM;
>> +             goto out;
>> +
>> +     max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
>> +     if (!max1586)
>> +             goto out_unmap;
>
> Could perhaps make these one struct?  But either way is fine.

Yes, I could put the struct regulator_dev[] at the end of max1586_data.

>> +       switch (pdata->r24) {
>> +       case MAX1586_R24_3k32:
>
> This switch statement doesn't reject invalid values which I'd expect it
> to and...
>
>> + * @r24: external regulator to increase V3 range
>> + *       (0, MAX1586_R24_3k32, MAX1586_R24_5k11 or MAX1586_R24_7k5)
>
> ...I think I'd be more comfortable if an explicit configuration were
> required for the no feedback case since otherwise the driver will
> happily start up and begin producing the wrong output voltages by
> default on platforms which have this feedback resistor fitted.  This
> could make the system non-obviously unstable and in the worst case
> possibly lead to long term hardware damage if something is consistently
> driven over voltage.

Ok. I'll add something like "#define MAX1586_NO_R24 -1" and reject
everything else with -EINVAL.

regards
Philipp

Re: [PATCH] regulator/max1586: support increased V3 voltage range

[Robert Jarzmik]

Philipp Zabel <philipp.zabel@gmail.com> writes:

> The V3 regulator can be configured with an external resistor
> connected to the feedback pin (R24 in the data sheet) to
> increase the voltage range.
>
> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
Cool.
Let me ask minor changes to the calculation formula, and r24 parameter.

>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  drivers/regulator/max1586.c       |   75 ++++++++++++++++++++++++++++---------
>  include/linux/regulator/max1586.h |    7 +++
>  2 files changed, 64 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
> index bbbb55f..db11099 100644
> --- a/drivers/regulator/max1586.c
> +++ b/drivers/regulator/max1586.c
> @@ -27,43 +27,56 @@
>  #define MAX1586_V3_MAX_VSEL 31
>  #define MAX1586_V6_MAX_VSEL 3
>  
> -#define MAX1586_V3_MIN_UV   700000
> -#define MAX1586_V3_MAX_UV  1475000
> -#define MAX1586_V3_STEP_UV   25000
These 3 should be kept I think, see below.

> -
>  #define MAX1586_V6_MIN_UV        0
>  #define MAX1586_V6_MAX_UV  3000000
>  
>  #define I2C_V3_SELECT (0 << 5)
>  #define I2C_V6_SELECT (1 << 5)
>  
> +struct max1586_data {
> +	struct i2c_client *client;
> +
> +	/* min/max V3 voltage */
> +	int min_uV;
> +	int max_uV;

> +};
> +
>  /*
>   * V3 voltage
>   * On I2C bus, sending a "x" byte to the max1586 means :
> - *   set V3 to 0.700V + (x & 0x1f) * 0.025V
> + *   set V3 to 0.700V + (x & 0x1f) * 0.025V,
> + *   set V3 to 0.730V + (x & 0x1f) * 0.026V,
> + *   set V3 to 0.750V + (x & 0x1f) * 0.027V or
> + *   set V3 to 0.780V + (x & 0x1f) * 0.028V
> + * depending on the external resistance R24.
That's not really the full truth.

The real V3 is (0.700V + (x & 0x1f) * 0.025V) * gain, where :
 - if R24 is not connected, gain = 1
 - if R24 and R25 are connected, gain = 1 + R24/R25 + R24/185.5
   Here, it should be assumed that R25 >> 10k.ohm, which means that R24/R25
   becomes meaningless compared to R24/185.5.
   Therefore, the formula becomes:
     gain = 1 + R24/185.5
For reference, R24 and R25 are described in Max1586 datasheet, in figure7
(extending the maximum core voltage range).

I'd like that put in the comment please.

>  
>  static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>  {
> -	struct i2c_client *client = rdev_get_drvdata(rdev);
> +	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
> +	struct i2c_client *client = max1586->client;
> +	unsigned range_uV = max1586->max_uV - max1586->min_uV;
>  	unsigned selector;
>  	u8 v3_prog;
>  
> -	if (min_uV < MAX1586_V3_MIN_UV || min_uV > MAX1586_V3_MAX_UV)
> -		return -EINVAL;
> -	if (max_uV < MAX1586_V3_MIN_UV || max_uV > MAX1586_V3_MAX_UV)
> +	if (min_uV > max1586->max_uV || max_uV < max1586->min_uV)
>  		return -EINVAL;
> +	if (min_uV < max1586->min_uV)
> +		min_uV = max1586->min_uV;
>  
> -	selector = (min_uV - MAX1586_V3_MIN_UV) / MAX1586_V3_STEP_UV;
> -	if (max1586_v3_calc_voltage(selector) > max_uV)
> +	selector = (min_uV - max1586->min_uV) * 31 / range_uV;
                                                \-> ???
Why 31 ? Could I have a define here, and probably that define would be a value
of 32 (as there are 32 different steps, not 31).

> +	if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
>  		return -EINVAL;
>  
>  	dev_dbg(&client->dev, "changing voltage v3 to %dmv\n",
> -		max1586_v3_calc_voltage(selector) / 1000);
> +		max1586_v3_calc_voltage(max1586, selector) / 1000);
>  
>  	v3_prog = I2C_V3_SELECT | (u8) selector;
>  	return i2c_smbus_write_byte(client, v3_prog);
> @@ -71,9 +84,11 @@ static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>  
>  static int max1586_v3_list(struct regulator_dev *rdev, unsigned selector)
>  {
> +	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
> +
>  	if (selector > MAX1586_V3_MAX_VSEL)
>  		return -EINVAL;
> -	return max1586_v3_calc_voltage(selector);
> +	return max1586_v3_calc_voltage(max1586, selector);
>  }
>  
>  /*
> @@ -164,12 +179,33 @@ static int max1586_pmic_probe(struct i2c_client *client,
>  {
>  	struct regulator_dev **rdev;
>  	struct max1586_platform_data *pdata = client->dev.platform_data;
> -	int i, id, ret = 0;
> +	struct max1586_data *max1586;
> +	int i, id, ret = -ENOMEM;
>  
>  	rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>  		       GFP_KERNEL);
>  	if (!rdev)
> -		return -ENOMEM;
> +		goto out;
> +
> +	max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
> +	if (!max1586)
> +		goto out_unmap;
> +
> +	max1586->client = client;

> +	switch (pdata->r24) {
> +	case MAX1586_R24_3k32:
> +		max1586->min_uV = 730000;
> +		max1586->max_uV = 1550000;
> +		break;
> +	case MAX1586_R24_5k11:
> +		max1586->min_uV = 750000;
> +		max1586->max_uV = 1600000;
> +		break;
> +	case MAX1586_R24_7k5:
> +		max1586->min_uV = 780000;
> +		max1586->max_uV = 1650000;
> +		break;
> +	}
I would like that changed to something more or less like :
if (pdata->r24 == 0)
	gain = 1;
else
	gain = 1 + R24/185.5;
max1586->min_uV = gain * MAX1586_V3_IN_UV;
max1586->max_uV = gain * MAX1586_V3_MAX_UV;

>  
>  	ret = -EINVAL;
>  	for (i = 0; i < pdata->num_subdevs && i <= MAX1586_V6; i++) {
> @@ -182,7 +218,7 @@ static int max1586_pmic_probe(struct i2c_client *client,
>  		}
>  		rdev[i] = regulator_register(&max1586_reg[id], &client->dev,
>  					     pdata->subdevs[i].platform_data,
> -					     client);
> +					     max1586);
>  		if (IS_ERR(rdev[i])) {
>  			ret = PTR_ERR(rdev[i]);
>  			dev_err(&client->dev, "failed to register %s\n",
> @@ -198,7 +234,10 @@ static int max1586_pmic_probe(struct i2c_client *client,
>  err:
>  	while (--i >= 0)
>  		regulator_unregister(rdev[i]);
> +	kfree(max1586);
> +out_unmap:
>  	kfree(rdev);
> +out:
>  	return ret;
>  }
>  
> diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
> index 2056973..cdf2010 100644
> --- a/include/linux/regulator/max1586.h
> +++ b/include/linux/regulator/max1586.h
> @@ -26,6 +26,10 @@
>  #define MAX1586_V3 0
>  #define MAX1586_V6 1
>  
> +#define MAX1586_R24_3k32 3320
> +#define MAX1586_R24_5k11 5110
> +#define MAX1586_R24_7k5  7500
This could be gone, as R24 will define the exact value of R24 in ohms.

> +
>  /**
>   * max1586_subdev_data - regulator data
>   * @id: regulator Id (either MAX1586_V3 or MAX1586_V6)
> @@ -43,10 +47,13 @@ struct max1586_subdev_data {
>   * @num_subdevs: number of regultors used (may be 1 or 2)
>   * @subdevs: regulator used
>   *           At most, there will be a regulator for V3 and one for V6 voltages.
> + * @r24: external regulator to increase V3 range
Rather :
+ * @r24: external resistor value to increase V3 range (in ohms), or 0 if none

Cheers.

--
Robert

Re: [PATCH] regulator/max1586: support increased V3 voltage range

[pHilipp Zabel]

On Tue, May 26, 2009 at 11:31 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Philipp Zabel <philipp.zabel@gmail.com> writes:
>
>> The V3 regulator can be configured with an external resistor
>> connected to the feedback pin (R24 in the data sheet) to
>> increase the voltage range.
>>
>> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
>> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
> Cool.
> Let me ask minor changes to the calculation formula, and r24 parameter.
>
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
>> ---
>>  drivers/regulator/max1586.c       |   75 ++++++++++++++++++++++++++++---------
>>  include/linux/regulator/max1586.h |    7 +++
>>  2 files changed, 64 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
>> index bbbb55f..db11099 100644
>> --- a/drivers/regulator/max1586.c
>> +++ b/drivers/regulator/max1586.c
>> @@ -27,43 +27,56 @@
>>  #define MAX1586_V3_MAX_VSEL 31
>>  #define MAX1586_V6_MAX_VSEL 3
>>
>> -#define MAX1586_V3_MIN_UV   700000
>> -#define MAX1586_V3_MAX_UV  1475000
>> -#define MAX1586_V3_STEP_UV   25000
> These 3 should be kept I think, see below.

The nice thing about the switch statement below was that I just had to
take the nominal values from the datasheet and not think about the
actual mechanism at all.

>> -
>>  #define MAX1586_V6_MIN_UV        0
>>  #define MAX1586_V6_MAX_UV  3000000
>>
>>  #define I2C_V3_SELECT (0 << 5)
>>  #define I2C_V6_SELECT (1 << 5)
>>
>> +struct max1586_data {
>> +     struct i2c_client *client;
>> +
>> +     /* min/max V3 voltage */
>> +     int min_uV;
>> +     int max_uV;
>
>> +};
>> +
>>  /*
>>   * V3 voltage
>>   * On I2C bus, sending a "x" byte to the max1586 means :
>> - *   set V3 to 0.700V + (x & 0x1f) * 0.025V
>> + *   set V3 to 0.700V + (x & 0x1f) * 0.025V,
>> + *   set V3 to 0.730V + (x & 0x1f) * 0.026V,
>> + *   set V3 to 0.750V + (x & 0x1f) * 0.027V or
>> + *   set V3 to 0.780V + (x & 0x1f) * 0.028V
>> + * depending on the external resistance R24.
> That's not really the full truth.

Hm, right. My MAX1586A/MAX1586B/MAX1587A data sheet only has  this
information in (Figure 9. Adding R24 and R25 to Increase Core Voltage
Programming Range):

R24 = 3.32kOhm, V3: 0.73V TO 1.55V, 26mV/STEP
R24 = 5.11kOhm, V3: 0.75V TO 1.60V, 27mV/STEP
R24 = 7.5kkOhm, V3: 0.78V TO 1.65V, 28mV/STEP

With R25 = 100kOhm. There's no mention of 185.5(kOhm?) anywhere.

> The real V3 is (0.700V + (x & 0x1f) * 0.025V) * gain, where :
>  - if R24 is not connected, gain = 1
>  - if R24 and R25 are connected, gain = 1 + R24/R25 + R24/185.5

Ok, so for example:
R24 = 3.32kOhm --> gain = 1.051098 --> 735768uV ... 1550369uV
But the nominal values from the datasheet for this R24 resistor are
730mV and 1550mV. Should we trust this calculation over the nominal
values?

>   Here, it should be assumed that R25 >> 10k.ohm, which means that R24/R25
>   becomes meaningless compared to R24/185.5.
>   Therefore, the formula becomes:
>     gain = 1 + R24/185.5

That is not true. For this formula to make sense, 185.5 has to be
given in kΩ, so R24/R25 is actually the bigger contribution.

> For reference, R24 and R25 are described in Max1586 datasheet, in figure7
> (extending the maximum core voltage range).
>
> I'd like that put in the comment please.

Yes, it would be better to document this.

>>
>>  static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>>  {
>> -     struct i2c_client *client = rdev_get_drvdata(rdev);
>> +     struct max1586_data *max1586 = rdev_get_drvdata(rdev);
>> +     struct i2c_client *client = max1586->client;
>> +     unsigned range_uV = max1586->max_uV - max1586->min_uV;
>>       unsigned selector;
>>       u8 v3_prog;
>>
>> -     if (min_uV < MAX1586_V3_MIN_UV || min_uV > MAX1586_V3_MAX_UV)
>> -             return -EINVAL;
>> -     if (max_uV < MAX1586_V3_MIN_UV || max_uV > MAX1586_V3_MAX_UV)
>> +     if (min_uV > max1586->max_uV || max_uV < max1586->min_uV)
>>               return -EINVAL;
>> +     if (min_uV < max1586->min_uV)
>> +             min_uV = max1586->min_uV;
>>
>> -     selector = (min_uV - MAX1586_V3_MIN_UV) / MAX1586_V3_STEP_UV;
>> -     if (max1586_v3_calc_voltage(selector) > max_uV)
>> +     selector = (min_uV - max1586->min_uV) * 31 / range_uV;
>                                                \-> ???
> Why 31 ? Could I have a define here, and probably that define would be a value
> of 32 (as there are 32 different steps, not 31).

We have 32 steps, 0 to 31. The difference between the smallest and
largest value is still 31. I should have used MAX1586_V3_MAX_VSEL here
to avoid confusion.

>> +     if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
>>               return -EINVAL;
>>
>>       dev_dbg(&client->dev, "changing voltage v3 to %dmv\n",
>> -             max1586_v3_calc_voltage(selector) / 1000);
>> +             max1586_v3_calc_voltage(max1586, selector) / 1000);
>>
>>       v3_prog = I2C_V3_SELECT | (u8) selector;
>>       return i2c_smbus_write_byte(client, v3_prog);
>> @@ -71,9 +84,11 @@ static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>>
>>  static int max1586_v3_list(struct regulator_dev *rdev, unsigned selector)
>>  {
>> +     struct max1586_data *max1586 = rdev_get_drvdata(rdev);
>> +
>>       if (selector > MAX1586_V3_MAX_VSEL)
>>               return -EINVAL;
>> -     return max1586_v3_calc_voltage(selector);
>> +     return max1586_v3_calc_voltage(max1586, selector);
>>  }
>>
>>  /*
>> @@ -164,12 +179,33 @@ static int max1586_pmic_probe(struct i2c_client *client,
>>  {
>>       struct regulator_dev **rdev;
>>       struct max1586_platform_data *pdata = client->dev.platform_data;
>> -     int i, id, ret = 0;
>> +     struct max1586_data *max1586;
>> +     int i, id, ret = -ENOMEM;
>>
>>       rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>>                      GFP_KERNEL);
>>       if (!rdev)
>> -             return -ENOMEM;
>> +             goto out;
>> +
>> +     max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
>> +     if (!max1586)
>> +             goto out_unmap;
>> +
>> +     max1586->client = client;
>
>> +     switch (pdata->r24) {
>> +     case MAX1586_R24_3k32:
>> +             max1586->min_uV = 730000;
>> +             max1586->max_uV = 1550000;
>> +             break;
>> +     case MAX1586_R24_5k11:
>> +             max1586->min_uV = 750000;
>> +             max1586->max_uV = 1600000;
>> +             break;
>> +     case MAX1586_R24_7k5:
>> +             max1586->min_uV = 780000;
>> +             max1586->max_uV = 1650000;
>> +             break;
>> +     }
> I would like that changed to something more or less like :
> if (pdata->r24 == 0)
>        gain = 1;
> else
>        gain = 1 + R24/185.5;

No floating point in the kernel, so let's say I do some ugly rounding:

        e6 = 1000000;
        gain = e6 + e6*r24/100000 + e6*r24/185500;
        min_uV = ((MAX1586_V3_MIN_UV/1000 * gain) / e6) / 10 * 10000;
        max_uV = ((MAX1586_V3_MAX_UV/1000 * gain) / e6 + 9) / 10 * 10000;

Then I get all the (nominal) values for R24 = 3220, 5110, 7500.
But of course not for values inbetween, which kind of defeats the
purpose of calculating it in the first place...

If I don't round up the max values, I get something like 1.548V
instead of the nominal 1.55V for max, so requests to set the voltage
to 1.55V will fail even though it's in the 1.5% tolerance.

>>       ret = -EINVAL;
>>       for (i = 0; i < pdata->num_subdevs && i <= MAX1586_V6; i++) {
>> @@ -182,7 +218,7 @@ static int max1586_pmic_probe(struct i2c_client *client,
>>               }
>>               rdev[i] = regulator_register(&max1586_reg[id], &client->dev,
>>                                            pdata->subdevs[i].platform_data,
>> -                                          client);
>> +                                          max1586);
>>               if (IS_ERR(rdev[i])) {
>>                       ret = PTR_ERR(rdev[i]);
>>                       dev_err(&client->dev, "failed to register %s\n",
>> @@ -198,7 +234,10 @@ static int max1586_pmic_probe(struct i2c_client *client,
>>  err:
>>       while (--i >= 0)
>>               regulator_unregister(rdev[i]);
>> +     kfree(max1586);
>> +out_unmap:
>>       kfree(rdev);
>> +out:
>>       return ret;
>>  }
>>
>> diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
>> index 2056973..cdf2010 100644
>> --- a/include/linux/regulator/max1586.h
>> +++ b/include/linux/regulator/max1586.h
>> @@ -26,6 +26,10 @@
>>  #define MAX1586_V3 0
>>  #define MAX1586_V6 1
>>
>> +#define MAX1586_R24_3k32 3320
>> +#define MAX1586_R24_5k11 5110
>> +#define MAX1586_R24_7k5  7500
> This could be gone, as R24 will define the exact value of R24 in ohms.

I thought it would be good to have the values given in the datasheet
as a reference somewhere.

>> +
>>  /**
>>   * max1586_subdev_data - regulator data
>>   * @id: regulator Id (either MAX1586_V3 or MAX1586_V6)
>> @@ -43,10 +47,13 @@ struct max1586_subdev_data {
>>   * @num_subdevs: number of regultors used (may be 1 or 2)
>>   * @subdevs: regulator used
>>   *           At most, there will be a regulator for V3 and one for V6 voltages.
>> + * @r24: external regulator to increase V3 range
> Rather :
> + * @r24: external resistor value to increase V3 range (in ohms), or 0 if none

As Mark noted, it is probably a good idea not to allow a value of 0 here.

regards
Philipp

Re: [PATCH] regulator/max1586: support increased V3 voltage range

[Robert Jarzmik]

pHilipp Zabel <philipp.zabel@gmail.com> writes:

>>> -#define MAX1586_V3_MIN_UV   700000
>>> -#define MAX1586_V3_MAX_UV  1475000
>>> -#define MAX1586_V3_STEP_UV   25000
>> These 3 should be kept I think, see below.
>
> The nice thing about the switch statement below was that I just had to
> take the nominal values from the datasheet and not think about the
> actual mechanism at all.
I know. But board with funky resistor values can come (or worst, a voltage
regulator providing reference voltage to pin FB3 : watch the "chicken and egg"
problem comming :))

>>>  /*
>>>   * V3 voltage
>>>   * On I2C bus, sending a "x" byte to the max1586 means :
>>> - *   set V3 to 0.700V + (x & 0x1f) * 0.025V
>>> + *   set V3 to 0.700V + (x & 0x1f) * 0.025V,
>>> + *   set V3 to 0.730V + (x & 0x1f) * 0.026V,
>>> + *   set V3 to 0.750V + (x & 0x1f) * 0.027V or
>>> + *   set V3 to 0.780V + (x & 0x1f) * 0.028V
>>> + * depending on the external resistance R24.
>> That's not really the full truth.
>
> Hm, right. My MAX1586A/MAX1586B/MAX1587A data sheet only has  this
> information in (Figure 9. Adding R24 and R25 to Increase Core Voltage
> Programming Range):
>
> R24 = 3.32kOhm, V3: 0.73V TO 1.55V, 26mV/STEP
> R24 = 5.11kOhm, V3: 0.75V TO 1.60V, 27mV/STEP
> R24 = 7.5kkOhm, V3: 0.78V TO 1.65V, 28mV/STEP
>
> With R25 = 100kOhm. There's no mention of 185.5(kOhm?) anywhere.
That's an internal resistor of the MAX158x, connected from FB3 pin to ground.

>> The real V3 is (0.700V + (x & 0x1f) * 0.025V) * gain, where :
>>  - if R24 is not connected, gain = 1
>>  - if R24 and R25 are connected, gain = 1 + R24/R25 + R24/185.5
>
> Ok, so for example:
> R24 = 3.32kOhm --> gain = 1.051098 --> 735768uV ... 1550369uV
> But the nominal values from the datasheet for this R24 resistor are
> 730mV and 1550mV. Should we trust this calculation over the nominal
> values?
I'd rather trust the calculation, as it seems related to the electronics. From
what I see, R24 forms a voltage divisor with the resultant resistor formed by
R25 and 185.5 kOhms in parallel formation.

>
>>   Here, it should be assumed that R25 >> 10k.ohm, which means that R24/R25
>>   becomes meaningless compared to R24/185.5.
>>   Therefore, the formula becomes:
>>     gain = 1 + R24/185.5
>
> That is not true. For this formula to make sense, 185.5 has to be
> given in kΩ, so R24/R25 is actually the bigger contribution.
Ah, specification british touch. The formula was written with R24/185,500, and I
took the comma for ... the decimal separator, while it was 185.5 k.ohms as you
say. You're right.

And so, the formula has to take into accound R25 as well, and become :
  gain = 1 + R24/185500 + R24/R25

>> For reference, R24 and R25 are described in Max1586 datasheet, in figure7
>> (extending the maximum core voltage range).
>>
>> I'd like that put in the comment please.
>
> Yes, it would be better to document this.

>>>
>>>  static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>>>  {
>>> -     struct i2c_client *client = rdev_get_drvdata(rdev);
>>> +     struct max1586_data *max1586 = rdev_get_drvdata(rdev);
>>> +     struct i2c_client *client = max1586->client;
>>> +     unsigned range_uV = max1586->max_uV - max1586->min_uV;
>>>       unsigned selector;
>>>       u8 v3_prog;
>>>
>>> -     if (min_uV < MAX1586_V3_MIN_UV || min_uV > MAX1586_V3_MAX_UV)
>>> -             return -EINVAL;
>>> -     if (max_uV < MAX1586_V3_MIN_UV || max_uV > MAX1586_V3_MAX_UV)
>>> +     if (min_uV > max1586->max_uV || max_uV < max1586->min_uV)
>>>               return -EINVAL;
>>> +     if (min_uV < max1586->min_uV)
>>> +             min_uV = max1586->min_uV;
>>>
>>> -     selector = (min_uV - MAX1586_V3_MIN_UV) / MAX1586_V3_STEP_UV;
>>> -     if (max1586_v3_calc_voltage(selector) > max_uV)
>>> +     selector = (min_uV - max1586->min_uV) * 31 / range_uV;
>>                                                \-> ???
>> Why 31 ? Could I have a define here, and probably that define would be a value
>> of 32 (as there are 32 different steps, not 31).
>
> We have 32 steps, 0 to 31. The difference between the smallest and
> largest value is still 31. I should have used MAX1586_V3_MAX_VSEL here
> to avoid confusion.
OK, that's right.

>>> @@ -71,9 +84,11 @@ static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
>>>
>>>  static int max1586_v3_list(struct regulator_dev *rdev, unsigned selector)
>>>  {
>>> +     struct max1586_data *max1586 = rdev_get_drvdata(rdev);
>>> +
>>>       if (selector > MAX1586_V3_MAX_VSEL)
>>>               return -EINVAL;
>>> -     return max1586_v3_calc_voltage(selector);
>>> +     return max1586_v3_calc_voltage(max1586, selector);
>>>  }
>>>
>>>  /*
>>> @@ -164,12 +179,33 @@ static int max1586_pmic_probe(struct i2c_client *client,
>>>  {
>>>       struct regulator_dev **rdev;
>>>       struct max1586_platform_data *pdata = client->dev.platform_data;
>>> -     int i, id, ret = 0;
>>> +     struct max1586_data *max1586;
>>> +     int i, id, ret = -ENOMEM;
>>>
>>>       rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>>>                      GFP_KERNEL);
>>>       if (!rdev)
>>> -             return -ENOMEM;
>>> +             goto out;
>>> +
>>> +     max1586 = kzalloc(sizeof(struct max1586_data *), GFP_KERNEL);
>>> +     if (!max1586)
>>> +             goto out_unmap;
>>> +
>>> +     max1586->client = client;
>>
>>> +     switch (pdata->r24) {
>>> +     case MAX1586_R24_3k32:
>>> +             max1586->min_uV = 730000;
>>> +             max1586->max_uV = 1550000;
>>> +             break;
>>> +     case MAX1586_R24_5k11:
>>> +             max1586->min_uV = 750000;
>>> +             max1586->max_uV = 1600000;
>>> +             break;
>>> +     case MAX1586_R24_7k5:
>>> +             max1586->min_uV = 780000;
>>> +             max1586->max_uV = 1650000;
>>> +             break;
>>> +     }

>> I would like that changed to something more or less like :
>> if (pdata->r24 == 0)
>>        gain = 1;
>> else
>>        gain = 1 + R24/185.5;
What do you think of putting directly the gain in the pdata structure ?
Something like pdata->gain_e6, which is equal to :
  1000000 * (1 + R24/185000 + R24/R25).

That way, whatever the electronics do (say someone crazy puts a voltage
regulator providing voltage reference for FB3 pin), the system still works.

>
> No floating point in the kernel, so let's say I do some ugly rounding:
>
>         e6 = 1000000;
>         gain = e6 + e6*r24/100000 + e6*r24/185500;
>         min_uV = ((MAX1586_V3_MIN_UV/1000 * gain) / e6) / 10 * 10000;
>         max_uV = ((MAX1586_V3_MAX_UV/1000 * gain) / e6 + 9) / 10 * 10000;
>
> Then I get all the (nominal) values for R24 = 3220, 5110, 7500.
> But of course not for values inbetween, which kind of defeats the
> purpose of calculating it in the first place...
OK. Maybe would be easier if gain was in pdata ?

> If I don't round up the max values, I get something like 1.548V
> instead of the nominal 1.55V for max, so requests to set the voltage
> to 1.55V will fail even though it's in the 1.5% tolerance.


>>> diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
>>> index 2056973..cdf2010 100644
>>> --- a/include/linux/regulator/max1586.h
>>> +++ b/include/linux/regulator/max1586.h
>>> @@ -26,6 +26,10 @@
>>>  #define MAX1586_V3 0
>>>  #define MAX1586_V6 1
>>>
>>> +#define MAX1586_R24_3k32 3320
>>> +#define MAX1586_R24_5k11 5110
>>> +#define MAX1586_R24_7k5  7500
>> This could be gone, as R24 will define the exact value of R24 in ohms.
>
> I thought it would be good to have the values given in the datasheet
> as a reference somewhere.
True. What would you think of defining something like :
#define MAX1586_GAIN_R24_3k32 <your_computed_value>
and so on ?

>>>  /**
>>>   * max1586_subdev_data - regulator data
>>>   * @id: regulator Id (either MAX1586_V3 or MAX1586_V6)
>>> @@ -43,10 +47,13 @@ struct max1586_subdev_data {
>>>   * @num_subdevs: number of regultors used (may be 1 or 2)
>>>   * @subdevs: regulator used
>>>   *           At most, there will be a regulator for V3 and one for V6 voltages.
>>> + * @r24: external regulator to increase V3 range
>> Rather :
>> + * @r24: external resistor value to increase V3 range (in ohms), or 0 if none
>
> As Mark noted, it is probably a good idea not to allow a value of 0 here.
And replacing with a gain ?

Cheers.

--
Robert

(no subject)

[Philipp Zabel]

Ok, I tried to include all proposed changes in 1/3. As you can see, for
R24=5.11kOhm and 7.5kOhm the maximum V3 voltage doesn't reach the nominal
value from the data sheet, but as I only need the nominal 1.55V for
R24=3.32kOhm to make pxa2xx-cpufreq happy, this is good enough for me.

Patch 2/3, 3/3 depend on this and should go into Eric's pending tree once
this is settled.

regards
Philipp

[PATCH 1/3] regulator/max1586: support increased V3 voltage range

[Philipp Zabel]

The V3 regulator can be configured with an external resistor
connected to the feedback pin (R24 in the data sheet) to
increase the voltage range.

For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/regulator/max1586.c       |   71 +++++++++++++++++++++++++++----------
 include/linux/regulator/max1586.h |   11 ++++++
 2 files changed, 63 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index bbbb55f..92799f4 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -29,7 +29,6 @@
 
 #define MAX1586_V3_MIN_UV   700000
 #define MAX1586_V3_MAX_UV  1475000
-#define MAX1586_V3_STEP_UV   25000
 
 #define MAX1586_V6_MIN_UV        0
 #define MAX1586_V6_MAX_UV  3000000
@@ -37,33 +36,52 @@
 #define I2C_V3_SELECT (0 << 5)
 #define I2C_V6_SELECT (1 << 5)
 
+struct max1586_data {
+	struct i2c_client *client;
+
+	/* min/max V3 voltage */
+	int min_uV;
+	int max_uV;
+
+	struct regulator_dev *rdev[0];
+};
+
 /*
  * V3 voltage
  * On I2C bus, sending a "x" byte to the max1586 means :
  *   set V3 to 0.700V + (x & 0x1f) * 0.025V
+ * This voltage can be increased by external resistors
+ * R24 and R25=100kOhm as described in the data sheet.
+ * The gain is approximately: 1 + R24/R25 + R24/185.5kOhm
  */
-static int max1586_v3_calc_voltage(unsigned selector)
+static int max1586_v3_calc_voltage(struct max1586_data *max1586,
+	unsigned selector)
 {
-	return MAX1586_V3_MIN_UV + (MAX1586_V3_STEP_UV * selector);
+	unsigned range_uV = max1586->max_uV - max1586->min_uV;
+
+	return max1586->min_uV + (selector * range_uV / MAX1586_V3_MAX_VSEL);
 }
 
 static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
 {
-	struct i2c_client *client = rdev_get_drvdata(rdev);
+	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
+	struct i2c_client *client = max1586->client;
+	unsigned range_uV = max1586->max_uV - max1586->min_uV;
 	unsigned selector;
 	u8 v3_prog;
 
-	if (min_uV < MAX1586_V3_MIN_UV || min_uV > MAX1586_V3_MAX_UV)
-		return -EINVAL;
-	if (max_uV < MAX1586_V3_MIN_UV || max_uV > MAX1586_V3_MAX_UV)
+	if (min_uV > max1586->max_uV || max_uV < max1586->min_uV)
 		return -EINVAL;
+	if (min_uV < max1586->min_uV)
+		min_uV = max1586->min_uV;
 
-	selector = (min_uV - MAX1586_V3_MIN_UV) / MAX1586_V3_STEP_UV;
-	if (max1586_v3_calc_voltage(selector) > max_uV)
+	selector = ((min_uV - max1586->min_uV) * MAX1586_V3_MAX_VSEL +
+			range_uV - 1) / range_uV;
+	if (max1586_v3_calc_voltage(max1586, selector) > max_uV)
 		return -EINVAL;
 
 	dev_dbg(&client->dev, "changing voltage v3 to %dmv\n",
-		max1586_v3_calc_voltage(selector) / 1000);
+		max1586_v3_calc_voltage(max1586, selector) / 1000);
 
 	v3_prog = I2C_V3_SELECT | (u8) selector;
 	return i2c_smbus_write_byte(client, v3_prog);
@@ -71,9 +89,11 @@ static int max1586_v3_set(struct regulator_dev *rdev, int min_uV, int max_uV)
 
 static int max1586_v3_list(struct regulator_dev *rdev, unsigned selector)
 {
+	struct max1586_data *max1586 = rdev_get_drvdata(rdev);
+
 	if (selector > MAX1586_V3_MAX_VSEL)
 		return -EINVAL;
-	return max1586_v3_calc_voltage(selector);
+	return max1586_v3_calc_voltage(max1586, selector);
 }
 
 /*
@@ -164,14 +184,25 @@ static int max1586_pmic_probe(struct i2c_client *client,
 {
 	struct regulator_dev **rdev;
 	struct max1586_platform_data *pdata = client->dev.platform_data;
-	int i, id, ret = 0;
+	struct max1586_data *max1586;
+	int i, id, ret = -ENOMEM;
+
+	max1586 = kzalloc(sizeof(struct max1586_data) +
+			sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
+			GFP_KERNEL);
+	if (!max1586)
+		goto out;
 
-	rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
-		       GFP_KERNEL);
-	if (!rdev)
-		return -ENOMEM;
+	max1586->client = client;
 
-	ret = -EINVAL;
+	if (!pdata->v3_gain) {
+		ret = -EINVAL;
+		goto out_unmap;
+	}
+	max1586->min_uV = MAX1586_V3_MIN_UV * pdata->v3_gain / 1000000;
+	max1586->max_uV = MAX1586_V3_MAX_UV * pdata->v3_gain / 1000000;
+
+	rdev = max1586->rdev;
 	for (i = 0; i < pdata->num_subdevs && i <= MAX1586_V6; i++) {
 		id = pdata->subdevs[i].id;
 		if (!pdata->subdevs[i].platform_data)
@@ -182,7 +213,7 @@ static int max1586_pmic_probe(struct i2c_client *client,
 		}
 		rdev[i] = regulator_register(&max1586_reg[id], &client->dev,
 					     pdata->subdevs[i].platform_data,
-					     client);
+					     max1586);
 		if (IS_ERR(rdev[i])) {
 			ret = PTR_ERR(rdev[i]);
 			dev_err(&client->dev, "failed to register %s\n",
@@ -198,7 +229,9 @@ static int max1586_pmic_probe(struct i2c_client *client,
 err:
 	while (--i >= 0)
 		regulator_unregister(rdev[i]);
-	kfree(rdev);
+out_unmap:
+	kfree(max1586);
+out:
 	return ret;
 }
 
diff --git a/include/linux/regulator/max1586.h b/include/linux/regulator/max1586.h
index 2056973..4456319 100644
--- a/include/linux/regulator/max1586.h
+++ b/include/linux/regulator/max1586.h
@@ -26,6 +26,12 @@
 #define MAX1586_V3 0
 #define MAX1586_V6 1
 
+/* precalculated values for v3_gain */
+#define MAX1586_GAIN_NO_R24   1000000  /* 700000 .. 1475000 mV */
+#define MAX1586_GAIN_R24_3k32 1051098  /* 735768 .. 1550369 mV */
+#define MAX1586_GAIN_R24_5k11 1078648  /* 755053 .. 1591005 mV */
+#define MAX1586_GAIN_R24_7k5  1115432  /* 780802 .. 1645262 mV */
+
 /**
  * max1586_subdev_data - regulator data
  * @id: regulator Id (either MAX1586_V3 or MAX1586_V6)
@@ -43,10 +49,15 @@ struct max1586_subdev_data {
  * @num_subdevs: number of regultors used (may be 1 or 2)
  * @subdevs: regulator used
  *           At most, there will be a regulator for V3 and one for V6 voltages.
+ * @v3_gain: gain on the V3 voltage output multiplied by 1e6.
+ *           This can be calculated as ((1 + R24/R25 + R24/185.5kOhm) * 1e6)
+ *           for an external resistor configuration as described in the
+ *           data sheet (R25=100kOhm).
  */
 struct max1586_platform_data {
 	int num_subdevs;
 	struct max1586_subdev_data *subdevs;
+	int v3_gain;
 };
 
 #endif
-- 
1.6.3.1

Re: [PATCH 1/3] regulator/max1586: support increased V3 voltage range

[Mark Brown]

On Thu, May 28, 2009 at 07:15:16AM +0200, Philipp Zabel wrote:
> The V3 regulator can be configured with an external resistor
> connected to the feedback pin (R24 in the data sheet) to
> increase the voltage range.
> 
> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
> 
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>

Looks good.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[PATCH] regulator/max1586: fix V3 gain calculation integer overflow

[Philipp Zabel]

On Thu, May 28, 2009 at 10:59 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, May 28, 2009 at 07:15:16AM +0200, Philipp Zabel wrote:
>> The V3 regulator can be configured with an external resistor
>> connected to the feedback pin (R24 in the data sheet) to
>> increase the voltage range.
>>
>> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
>> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
>
> Looks good.
>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Thanks, but it turns out I hit a 32 bit integer overflow in
the gain calculation. I'd like to mend that with the following
patch. Now max_uV could be increased up to 4.294 V, enough to
charge LiPo cells.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/regulator/max1586.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max1586.c b/drivers/regulator/max1586.c
index 92799f4..2c082d3 100644
--- a/drivers/regulator/max1586.c
+++ b/drivers/regulator/max1586.c
@@ -40,8 +40,8 @@ struct max1586_data {
 	struct i2c_client *client;
 
 	/* min/max V3 voltage */
-	int min_uV;
-	int max_uV;
+	unsigned int min_uV;
+	unsigned int max_uV;
 
 	struct regulator_dev *rdev[0];
 };
@@ -199,8 +199,8 @@ static int max1586_pmic_probe(struct i2c_client *client,
 		ret = -EINVAL;
 		goto out_unmap;
 	}
-	max1586->min_uV = MAX1586_V3_MIN_UV * pdata->v3_gain / 1000000;
-	max1586->max_uV = MAX1586_V3_MAX_UV * pdata->v3_gain / 1000000;
+	max1586->min_uV = MAX1586_V3_MIN_UV / 1000 * pdata->v3_gain / 1000;
+	max1586->max_uV = MAX1586_V3_MAX_UV / 1000 * pdata->v3_gain / 1000;
 
 	rdev = max1586->rdev;
 	for (i = 0; i < pdata->num_subdevs && i <= MAX1586_V6; i++) {
-- 
1.6.3.1

Re: [PATCH] regulator/max1586: fix V3 gain calculation integer overflow

[Robert Jarzmik]

Philipp Zabel <philipp.zabel@gmail.com> writes:

> On Thu, May 28, 2009 at 10:59 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
>> On Thu, May 28, 2009 at 07:15:16AM +0200, Philipp Zabel wrote:
>>> The V3 regulator can be configured with an external resistor
>>> connected to the feedback pin (R24 in the data sheet) to
>>> increase the voltage range.
>>>
>>> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
>>> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
>>>
>>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
>>
>> Looks good.
>>
>> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>
> Thanks, but it turns out I hit a 32 bit integer overflow in
> the gain calculation. I'd like to mend that with the following
> patch. Now max_uV could be increased up to 4.294 V, enough to
> charge LiPo cells.
>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
OK.

With that one melt into the previous one, have my :
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Would you submit that to Liam or Mark for queuing into the regulator patch queue ?

Cheers.

--
Robert

Re: [PATCH] regulator/max1586: fix V3 gain calculation integer overflow

[Liam Girdwood]

On Thu, 2009-05-28 at 22:36 +0200, Robert Jarzmik wrote:
> Philipp Zabel <philipp.zabel@gmail.com> writes:
> 
> > On Thu, May 28, 2009 at 10:59 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> >> On Thu, May 28, 2009 at 07:15:16AM +0200, Philipp Zabel wrote:
> >>> The V3 regulator can be configured with an external resistor
> >>> connected to the feedback pin (R24 in the data sheet) to
> >>> increase the voltage range.
> >>>
> >>> For example, hx4700 has R24 = 3.32 kOhm to achieve a maximum
> >>> V3 voltage of 1.55 V which is needed for 624 MHz CPU frequency.
> >>>
> >>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> >>
> >> Looks good.
> >>
> >> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> >
> > Thanks, but it turns out I hit a 32 bit integer overflow in
> > the gain calculation. I'd like to mend that with the following
> > patch. Now max_uV could be increased up to 4.294 V, enough to
> > charge LiPo cells.
> >
> > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> OK.
> 
> With that one melt into the previous one, have my :
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
> 
> Would you submit that to Liam or Mark for queuing into the regulator patch queue ?
> 

Both patches now applied.

Thanks

Liam

Re: [PATCH 1/3] regulator/max1586: support increased V3 voltage range

[Robert Jarzmik]

Philipp Zabel <philipp.zabel@gmail.com> writes:

<snip>
> @@ -164,14 +184,25 @@ static int max1586_pmic_probe(struct i2c_client *client,
>  {
>  	struct regulator_dev **rdev;
>  	struct max1586_platform_data *pdata = client->dev.platform_data;
> -	int i, id, ret = 0;
> +	struct max1586_data *max1586;
> +	int i, id, ret = -ENOMEM;
> +
> +	max1586 = kzalloc(sizeof(struct max1586_data) +
> +			sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
> +			GFP_KERNEL);
> +	if (!max1586)
> +		goto out;
>  
> -	rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
> -		       GFP_KERNEL);
> -	if (!rdev)
> -		return -ENOMEM;
> +	max1586->client = client;
>  
> -	ret = -EINVAL;
> +	if (!pdata->v3_gain) {
> +		ret = -EINVAL;
> +		goto out_unmap;
> +	}

> +	max1586->min_uV = MAX1586_V3_MIN_UV * pdata->v3_gain / 1000000;
> +	max1586->max_uV = MAX1586_V3_MAX_UV * pdata->v3_gain / 1000000;
My last comment : you'll overflow an integer capacity here, and will get
min_uV=max_uV=0.

Please replace by :
       max1586->min_uV = MAX1586_V3_MIN_UV / 1000 * (pdata->v3_gain / 1000);
       max1586->max_uV = MAX1586_V3_MAX_UV / 1000 * (pdata->v3_gain / 1000);

And then add my :
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [PATCH 1/3] regulator/max1586: support increased V3 voltage range

[pHilipp Zabel]

On Thu, May 28, 2009 at 8:58 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Philipp Zabel <philipp.zabel@gmail.com> writes:
>
> <snip>
>> @@ -164,14 +184,25 @@ static int max1586_pmic_probe(struct i2c_client *client,
>>  {
>>       struct regulator_dev **rdev;
>>       struct max1586_platform_data *pdata = client->dev.platform_data;
>> -     int i, id, ret = 0;
>> +     struct max1586_data *max1586;
>> +     int i, id, ret = -ENOMEM;
>> +
>> +     max1586 = kzalloc(sizeof(struct max1586_data) +
>> +                     sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>> +                     GFP_KERNEL);
>> +     if (!max1586)
>> +             goto out;
>>
>> -     rdev = kzalloc(sizeof(struct regulator_dev *) * (MAX1586_V6 + 1),
>> -                    GFP_KERNEL);
>> -     if (!rdev)
>> -             return -ENOMEM;
>> +     max1586->client = client;
>>
>> -     ret = -EINVAL;
>> +     if (!pdata->v3_gain) {
>> +             ret = -EINVAL;
>> +             goto out_unmap;
>> +     }
>
>> +     max1586->min_uV = MAX1586_V3_MIN_UV * pdata->v3_gain / 1000000;
>> +     max1586->max_uV = MAX1586_V3_MAX_UV * pdata->v3_gain / 1000000;
> My last comment : you'll overflow an integer capacity here, and will get
> min_uV=max_uV=0.
>
> Please replace by :
>       max1586->min_uV = MAX1586_V3_MIN_UV / 1000 * (pdata->v3_gain / 1000);
>       max1586->max_uV = MAX1586_V3_MAX_UV / 1000 * (pdata->v3_gain / 1000);
>
> And then add my :
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Wow, thanks. I literally sent a patch to that effect a few seconds
ago. Would you be ok with that one, too? I'd like not to lose too much
resolution in the v3_gain parameter.

regards
Philipp

Re: [PATCH 1/3] regulator/max1586: support increased V3 voltage range

[Robert Jarzmik]

pHilipp Zabel <philipp.zabel@gmail.com> writes:

> Wow, thanks. I literally sent a patch to that effect a few seconds
> ago. Would you be ok with that one, too? I'd like not to lose too much
> resolution in the v3_gain parameter.
Yep. Acked it already.

Cheers.

--
Robert

[PATCH 4/5] pxa/mioa701: add V3 gain configuration for Maxim 1586 voltage regulator

[Philipp Zabel]

Maxim 1586 V3 gain can be configured by external resistors
(R24, R25) connected to the FB3 pin. To avoid accidental
overvoltage, it now requires this gain to be configured
explicitly.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 arch/arm/mach-pxa/mioa701.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
index 54c91db..d805218 100644
--- a/arch/arm/mach-pxa/mioa701.c
+++ b/arch/arm/mach-pxa/mioa701.c
@@ -746,6 +746,7 @@ static struct max1586_subdev_data max1586_subdevs[] = {
 static struct max1586_platform_data max1586_info = {
 	.subdevs = max1586_subdevs,
 	.num_subdevs = ARRAY_SIZE(max1586_subdevs),
+	.v3_gain = MAX1586_GAIN_NO_R24, /* 700..1475 mV */
 };
 
 /*
-- 
1.6.3.1

Re: [PATCH 4/5] pxa/mioa701: add V3 gain configuration for Maxim 1586 voltage regulator

[pHilipp Zabel]

That should have been "[PATCH 2/3] pxa/mioa701: ..."

On Thu, May 28, 2009 at 7:15 AM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> Maxim 1586 V3 gain can be configured by external resistors
> (R24, R25) connected to the FB3 pin. To avoid accidental
> overvoltage, it now requires this gain to be configured
> explicitly.
>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  arch/arm/mach-pxa/mioa701.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/mioa701.c b/arch/arm/mach-pxa/mioa701.c
> index 54c91db..d805218 100644
> --- a/arch/arm/mach-pxa/mioa701.c
> +++ b/arch/arm/mach-pxa/mioa701.c
> @@ -746,6 +746,7 @@ static struct max1586_subdev_data max1586_subdevs[] = {
>  static struct max1586_platform_data max1586_info = {
>        .subdevs = max1586_subdevs,
>        .num_subdevs = ARRAY_SIZE(max1586_subdevs),
> +       .v3_gain = MAX1586_GAIN_NO_R24, /* 700..1475 mV */
>  };
>
>  /*
> --
> 1.6.3.1
>
>

Re: [PATCH 4/5] pxa/mioa701: add V3 gain configuration for Maxim 1586 voltage regulator

[Robert Jarzmik]

Philipp Zabel <philipp.zabel@gmail.com> writes:

> Maxim 1586 V3 gain can be configured by external resistors
> (R24, R25) connected to the FB3 pin. To avoid accidental
> overvoltage, it now requires this gain to be configured
> explicitly.
>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert

Re: [PATCH 4/5] pxa/mioa701: add V3 gain configuration for Maxim 1586 voltage regulator

[Eric Miao]

On Fri, May 29, 2009 at 2:22 PM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Philipp Zabel <philipp.zabel@gmail.com> writes:
>
>> Maxim 1586 V3 gain can be configured by external resistors
>> (R24, R25) connected to the FB3 pin. To avoid accidental
>> overvoltage, it now requires this gain to be configured
>> explicitly.
>>
>> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
>

Applied to 'pending'.

[PATCH 3/3] pxa/hx4700: add Maxim 1587A voltage regulator

[Philipp Zabel]

On this board, the PXA270 CPU voltage VCC_CORE is provided
by a Maxim 1587A voltage regulator configured to provide
1.55 V maximum voltage for 624 MHz operation.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 arch/arm/mach-pxa/hx4700.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-pxa/hx4700.c b/arch/arm/mach-pxa/hx4700.c
index 0f65680..5bc1e2b 100644
--- a/arch/arm/mach-pxa/hx4700.c
+++ b/arch/arm/mach-pxa/hx4700.c
@@ -30,6 +30,7 @@
 #include <linux/pwm_backlight.h>
 #include <linux/regulator/bq24022.h>
 #include <linux/regulator/machine.h>
+#include <linux/regulator/max1586.h>
 #include <linux/spi/ads7846.h>
 #include <linux/spi/spi.h>
 #include <linux/usb/gpio_vbus.h>
@@ -775,6 +776,45 @@ static struct platform_device strataflash = {
 };
 
 /*
+ * Maxim MAX1587A on PI2C
+ */
+
+static struct regulator_consumer_supply max1587a_consumer = {
+	.supply = "vcc_core",
+};
+
+static struct regulator_init_data max1587a_v3_info = {
+	.constraints = {
+		.name = "vcc_core range",
+		.min_uV =  900000,
+		.max_uV = 1705000,
+		.always_on = 1,
+		.valid_ops_mask = REGULATOR_CHANGE_VOLTAGE,
+	},
+	.num_consumer_supplies = 1,
+	.consumer_supplies     = &max1587a_consumer,
+};
+
+static struct max1586_subdev_data max1587a_subdev = {
+	.name = "vcc_core",
+	.id   = MAX1586_V3,
+	.platform_data = &max1587a_v3_info,
+};
+
+static struct max1586_platform_data max1587a_info = {
+	.num_subdevs = 1,
+	.subdevs     = &max1587a_subdev,
+	.v3_gain     = MAX1586_GAIN_R24_3k32, /* 730..1550 mV */
+};
+
+static struct i2c_board_info __initdata pi2c_board_info[] = {
+	{
+		I2C_BOARD_INFO("max1586", 0x14),
+		.platform_data = &max1587a_info,
+	},
+};
+
+/*
  * PCMCIA
  */
 
@@ -828,6 +868,7 @@ static void __init hx4700_init(void)
 	pxa_set_ficp_info(&ficp_info);
 	pxa27x_set_i2c_power_info(NULL);
 	pxa_set_i2c_info(NULL);
+	i2c_register_board_info(1, ARRAY_AND_SIZE(pi2c_board_info));
 	pxa2xx_set_spi_info(2, &pxa_ssp2_master_info);
 	spi_register_board_info(ARRAY_AND_SIZE(tsc2046_board_info));
 
-- 
1.6.3.1

Re: [PATCH 3/3] pxa/hx4700: add Maxim 1587A voltage regulator

[Eric Miao]

On Thu, May 28, 2009 at 1:15 PM, Philipp Zabel <philipp.zabel@gmail.com> wrote:
> On this board, the PXA270 CPU voltage VCC_CORE is provided
> by a Maxim 1587A voltage regulator configured to provide
> 1.55 V maximum voltage for 624 MHz operation.
>
> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>

Applied to 'pending'.