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 for Android: free password hash cracker in your pocket

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

Message-ID: <20120530172652.GS9947@opensource.wolfsonmicro.com>
Date:	Wed, 30 May 2012 18:26:52 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Jonghwa Lee <jonghwa3.lee@...sung.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

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?

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)