Re: [NEW DRIVER V1 7/7] DA9058 REGULATOR driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Thu, Aug 02, 2012 at 09:48:58AM +0100, Anthony Olech wrote:

Overall the big issue here is that the driver isn't making much use of
framework features, there should be a *lot* less code here.

> +static unsigned int da9058_regulator_val_to_mvolts(unsigned int val,
> +						struct regulator_dev *rdev)

regualtor_map_voltage_linear()

> +	ret = da9058_reg_read(da9058, regulator->control_register, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (regulator->control_enable_mask&val)
> +		return true;
> +	else
> +		return false;

regulator_is_enabled_regmap().

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

Should be set_voltage_sel().

> +	if (regulator->ramp_register && regulator->ramp_enable_mask)
> +		ret =
> +			da9058_set_bits(da9058, regulator->ramp_register,
> +					regulator->ramp_enable_mask);

What is this doing?

> +	if (regulator->control_voltage_step == 0) {
> +		if (da9058_regulator_is_enabled(rdev))
> +			return regulator->fixed_voltage;
> +		else
> +			return 0;
> +	}

No, don't do stuff like this - implement a separate op (or just use the
standard ops in this case).

> +
> +	ret = da9058_reg_read(da9058, regulator->control_register, &val);
> +	if (ret)
> +		return ret;

This should just be regulator_get_voltage_sel_regmap().

> +static int da9058_regulator_enable(struct regulator_dev *rdev)
> +{

regulator_enable_regmap() and similarly for all the others.

> +static unsigned int da9058_regulator_get_mode(struct regulator_dev *rdev)
> +{
> +	return REGULATOR_MODE_NORMAL;
> +}

If you don't support modes don't implement the op.

> +	regulator_pdata = cell->platform_data;

> +	if (regulator_pdata == NULL) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}

The driver should be able to start without any platform data.

> +	dev_info(&pdev->dev, "Starting REGULATOR %d = %s\n",
> +					regulator_pdata->regulator_id,
> +					regulator_pdata->regulator_name);

There's a couple of issues here.  One is that you shouldn't be logging
anything here, log messages on driver load are only useful if you're
saying something you've identified from the hardware - for example,
logging the device revision on startup is very helpful.  The other is
that you shouldn't need to supply a name and ID as platform data since
that's exactly how the platform bus binds drivers.

> +	regulator->control_voltage_step = regulator_pdata->control_voltage_step;
> +	regulator->control_register = regulator_pdata->control_register;
> +	regulator->control_enable_mask = regulator_pdata->control_enable_mask;
> +	regulator->ramp_register = regulator_pdata->ramp_register;
> +	regulator->ramp_enable_mask = regulator_pdata->ramp_enable_mask;
> +	regulator->fixed_voltage = regulator_pdata->fixed_voltage;

Your MFD didn't visibly provide any of this stuff, though I may have
missed it, and honestly I'd expect most of this to be in the driver.