[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20090526204302.GB4951@sirena.org.uk>
Date: Tue, 26 May 2009 21:43:03 +0100
From: Mark Brown <broonie@...nsource.wolfsonmicro.com>
To: Philipp Zabel <philipp.zabel@...il.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
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.
> -#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?
> + 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.
> 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.
> + 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.
--
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