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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <4FC71161.9010002@samsung.com>
Date:	Thu, 31 May 2012 15:36:17 +0900
From:	jonghwa3.lee@...sung.com
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
Cc:	linux-kernel@...r.kernel.org, Liam Girdwood <lrg@...com>,
	Yadwinder Singh Brar <yadi.brar01@...il.com>,
	Chiwoong Byun <woong.byun@...sung.com>,
	Myungjoo Ham <myungjoo.ham@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>
Subject: Re: [PATCH v5] regulator: MAX77686: Add Maxim 77686 regulator driver

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/