lkml.org 
[lkml]   [2013]   [Jul]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC V2] DA9210 driver files
Date
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




\
 
 \ /
  Last update: 2013-07-04 09:01    [W:0.091 / U:2.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site