Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

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

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

On Tue, May 29, 2012 at 11:20:51AM +0900, Jonghwa Lee wrote:
> Add driver for support max77686 regulator.
> MAX77686 provides LDOs[1~26] and BUCKs[1~9]. It support to set or get the
> volatege of regulator on max77686 chip with using regmap.
> 
> v5
> - Remove unnecessary initializing and variable.
> 

Don't put stuff like this in the changelog, it's useless noise in git.
Include it after the cut as documented in SubmittingPatches.

> +#ifdef CONFIG_COMMON_CLK
> +	struct clk clk32khz_ap;
> +	struct clk clk32khz_cp;
> +	struct clk clk32khz_pmic;
> +#endif

This should be a clock driver in drivers/clock.

> +static int max77686_set_voltage_time_sel(struct regulator_dev *rdev,
> +			unsigned int old_selector, unsigned int new_selector)
> +{
> +	struct max77686_data *max77686 = rdev_get_drvdata(rdev);
> +	int rid = rdev_get_id(rdev);
> +
> +	switch (rid) {
> +	case MAX77686_BUCK2 ... MAX77686_BUCK4:
> +		return (DIV_ROUND_UP(rdev->desc->uV_step
> +			* abs(new_selector - old_selector),
> +			max77686->ramp_delay * 1000));
> +	}
> +	/* Unconditionally 100 mV/us */
> +	return (DIV_ROUND_UP(rdev->desc->uV_step
> +		 * abs(new_selector - old_selector), 100000));
> +}

Just do separate functions.  The above is pretty illegible.

> +	max77686->ramp_delay = MAX77686_RAMP_RATE; /* Set 0x3 for RAMP */
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK2CTRL1, 0xC0, 0xC0);
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK3CTRL1, 0xC0, 0xC0);
> +	regmap_update_bits(max77686->iodev->regmap,
> +					 MAX77686_REG_BUCK4CTRL1, 0xC0, 0xC0);

This still appears to not be referencing the ramp rate that's being set?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]