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

From: Mark Brown
Date: Thu Aug 02 2012 - 06:30:13 EST


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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/