MIME-version: 1.0
Content-transfer-encoding: 8BIT
Content-type: text/plain; charset=UTF-8
Message-id: <4FC71161.9010002@samsung.com>
Date: Thu, 31 May 2012 15:36:17 +0900
From: jonghwa3.lee@samsung.com
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20120216
 Thunderbird/10.0.2
To: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: linux-kernel@vger.kernel.org, Liam Girdwood <lrg@ti.com>,
        Yadwinder Singh Brar <yadi.brar01@gmail.com>,
        Chiwoong Byun <woong.byun@samsung.com>,
        Myungjoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>
Subject: Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver
References: <1338258051-3155-1-git-send-email-jonghwa3.lee@samsung.com>
 <20120530172652.GS9947@opensource.wolfsonmicro.com>
In-reply-to: <20120530172652.GS9947@opensource.wolfsonmicro.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2274
Lines: 73

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/