linux-kernel - Re: [PATCH V3] regulator: tps65910: Sleep control through external inputs

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: <20120127170757.GD18572@opensource.wolfsonmicro.com>
Date:	Fri, 27 Jan 2012 17:07:58 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	lrg@...com, jedu@...mlogic.co.uk, sameo@...ux.intel.com,
	gg@...mlogic.co.uk, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH V3] regulator: tps65910: Sleep control through external
 inputs

On Wed, Jan 25, 2012 at 09:18:30PM +0530, Laxman Dewangan wrote:

> +static int tps65910_set_suspend_enable(struct regulator_dev *dev)
> +{
> +	struct tps65910_reg *pmic = rdev_get_drvdata(dev);
> +	int id = rdev_get_id(dev);
> +	/*
> +	 * If regulator is controlled through external control then
> +	 * it can be enable/disable by toggling external signal.
> +	 */
> +	if (pmic->board_ext_control[id])
> +		return 0;
> +	else
> +		return tps65910_set_mode(dev, REGULATOR_MODE_NORMAL);
> +}

I'm really confuseed now.  This definitely looks like it's doing the
wrong thing for the non-ext_control case, it's setting the mode which
really isn't what this is supposed to do and collides with any actual
configuration of the mode that might happen...

> +	/*
> +	 * Keep the regulator in OFF state if it needs to be disable
> +	 * in suspend state.
> +	 */
> +	if (pmic->board_ext_control[id]) {
> +		u8 regoffs = (pmic->ext_sleep_control[id] >> 8) & 0xFF;
> +		u8 bit_pos = (1 << pmic->ext_sleep_control[id] & 0xFF);
> +		int ret;
> +		ret = tps65910_clear_bits(mfd,
> +			TPS65910_SLEEP_KEEP_LDO_ON + regoffs, bit_pos);
> +		if (!ret)
> +			ret = tps65910_set_bits(mfd,
> +				TPS65910_SLEEP_SET_LDO_OFF + regoffs, bit_pos);
> +		if (ret < 0)
> +			dev_err(mfd->dev,
> +				"Error in configuring SLEEP register\n");

...and I'd really expect something that reverses these changes?

The actual bits setting up the ext_control look OK - can you split those
off from the bits implementing the suspend mode callbacks please so they
can be applied while the callbacks are reviewed?

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