On Tue, Sep 25, 2012 at 10:11:58AM -0600, [email protected] wrote: > From: Johan Bjornstedt <[email protected]> > > There is no state machine in the AB to step up/down > the charger current to avoid dips and spikes on VBUS > and VBAT when charging is started. > Instead this is implemented in SW
Some general comment: the commit messages use random line wrapping length. It's usually a good idea to make lines no longer than 74 columns (since 'git log' adds some spaces before the message) , but too short or random is also not pretty. > Signed-off-by: Johan Bjornstedt <[email protected]> > Signed-off-by: Mattias Wallin <[email protected]> > Signed-off-by: Mathieu Poirier <[email protected]> > Reviewed-by: Karl KOMIEROWSKI <[email protected]> > --- > drivers/power/ab8500_charger.c | 172 > +++++++++++++++++++++++++++++++--------- > 1 files changed, 133 insertions(+), 39 deletions(-) > > diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c > index d4f0c98..3ceb788 100644 > --- a/drivers/power/ab8500_charger.c > +++ b/drivers/power/ab8500_charger.c > @@ -77,6 +77,9 @@ > /* Lowest charger voltage is 3.39V -> 0x4E */ > #define LOW_VOLT_REG 0x4E > > +/* Step up/down delay in us */ > +#define STEP_UDELAY 1000 > + > /* UsbLineStatus register - usb types */ > enum ab8500_charger_link_status { > USB_STAT_NOT_CONFIGURED, > @@ -934,6 +937,88 @@ static int ab8500_charger_get_usb_cur(struct > ab8500_charger *di) > } > > /** > + * ab8500_charger_set_current() - set charger current > + * @di: pointer to the ab8500_charger structure > + * @ich: charger current, in mA > + * @reg: select what charger register to set > + * > + * Set charger current. > + * There is no state machine in the AB to step up/down the charger > + * current to avoid dips and spikes on MAIN, VBUS and VBAT when > + * charging is started. Instead we need to implement > + * this charger current step-up/down here. > + * Returns error code in case of failure else 0(on success) Random line wrapping... Sophisticated editors (like vim :-), can format text for you. i.e. 'gqip' command. I'm sure emacs can do this too. > + */ > +static int ab8500_charger_set_current(struct ab8500_charger *di, > + int ich, int reg) > +{ > + int ret, i; > + int curr_index, prev_curr_index, shift_value; One variable per line, please. > + u8 reg_value; > + > + switch (reg) { > + case AB8500_MCH_IPT_CURLVL_REG: > + shift_value = MAIN_CH_INPUT_CURR_SHIFT; > + curr_index = ab8500_current_to_regval(ich); > + break; > + case AB8500_USBCH_IPT_CRNTLVL_REG: > + shift_value = VBUS_IN_CURR_LIM_SHIFT; > + curr_index = ab8500_vbus_in_curr_to_regval(ich); > + break; > + case AB8500_CH_OPT_CRNTLVL_REG: > + shift_value = 0; > + curr_index = ab8500_current_to_regval(ich); > + break; > + default: > + dev_err(di->dev, "%s current register not valid\n", __func__); > + return -ENXIO; > + } > + > + if (curr_index < 0) { > + dev_err(di->dev, "requested current limit out-of-range\n"); > + return -ENXIO; > + } > + > + ret = abx500_get_register_interruptible(di->dev, AB8500_CHARGER, > + reg, ®_value); > + if (ret < 0) { > + dev_err(di->dev, "%s read failed\n", __func__); > + return ret; > + } > + prev_curr_index = (reg_value >> shift_value); No need for parenthesis. > + /* only update current if it's been changed */ > + if (prev_curr_index == curr_index) > + return 0; > + > + dev_dbg(di->dev, "%s set charger current: %d mA for reg: 0x%02x\n", > + __func__, ich, reg); > + > + if (prev_curr_index > curr_index) { > + for (i = prev_curr_index - 1; i >= curr_index; i--) { > + ret = abx500_set_register_interruptible(di->dev, > + AB8500_CHARGER, reg, (u8) i << shift_value); > + if (ret) { > + dev_err(di->dev, "%s write failed\n", __func__); > + return ret; > + } > + usleep_range(STEP_UDELAY, STEP_UDELAY * 2); > + } > + } else { > + for (i = prev_curr_index + 1; i <= curr_index; i++) { > + ret = abx500_set_register_interruptible(di->dev, > + AB8500_CHARGER, reg, (u8) i << shift_value); > + if (ret) { > + dev_err(di->dev, "%s write failed\n", __func__); > + return ret; > + } > + usleep_range(STEP_UDELAY, STEP_UDELAY * 2); > + } > + } Too much duplication. Assuming that you need to preserve the order of the writes, i.e. if it matters to the hw, I guess this (or something alike) will work: write_current() { uint start = prev_curr_index + 1; uint stop = curr_index + 1; int direction = 1; if (prev_curr_index > curr_index) { start = prev_curr_index - 1; stop = curr_index - 1; direction = -1; } for (i = start; i != stop; i += direction) { ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, reg, (u8)i << shift_value); if (ret) { dev_err(di->dev, "%s write failed\n", __func__); return ret; } usleep_range(STEP_UDELAY, STEP_UDELAY * 2); } } If the order doesn't matter, it could be even simpler. > + return ret; > +} > + > +/** > * ab8500_charger_set_vbus_in_curr() - set VBUS input current limit > * @di: pointer to the ab8500_charger structure > * @ich_in: charger input current limit > @@ -944,8 +1029,6 @@ static int ab8500_charger_get_usb_cur(struct > ab8500_charger *di) > static int ab8500_charger_set_vbus_in_curr(struct ab8500_charger *di, > int ich_in) > { > - int ret; > - int input_curr_index; > int min_value; > > /* We should always use to lowest current limit */ > @@ -964,19 +1047,38 @@ static int ab8500_charger_set_vbus_in_curr(struct > ab8500_charger *di, > break; > } > > - input_curr_index = ab8500_vbus_in_curr_to_regval(min_value); > - if (input_curr_index < 0) { > - dev_err(di->dev, "VBUS input current limit too high\n"); > - return -ENXIO; > - } > + return ab8500_charger_set_current(di, min_value, > + AB8500_USBCH_IPT_CRNTLVL_REG); > +} > > - ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, > - AB8500_USBCH_IPT_CRNTLVL_REG, > - input_curr_index << VBUS_IN_CURR_LIM_SHIFT); > - if (ret) > - dev_err(di->dev, "%s write failed\n", __func__); > +/** > + * ab8500_charger_set_main_in_curr() - set main charger input current > + * @di: pointer to the ab8500_charger structure > + * @ich_in: input charger current, in mA > + * > + * Set main charger input current. > + * Returns error code in case of failure else 0(on success) > + */ > +static int ab8500_charger_set_main_in_curr(struct ab8500_charger *di, > + int ich_in) > +{ > + return ab8500_charger_set_current(di, ich_in, > + AB8500_MCH_IPT_CURLVL_REG); > +} > > - return ret; > +/** > + * ab8500_charger_set_output_curr() - set charger output current > + * @di: pointer to the ab8500_charger structure > + * @ich_out: output charger current, in mA > + * > + * Set charger output current. > + * Returns error code in case of failure else 0(on success) > + */ > +static int ab8500_charger_set_output_curr(struct ab8500_charger *di, > + int ich_out) Wrong indentation. > +{ > + return ab8500_charger_set_current(di, ich_out, > + AB8500_CH_OPT_CRNTLVL_REG); > } > > /** > @@ -1088,18 +1190,18 @@ static int ab8500_charger_ac_en(struct ux500_charger > *charger, > return ret; > } > /* MainChInputCurr: current that can be drawn from the charger*/ > - ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, > - AB8500_MCH_IPT_CURLVL_REG, > - input_curr_index << MAIN_CH_INPUT_CURR_SHIFT); > + ret = ab8500_charger_set_main_in_curr(di, > + di->bat->chg_params->ac_curr_max); > if (ret) { > - dev_err(di->dev, "%s write failed\n", __func__); > + dev_err(di->dev, "%s Failed to set MainChInputCurr\n", > + __func__); > return ret; > } > /* ChOutputCurentLevel: protected output current */ > - ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, > - AB8500_CH_OPT_CRNTLVL_REG, (u8) curr_index); > + ret = ab8500_charger_set_output_curr(di, iset); > if (ret) { > - dev_err(di->dev, "%s write failed\n", __func__); > + dev_err(di->dev, > + "%s Failed to set ChOutputCurentLevel\n", __func__); > return ret; > } > > @@ -1156,12 +1258,11 @@ static int ab8500_charger_ac_en(struct ux500_charger > *charger, > return ret; > } > > - ret = abx500_set_register_interruptible(di->dev, > - AB8500_CHARGER, > - AB8500_CH_OPT_CRNTLVL_REG, CH_OP_CUR_LVL_0P1); > + ret = ab8500_charger_set_output_curr(di, 0); > if (ret) { > dev_err(di->dev, > - "%s write failed\n", __func__); > + "%s Failed to set ChOutputCurentLevel\n", > + __func__); Weird, inconsistent indentation. > return ret; > } > } else { > @@ -1264,10 +1365,11 @@ static int ab8500_charger_usb_en(struct ux500_charger > *charger, > return ret; > } > /* ChOutputCurentLevel: protected output current */ > - ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, > - AB8500_CH_OPT_CRNTLVL_REG, (u8) curr_index); > + ret = ab8500_charger_set_output_curr(di, ich_out); > if (ret) { > - dev_err(di->dev, "%s write failed\n", __func__); > + dev_err(di->dev, > + "%s Failed to set ChOutputCurentLevel\n", > + __func__); Weird indentation. > return ret; > } > /* Check if VBAT overshoot control should be enabled */ > @@ -1364,7 +1466,6 @@ static int ab8500_charger_update_charger_current(struct > ux500_charger *charger, > int ich_out) > { > int ret; > - int curr_index; > struct ab8500_charger *di; > > if (charger->psy.type == POWER_SUPPLY_TYPE_MAINS) > @@ -1374,18 +1475,11 @@ static int > ab8500_charger_update_charger_current(struct ux500_charger *charger, > else > return -ENXIO; > > - curr_index = ab8500_current_to_regval(ich_out); > - if (curr_index < 0) { > - dev_err(di->dev, > - "Charger current too high, " > - "charging not started\n"); > - return -ENXIO; > - } > - > - ret = abx500_set_register_interruptible(di->dev, AB8500_CHARGER, > - AB8500_CH_OPT_CRNTLVL_REG, (u8) curr_index); > + ret = ab8500_charger_set_output_curr(di, ich_out); > if (ret) { > - dev_err(di->dev, "%s write failed\n", __func__); > + dev_err(di->dev, > + "%s Failed to set ChOutputCurentLevel\n", > + __func__); Weird indentation. No need for the first line wrap. > return ret; > } > > -- > 1.7.5.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
