linux-kernel - Re: [PATCH 2/2] regulator: Add Dialog DA9063 voltage regulators support.

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: <20130724172959.GR9858@sirena.org.uk>
Date:	Wed, 24 Jul 2013 18:29:59 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	linux-kernel@...r.kernel.org, Lee Jones <lee.jones@...aro.org>,
	Krystian Garbaciak <krystian.garbaciak@...semi.com>
Subject: Re: [PATCH 2/2] regulator: Add Dialog DA9063 voltage regulators
 support.

On Wed, Jul 24, 2013 at 06:34:43PM +0200, Philipp Zabel wrote:

> +/* Definition for registering bit fields */
> +struct bfield {
> +	unsigned short	addr;
> +	unsigned char	mask;
> +};
> +#define BFIELD(_addr, _mask) \
> +	{ .addr = _addr, .mask = _mask }
> +
> +struct field {
> +	unsigned short	addr;
> +	unsigned char	mask;
> +	unsigned char	shift;
> +	unsigned char	offset;
> +};
> +#define FIELD(_addr, _mask, _shift, _offset) \
> +	{ .addr = _addr, .mask = _mask, .shift = _shift, .offset = _offset }

This looks like it should be generic (and there is actually a
regmap_field API for bitfields...).

> +static struct regulator_ops da9063_switch_ops = {
> +	.enable			= regulator_enable_regmap,
> +	.disable		= regulator_disable_regmap,
> +	.is_enabled		= regulator_is_enabled_regmap,
> +	.set_suspend_enable	= regulator_enable_regmap,
> +	.set_suspend_disable	= regulator_disable_regmap,
> +};

This is clearly broken, it's using the same generic ops for both suspend
and regular enable which means that changing the suspend state will
change the state at runtime which isn't what's wanted.

> +static struct regulator_ops da9063_ldo_ops = {
> +	.enable			= da9063_enable,

You should be avoiding forward declarations of all these things by the
way, look at the styles followed by other drivers.

> +static int da9063_update_mode_internal(struct da9063_regulator *regl,
> +				       int sys_state)
> +{
> +	const struct da9063_regulator_info *rinfo = regl->info;
> +	unsigned val;
> +	unsigned mode;
> +	int ret;
> +
> +	if (sys_state == SYS_STATE_SUSPEND)
> +		/* Get mode for regulator in suspend state */
> +		mode = regl->suspend_mode;
> +	else
> +		/* Get mode for regulator in normal operation */
> +		mode = regl->mode;

What's all this about then...

> +	/* Bucks use single mode register field for normal operation
> +	   and suspend state. LDOs use sleep flags - one for normal
> +	   and one for suspend state. */
> +	if (rinfo->mode.addr) {
> +		/* For BUCKs, there are 3 modes to map to */
> +		ret = regmap_read(regl->hw->regmap, rinfo->mode.addr, &val);
> +		if (ret < 0)
> +			return ret;

If the different regulators have different register layouts and mode
sets then they should be using different ops, this will make things much
easier to follow as there will be fewer conditional cases.

> +	} else {
> +		/* No support */
> +		return 0;
> +	}

This should be an error, though if the regulator doesn't support mode
setting it oughtn't to have the operation in the first place.

> +static int da9063_enable(struct regulator_dev *rdev)
> +{
> +	struct da9063_regulator *regl = rdev_get_drvdata(rdev);
> +	int ret;
> +
> +	if (regl->info->suspend.mask) {
> +		/* Make sure to exit from suspend mode on enable */
> +		ret = regmap_update_bits(regl->hw->regmap,
> +					 regl->info->suspend.addr,
> +					 regl->info->suspend.mask, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* BUCKs need mode update after wake-up from suspend state. */
> +		ret = da9063_update_mode_internal(regl, SYS_STATE_NORMAL);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return regulator_enable_regmap(rdev);
> +}

You really do need to expain all this suspend mode stuff in more detail.
Why is a regulator enable doing anything to do with suspend states,
especially given that the regulator isn't put into suspend mode when
disabled.

> +#ifdef CONFIG_OF
> +static struct of_regulator_match da9063_matches[] = {
> +	[DA9063_ID_BCORE1]           = { .name = "bcore1"           },

Any new DT bindings need to be documented.

> +	/* Allocate memory required by usable regulators */
> +	size = sizeof(struct da9063_regulators) +
> +		regl_pdata->n_regulators * sizeof(struct da9063_regulator);
> +	regulators = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> +	if (!regulators) {
> +		dev_err(&pdev->dev, "No memory for regulators\n");
> +		return -ENOMEM;
> +	}

The driver should register all the regulators that physically exist, it
shouldn't be referring to platform data for this.

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