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

[jonghwa3.lee@samsung.com]

Hi, Mark,
On 2012년 05월 31일 02:26, Mark Brown wrote:

> 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.
> 


Okay, I'll do that,

>> +#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.
> 


Isn't it drivers/clk ? Could you explain more about this?


>> +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.
> 


Okay I'll separate it into two functions. One for DVS BUCKs and another
for remains.

>> +	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?


Okay, I'll remove hard coding, and will use ramp rate for setting.