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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
