Re: [PATCH] Add regulator driver for the bq2407x family of charger ICs

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Sat, Aug 20, 2011 at 10:24:51PM +0200, Heiko Stübner wrote:

Mostly looks good, just a few fairly small comments below.

> +	if (pdata->max_uA && pdata->max_uA > 500000
> +			      && max_uA >= pdata->max_uA) {
> +		dev_dbg(rdev_get_dev(rdev),
> +			"setting current limit to %d mA\n",
> +			pdata->max_uA / 1000);
> +		gpio_set_value(pdata->gpio_en2, 1);
> +		gpio_set_value(pdata->gpio_en1, 0);
> +	} else if (max_uA >= 500000) {
> +		dev_dbg(rdev_get_dev(rdev),
> +			"setting current limit to 500 mA\n");
> +		gpio_set_value(pdata->gpio_en2, 0);
> +		gpio_set_value(pdata->gpio_en1, 1);
> +	} else if (max_uA >= 100000) {
> +		dev_dbg(rdev_get_dev(rdev),
> +			"setting current limit to 100 mA\n");
> +		gpio_set_value(pdata->gpio_en2, 0);
> +		gpio_set_value(pdata->gpio_en1, 0);
> +	} else {
> +		dev_dbg(rdev_get_dev(rdev),
> +			"setting current limit to 0 mA\n");
> +		gpio_set_value(pdata->gpio_en2, 1);
> +		gpio_set_value(pdata->gpio_en1, 1);
> +	}

I'd rather expect this to return an error sometimes.

> +static int bq2407x_get_current_limit(struct regulator_dev *rdev)
> +{
> +	struct bq2407x_mach_info *pdata = rdev_get_drvdata(rdev);
> +
> +	int en2 = gpio_get_value(pdata->gpio_en2);
> +	int en1 = gpio_get_value(pdata->gpio_en1);

Calling gpio_get_value() on an output GPIO is undefined, you need to
remember what values you set.

> +static int bq2407x_enable(struct regulator_dev *rdev)
> +{
> +	struct bq2407x_mach_info *pdata = rdev_get_drvdata(rdev);
> +
> +	dev_dbg(rdev_get_dev(rdev), "enabling charger\n");
> +
> +	gpio_set_value(pdata->gpio_nce, 0);
> +	return 0;

Blank line here.