On Fri, Jan 14, 2011 at 03:53:38PM +0900, MyungJoo Ham wrote:
> This patch supports PMIC/Regulator part of MAX8997/MAX8966 MFD.
> In this initial release, selecting voltages or current-limit
> and switching on/off the regulators are supported.
Mostly good, a few stylistic things and this is out of date for the
current regulator API.
> +static int max8997_get_voltage(struct regulator_dev *rdev)
> +{
> + ret = max8997_list_voltage(rdev, val);
> + if (ret == -EINVAL) {
> + int ldo = max8997_get_ldo(rdev);
It'd probably be easier to just define separate get_voltage() and
list_voltage() functions for these regulators.
> +static int max8997_set_voltage_ldo(struct regulator_dev *rdev,
> + int min_uV, int max_uV)
This needs updating for the current API in -next (and 2.6.38).
> + if ((ldo > MAX8997_BUCK7 || ldo < MAX8997_LDO1) &&
> + ldo != MAX8997_CHARGER_CV &&
> + ldo != MAX8997_CHARGER &&
> + ldo != MAX8997_CHARGER_TOPOFF)
> + return -EINVAL;
> + if (ldo == MAX8997_BUCK6)
> + return -EINVAL;
A switch statement for the valid regulators might be clearer.
> +static int max8997_set_voltage_buck(struct regulator_dev *rdev,
> + int min_uV, int max_uV)
API update needed here too.
> +static int max8997_set_voltage_safeout(struct regulator_dev *rdev,
> + int min_uV, int max_uV)
> +{
Here also. Given that you've got a separate set_voltage() the comment
about a separate get_voltage() above seems even stronger.
> +static inline unsigned int max8997_buck_voltage(int uV)
> +{
> + int step;
> +
> + if (uV <= 650000)
> + return 0x0;
> + if (uV >= 2225000)
> + return 0x3f;
> +
> + step = uV - 650000;
> + step /= 25000;
> +
> + return step + 1;
> +}
Shouldn't this code be shared with set_voltage()? Or...
> + max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
> + max8997_buck_voltage(pdata->buck1_voltage[i]),
> + 0x3f);
> + max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i),
> + max8997_buck_voltage(pdata->buck2_voltage[i]),
> + 0x3f);
> + max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i),
> + max8997_buck_voltage(pdata->buck5_voltage[i]),
> + 0x3f);
...just call your set_voltage() operation directly for these?
> + if (gpio_is_valid(pdata->buck125_gpios[0]) &&
> + gpio_is_valid(pdata->buck125_gpios[1]) &&
> + gpio_is_valid(pdata->buck125_gpios[2])) {
Strange indentation here.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
