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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3992 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]