Re: [PATCH] regulator: cpcap: Add basic regulator support

[Mark Brown]

On Mon, Jan 30, 2017 at 04:12:41PM -0800, Tony Lindgren wrote:

> +static int cpcap_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	unsigned int value = 0;
> +
> +	regmap_read(rdev->regmap, rdev->desc->enable_reg, &value);
> +
> +	return ((value & rdev->desc->enable_mask) == rdev->desc->enable_reg);
> +}

Use the core support for this, reulator_is_enabled_regmap().

> +	error = regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
> +				   rdev->desc->enable_mask,
> +				   rdev->desc->enable_val);
> +	if (error)
> +		return error;
> +
> +	if (rdev->desc->enable_val & CPCAP_REG_OFF_MODE_SEC) {
> +		error = regmap_update_bits(rdev->regmap, regulator->assign_reg,
> +					   regulator->assign_mask,
> +					   regulator->assign_mask);
> +		if (error)
> +			return error;
> +	}

What does this second register bit do?

> +static int cpcap_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct cpcap_regulator *regulator = rdev_get_drvdata(rdev);
> +	unsigned int value = 0;
> +	int voltage;

regulator_get_voltage_sel_regmap() (the driver should just use selectors
and let the mapping happen in the core).

> +	regmap_read(rdev->regmap, rdev->desc->enable_reg, &value);
> +
> +	if (!(value & rdev->desc->enable_mask))
> +		return 0;

This is buggy, the driver should report the voltage the regulator will
have when powered.

> +static int cpcap_regulator_set_voltage(struct regulator_dev *rdev,
> +				       int min_uV, int max_uV,
> +				       unsigned int *selector)

regulator_map_voltage_iterate() and regulator_set_voltage_regmap()

> +	switch (mode) {
> +	case REGULATOR_MODE_NORMAL:
> +		value = CPCAP_BIT_AUDIO_LOW_PWR;
> +		break;
> +	case REGULATOR_MODE_IDLE:
> +	case REGULATOR_MODE_STANDBY:
> +		value = 0;
> +		break;

There should be a 1:1 mapping between modes and register values, if the
device only supports two modes so should the driver.

> +	init_data.constraints.valid_modes_mask =
> +		REGULATOR_MODE_NORMAL | REGULATOR_MODE_STANDBY;
> +	init_data.constraints.valid_ops_mask =
> +		REGULATOR_CHANGE_MODE | REGULATOR_CHANGE_STATUS;
> +	config.init_data = &init_data;

Drivers should not configure constraints, they're part of the system
integration and come from the firmware or board file.

> +		dev_info(&pdev->dev, "registering regulator %s\n",
> +			 regulator->rdesc.name);

This is needless spam, please remove it.

Attachment: signature.asc
Description: PGP signature