Re: [PATCH 01/01] regulator: support max8649
Mark Brown
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Mark Brown
> 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.
Liam Girdwood
> From 2b5a73c336d2b5dc48c8cf1f2a804b6968659f78 Mon Sep 17 00:00:00 2001
> From: Haojian Zhuang <haojian...@marvell.com>
> Date: Mon, 25 Jan 2010 10:24:09 -0500
> Subject: [PATCH] regulator: enable max8649 regulator driver
>
> Signed-off-by: Haojian Zhuang <haojian...@marvell.com>
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
Mark Brown
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.
Haojian Zhuang
> On Tue, 2010-01-26 at 01:26 -0500, Haojian Zhuang wrote:
>> From 2b5a73c336d2b5dc48c8cf1f2a804b6968659f78 Mon Sep 17 00:00:00 2001
>> From: Haojian Zhuang <haojian...@marvell.com>
>> Date: Mon, 25 Jan 2010 10:24:09 -0500
>> Subject: [PATCH] regulator: enable max8649 regulator driver
>>
>> Signed-off-by: Haojian Zhuang <haojian...@marvell.com>
>
> 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 ?
>
Fixed.
Mark Brown
> 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.
Mark Brown
> update this patch.
Acked-by: Mark Brown <bro...@opensource.wolfsonmicro.com>
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.
Liam Girdwood
> On Tue, Jan 26, 2010 at 7:00 AM, Mark Brown
> <bro...@opensource.wolfsonmicro.com> 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.
> >
>
>
Applied.
Thanks
Liam
Haojian Zhuang
> On Tue, 2010-01-26 at 07:14 -0500, Haojian Zhuang wrote:
>> On Tue, Jan 26, 2010 at 7:00 AM, Mark Brown
>> <bro...@opensource.wolfsonmicro.com> wrote:
>> >
>>
>> update this patch.
>>
>
> Applied.
>
> Thanks
>
> Liam
>
>
Hi Liam & Mark,
I just want to confirm whether max8649 is merged into regulator
codebase. It seems that I can't find max8649 in regulator git tree.
Thanks
Haojian
Liam Girdwood
> On Tue, Jan 26, 2010 at 11:10 AM, Liam Girdwood <l...@slimlogic.co.uk> wrote:
> > On Tue, 2010-01-26 at 07:14 -0500, Haojian Zhuang wrote:
> >> On Tue, Jan 26, 2010 at 7:00 AM, Mark Brown
> >> <bro...@opensource.wolfsonmicro.com> wrote:
> >> >
> >>
> >> update this patch.
> >>
> >
> > Applied.
> >
> > Thanks
> >
> > Liam
> >
> >
>
> Hi Liam & Mark,
>
> I just want to confirm whether max8649 is merged into regulator
> codebase. It seems that I can't find max8649 in regulator git tree.
max8649 was merged into regulator and has now been pulled by Linus.
Where were you looking ?
Thanks
Liam
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk