Messages in this thread | | | From | "Opensource [Steve Twiss]" <> | Subject | RE: [RFC V2] DA9210 driver files | Date | Thu, 4 Jul 2013 06:19:15 +0000 |
| |
On 02 July 2013 @ 22:06, Mark Brown wrote:
>Please follow the patch submission process in SubmittingPatches. This doesn't visually >resemble most patch submissions...
Will follow the rules more closely in future.
>> >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... >
I will take a look at putting regmap_range into the driver.
>> +config REGULATOR_DA9210 >> + tristate "Dialog Semiconductor DA9210 Regulator" > >Capitalisation is wrong Here.
Will remove "R" from Regulator.
>> +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. >
Will review this and then resubmit.
>> + 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? >
.. and again
>> + 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... >
.. and this: will look at this and then re-submit.
>> + 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.
oh... apologies for this. I took it out after your last request, but it went back in again while I was debugging and I forgot to take it out before I re-submitted
| |