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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

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/