On Tue, Jan 12, 2010 at 03:51:09AM -0500, Haojian Zhuang wrote:
> Enable Maxim max8649 regulator driver.
This seems basically fine but there's a few relatively minor issues
below, mostly coding style rather than anything serious.
> +static int max8649_list_voltage(struct regulator_dev *rdev, unsigned index)
> +{
> + return MAX8649_DCDC_VMIN + index * MAX8649_DCDC_STEP;
> +}
Brackets here would help legibility even if not strictly required.
> + data= (min_uV - MAX8649_DCDC_VMIN + MAX8649_DCDC_STEP - 1)
> + / MAX8649_DCDC_STEP;
Should be "data ="
> +static struct regulator_desc dcdc_desc = {
> + .name = "DCDC",
MAX8649 might be a better name but it doesn't really make any odds.
> + .ops = &max8649_dcdc_ops,
> + .type = REGULATOR_VOLTAGE,
> + .n_voltages = 1 << 7,
Use the max index value you have above?
> + info->vol_reg = (info->mode == 0) ? MAX8649_MODE0
> + : ((info->mode == 1) ? MAX8649_MODE1
> + : ((info->mode == 2) ? MAX8649_MODE2
> + : MAX8649_MODE3));
This should really be a switch statement for legibility. In general the
ternery operator should be used very sparingly, and if you've got more
than one of them it's not a good sign.
> + /* enable/disable auto enter power save mode */
> + info->powersave = pdata->powersave;
> + data = (info->powersave) ? 0 : MAX8649_POWER_SAVE;
> + max8649_set_bits(info->i2c, info->vol_reg, MAX8649_POWER_SAVE, data);
I'm not sure what this power save mode is but I suspect it'd map well
onto the regulator_set_mode() API - normal mode for power saving, fast
mode for power saving disabled.
> + if (pdata->ramp_timing) {
> + info->ramp_timing = pdata->ramp_timing;
> + max8649_set_bits(info->i2c, MAX8649_RAMP, MAX8649_RAMP_MASK,
> + info->ramp_timing << 5);
> + }
You might want to implement the new enable_time() API for this.
> + pr_info("Max8649 regulator device is detected.\n");
This should be at most debug level, and should be dev_() to distinguish
between multiple devices.
On Tue, Jan 12, 2010 at 6:51 AM, Mark Brown
<[email protected]> wrote:
> On Tue, Jan 12, 2010 at 03:51:09AM -0500, Haojian Zhuang wrote:
>
>> Enable Maxim max8649 regulator driver.
>
> This seems basically fine but there's a few relatively minor issues
> below, mostly coding style rather than anything serious.
>
>
>> + ? ? if (pdata->ramp_timing) {
>> + ? ? ? ? ? ? info->ramp_timing = pdata->ramp_timing;
>> + ? ? ? ? ? ? max8649_set_bits(info->i2c, MAX8649_RAMP, MAX8649_RAMP_MASK,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?info->ramp_timing << 5);
>> + ? ? }
>
> You might want to implement the new enable_time() API for this.
>
This ramp timing is the time interval of each step on adjusting
voltage. I just want to control the timing in initialization.
enable_time() is only called before enabling regulator. And I don't
understand what would be done in enable_time().
Others are updated in this attached patch.
Thanks
Haojian
On Mon, Jan 25, 2010 at 06:01:41AM -0500, Haojian Zhuang wrote:
> On Tue, Jan 12, 2010 at 6:51 AM, Mark Brown
> >> + ? ? if (pdata->ramp_timing) {
> >> + ? ? ? ? ? ? info->ramp_timing = pdata->ramp_timing;
> >> + ? ? ? ? ? ? max8649_set_bits(info->i2c, MAX8649_RAMP, MAX8649_RAMP_MASK,
> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?info->ramp_timing << 5);
> >> + ? ? }
> > You might want to implement the new enable_time() API for this.
> This ramp timing is the time interval of each step on adjusting
> voltage. I just want to control the timing in initialization.
If this applies to any voltage change at all then rather than just power
on I need to finish off the stuff I've been sitting on for handling slew
times for voltage changes. If the regulator hasn't yet reached the
requested output when the consumer tries using it the consumer might get
broken.
If the ramp also gets applied when initially turning on the regulator
you'd still want to implement enable_time() for the same reason.
> enable_time() is only called before enabling regulator. And I don't
> understand what would be done in enable_time().
You'd return the amount of time taken to turn on the regulator and get
the output voltage stable in the current configuration. This will then
be used by the core to ensure that the consumer only tries to use the
regulator once it's fully enabled.
> +static int max8649_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> + struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + switch (mode) {
> + case REGULATOR_MODE_FAST:
> + case REGULATOR_MODE_NORMAL:
> + max8649_set_bits(info->i2c, info->vol_reg, MAX8649_FORCE_PWM,
> + MAX8649_FORCE_PWM);
> + break;
Are you sure about this? I'd expect to see force PWM used only for
_FAST, for _NORMAL pulse skipping is usually desired behaviour.
> + case REGULATOR_MODE_IDLE:
> + case REGULATOR_MODE_STANDBY:
> + max8649_set_bits(info->i2c, info->vol_reg,
> + MAX8649_FORCE_PWM, 0);
I'd just leave these unimplemented (there's no need to support all
modes) and make sure that this ties in with...
> +static unsigned int max8649_get_mode(struct regulator_dev *rdev)
> +{
> + struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
> + int ret;
> +
> + ret = max8649_reg_read(info->i2c, info->vol_reg);
> + if (ret & MAX8649_FORCE_PWM)
> + return REGULATOR_MODE_NORMAL;
> + return REGULATOR_MODE_IDLE;
...this.
On Mon, Jan 25, 2010 at 8:56 AM, Mark Brown
<[email protected]> wrote:
> On Mon, Jan 25, 2010 at 06:01:41AM -0500, Haojian Zhuang wrote:
>> On Tue, Jan 12, 2010 at 6:51 AM, Mark Brown
>
>> >> + ? ? if (pdata->ramp_timing) {
>> >> + ? ? ? ? ? ? info->ramp_timing = pdata->ramp_timing;
>> >> + ? ? ? ? ? ? max8649_set_bits(info->i2c, MAX8649_RAMP, MAX8649_RAMP_MASK,
>> >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?info->ramp_timing << 5);
>> >> + ? ? }
>
>> > You might want to implement the new enable_time() API for this.
>
>> This ramp timing is the time interval of each step on adjusting
>> voltage. I just want to control the timing in initialization.
>
> If this applies to any voltage change at all then rather than just power
> on I need to finish off the stuff I've been sitting on for handling slew
> times for voltage changes. ?If the regulator hasn't yet reached the
> requested output when the consumer tries using it the consumer might get
> broken.
>
> If the ramp also gets applied when initially turning on the regulator
> you'd still want to implement enable_time() for the same reason.
>
>> enable_time() is only called before enabling regulator. And I don't
>> understand what would be done in enable_time().
>
> You'd return the amount of time taken to turn on the regulator and get
> the output voltage stable in the current configuration. ?This will then
> be used by the core to ensure that the consumer only tries to use the
> regulator once it's fully enabled.
>
>> +static int max8649_set_mode(struct regulator_dev *rdev, unsigned int mode)
>> +{
>> + ? ? struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
>> +
>> + ? ? switch (mode) {
>> + ? ? case REGULATOR_MODE_FAST:
>> + ? ? case REGULATOR_MODE_NORMAL:
>> + ? ? ? ? ? ? max8649_set_bits(info->i2c, info->vol_reg, MAX8649_FORCE_PWM,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MAX8649_FORCE_PWM);
>> + ? ? ? ? ? ? break;
>
> Are you sure about this? ?I'd expect to see force PWM used only for
> _FAST, for _NORMAL pulse skipping is usually desired behaviour.
>
>> + ? ? case REGULATOR_MODE_IDLE:
>> + ? ? case REGULATOR_MODE_STANDBY:
>> + ? ? ? ? ? ? max8649_set_bits(info->i2c, info->vol_reg,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?MAX8649_FORCE_PWM, 0);
>
> I'd just leave these unimplemented (there's no need to support all
> modes) and make sure that this ties in with...
>
>> +static unsigned int max8649_get_mode(struct regulator_dev *rdev)
>> +{
>> + ? ? struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
>> + ? ? int ret;
>> +
>> + ? ? ret = max8649_reg_read(info->i2c, info->vol_reg);
>> + ? ? if (ret & MAX8649_FORCE_PWM)
>> + ? ? ? ? ? ? return REGULATOR_MODE_NORMAL;
>> + ? ? return REGULATOR_MODE_IDLE;
>
> ...this.
>
update patch now.
Thanks
Haojian
On Tue, 2010-01-26 at 01:26 -0500, Haojian Zhuang wrote:
> From 2b5a73c336d2b5dc48c8cf1f2a804b6968659f78 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <[email protected]>
> Date: Mon, 25 Jan 2010 10:24:09 -0500
> Subject: [PATCH] regulator: enable max8649 regulator driver
>
> Signed-off-by: Haojian Zhuang <[email protected]>
Can you confirm all the changes compared to the last version.
> +static int max8649_get_voltage(struct regulator_dev *rdev)
> +{
> + struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
> + unsigned char data;
> + int ret;
> +
> + ret = max8649_reg_read(info->i2c, info->vol_reg);
> + if (ret < 0)
> + return ret;
> + data = (unsigned char)ret & MAX8649_VOL_MASK;
> + return (max8649_list_voltage(rdev, data));
Any reason why we have extra () here ?
Thanks
Liam
On Tue, Jan 26, 2010 at 01:26:08AM -0500, Haojian Zhuang wrote:
This all looks good except...
> +static int max8649_enable_time(struct regulator_dev *rdev)
> +{
...
> + return (voltage / step);
I'd expect the time taken to enable to be the voltage multipled by the
step size rather than divided by the step size?
> +static int max8649_set_mode(struct regulator_dev *rdev, unsigned int mode)
> +{
> + struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
> +
> + switch (mode) {
> + case REGULATOR_MODE_FAST:
> + max8649_set_bits(info->i2c, info->vol_reg, MAX8649_FORCE_PWM,
> + MAX8649_FORCE_PWM);
> + break;
> + case REGULATOR_MODE_NORMAL:
> + max8649_set_bits(info->i2c, info->vol_reg,
> + MAX8649_FORCE_PWM, 0);
> + break;
This should really have a default case which rejects other modes.
On Tue, Jan 26, 2010 at 6:01 AM, Liam Girdwood <[email protected]> wrote:
> On Tue, 2010-01-26 at 01:26 -0500, Haojian Zhuang wrote:
>> From 2b5a73c336d2b5dc48c8cf1f2a804b6968659f78 Mon Sep 17 00:00:00 2001
>> From: Haojian Zhuang <[email protected]>
>> Date: Mon, 25 Jan 2010 10:24:09 -0500
>> Subject: [PATCH] regulator: enable max8649 regulator driver
>>
>> Signed-off-by: Haojian Zhuang <[email protected]>
>
> Can you confirm all the changes compared to the last version.
>
Yes, exactly.
>> +static int max8649_get_voltage(struct regulator_dev *rdev)
>> +{
>> + ? ? ? struct max8649_regulator_info *info = rdev_get_drvdata(rdev);
>> + ? ? ? unsigned char data;
>> + ? ? ? int ret;
>> +
>> + ? ? ? ret = max8649_reg_read(info->i2c, info->vol_reg);
>> + ? ? ? if (ret < 0)
>> + ? ? ? ? ? ? ? return ret;
>> + ? ? ? data = (unsigned char)ret & MAX8649_VOL_MASK;
>> + ? ? ? return (max8649_list_voltage(rdev, data));
>
> Any reason why we have extra () here ?
>
Fixed.
On Tue, Jan 26, 2010 at 6:04 AM, Mark Brown
<[email protected]> wrote:
> On Tue, Jan 26, 2010 at 01:26:08AM -0500, Haojian Zhuang wrote:
>
> This all looks good except...
>
>> +static int max8649_enable_time(struct regulator_dev *rdev)
>> +{
>
> ...
>
>> + ? ? return (voltage / step);
>
> I'd expect the time taken to enable to be the voltage multipled by the
> step size rather than divided by the step size?
>
I don't agree at this point. The unit of step is uV/uSec. The function
should return uSec. So voltage divided by the step is more reasonable.
Others are updated.
Thanks
Haojian
On Tue, Jan 26, 2010 at 06:54:48AM -0500, Haojian Zhuang wrote:
> On Tue, Jan 26, 2010 at 6:04 AM, Mark Brown
> > I'd expect the time taken to enable to be the voltage multipled by the
> > step size rather than divided by the step size?
> I don't agree at this point. The unit of step is uV/uSec. The function
> should return uSec. So voltage divided by the step is more reasonable.
Ah, then the variable step is confusingly named since it's actually a
rate of change rather than a step size - I'd suggest rate or something
like that instead.
On Tue, Jan 26, 2010 at 7:00 AM, Mark Brown
<[email protected]> wrote:
> On Tue, Jan 26, 2010 at 06:54:48AM -0500, Haojian Zhuang wrote:
>> On Tue, Jan 26, 2010 at 6:04 AM, Mark Brown
>
>> > I'd expect the time taken to enable to be the voltage multipled by the
>> > step size rather than divided by the step size?
>
>> I don't agree at this point. The unit of step is uV/uSec. The function
>> should return uSec. So voltage divided by the step is more reasonable.
>
> Ah, then the variable step is confusingly named since it's actually a
> rate of change rather than a step size - I'd suggest rate or something
> like that instead.
>
update this patch.
Thanks
Haojian
On Tue, Jan 26, 2010 at 07:14:14AM -0500, Haojian Zhuang wrote:
> update this patch.
Acked-by: Mark Brown <[email protected]>
As Liam said previously it'd be handy if you could explicitly list the
changes when you provide new patches - this makes it much easier to
focus in on the bits that need incremental review.
On Tue, 2010-01-26 at 07:14 -0500, Haojian Zhuang wrote:
> On Tue, Jan 26, 2010 at 7:00 AM, Mark Brown
> <[email protected]> wrote:
> > On Tue, Jan 26, 2010 at 06:54:48AM -0500, Haojian Zhuang wrote:
> >> On Tue, Jan 26, 2010 at 6:04 AM, Mark Brown
> >
> >> > I'd expect the time taken to enable to be the voltage multipled by the
> >> > step size rather than divided by the step size?
> >
> >> I don't agree at this point. The unit of step is uV/uSec. The function
> >> should return uSec. So voltage divided by the step is more reasonable.
> >
> > Ah, then the variable step is confusingly named since it's actually a
> > rate of change rather than a step size - I'd suggest rate or something
> > like that instead.
> >
>
> update this patch.
>
Applied.
Thanks
Liam