lkml.org 
[lkml]   [2009]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH] regulator/max1586: support increased V3 voltage range
From
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-05-27 01:43    [W:0.125 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site