Re: [PATCH v2] regulator: add new regulator driver for lp872x

[Mark Brown]

On Fri, Jun 15, 2012 at 08:46:34AM +0000, Kim, Milo wrote:
> This driver supports TI/National Semiconductor LP8720, LP8725 PMIC.

Overall looks very good, just a few fairly minor things below but
nothing substantial.

> 
> patch v2
> --------

See Documentation/SubmittingPatches for how to format stuff like this -
put it after the ---

> +	if (chip == LP8720) {
> +		mask = LP8720_TIMESTEP_M;
> +		shift = LP8720_TIMESTEP_S;
> +		time_usec = &lp8720_time_usec[0];
> +		size = ARRAY_SIZE(lp8720_time_usec);
> +	} else if (chip == LP8725) {
> +		mask = LP8725_TIMESTEP_M;
> +		shift = LP8725_TIMESTEP_S;
> +		time_usec = &lp8725_time_usec[0];
> +		size = ARRAY_SIZE(lp8725_time_usec);
> +	} else {
> +		return -EINVAL;
> +	}

This should be a switch statement.

> +static int lp8725_buck_set_current_limit(struct regulator_dev *rdev,
> +					int min_uA, int max_uA)
> +{
> +	struct lp872x *lp = rdev_get_drvdata(rdev);
> +	enum lp872x_regulator_id buck = rdev_get_id(rdev);
> +	int i, max = ARRAY_SIZE(lp8725_buck_uA);
> +	u8 addr, val;
> +
> +	if (buck == LP8725_ID_BUCK1)
> +		addr = LP8725_BUCK1_VOUT2;
> +	else if (buck == LP8725_ID_BUCK2)
> +		addr = LP8725_BUCK2_VOUT2;
> +	else
> +		return -EINVAL;

Again, switch statement.

> +static const struct regmap_config lp872x_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = MAX_REGISTERS,
> +};

Probably worth enabling cache here, it'll cut out read/modify/write
cycles which will improve the performance as the reads should be free.
Not essential but worth considering.

Attachment: signature.asc
Description: Digital signature