[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <74d0deb30905261426w7739c394ge3246c80890f1de6@mail.gmail.com>
Date: Tue, 26 May 2009 23:26:32 +0200
From: pHilipp Zabel <philipp.zabel@...il.com>
To: Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc: linux-kernel@...r.kernel.org,
Robert Jarzmik <robert.jarzmik@...e.fr>,
Liam Girdwood <lrg@...mlogic.co.uk>
Subject: Re: [PATCH] regulator/max1586: support increased V3 voltage range
Hi Mark,
thanks for the review.
On Tue, May 26, 2009 at 10:43 PM, Mark Brown
<broonie@...nsource.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists