On Mon, Jul 01, 2013 at 03:10:28PM +0100, Steve Twiss wrote: > From: Steve Twiss <stwiss.opensou...@diasemi.com> > > This is the regulator driver for the Dialog DA9210 Multi-phase Buck. > The patch is relative to linux-next next-20130701 > and this is RFC attempt number 2
Please follow the patch submission process in SubmittingPatches. This
doesn't visually resemble most patch submissions...
> >This looks like you should be using a regmap range.
> The use of regmap_range is not being considered because I am not intending
> to use PAGE_CON register page selection in any of the driver development.
Makes sense to map things in for diagnostics...
> +config REGULATOR_DA9210
> + tristate "Dialog Semiconductor DA9210 Regulator"
Capitalisation is wrong Here.
> +static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
> + int max_uA)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + unsigned int sel;
> + int i;
> +
> + /* search for closest to maximum */
> + for (i = N_CURRENT_LIMITS-1; i >= 0; i--) {
Coding style.
> + ret = regmap_read(chip->regmap, DA9210_REG_BUCK_ILIM, &data);
> + if (ret < 0)
> + return ret;
> +
> + sel = (data & DA9210_BUCK_ILIM_MASK) >> DA9210_BUCK_ILIM_SHIFT;
> +
> + return da9210_buck_limits[sel];
There's no unused values in the selector?
> + chip->desc.id = 0;
> + chip->desc.type = REGULATOR_VOLTAGE;
> + chip->desc.n_voltages = ((DA9210_MAX_MV - DA9210_MIN_MV)
> + / DA9210_STEP_MV) + 1;
> + chip->desc.ops = &da9210_buck_ops;
> + chip->desc.owner = THIS_MODULE;
> + chip->desc.name = "DA9210";
> + chip->desc.enable_reg = DA9210_REG_BUCK_CONT;
> + chip->desc.enable_mask = DA9210_BUCK_EN;
> + chip->desc.vsel_reg = DA9210_REG_VBUCK_A;
> + chip->desc.vsel_mask = DA9210_VBUCK_MASK;
> + chip->desc.min_uV = (DA9210_MIN_MV * 1000);
> + chip->desc.uV_step = (DA9210_STEP_MV * 1000);
Why is this not just global static data? There's nothing variable
here...
> + dev_info(&i2c->dev,
> + "DA9210 device detected\n");
> +
Remove this - it's just noise, apart from anything else nothing here has
actually verified that the chip exists.
signature.asc
Description: Digital signature
