| Message ID | 1474932670-11953-3-git-send-email-wolfgit@wiedmeyer.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Sep 27, 2016 at 01:31:09AM +0200, Wolfgang Wiedmeyer wrote: > For MAX77693, the fast charge current also needs to be manipulated for > proper charging. The fast charge current is only set in the case of > the MAX77693 type, as the MAX77843 properly manipulates the fast > charge current. Are you sure it has to be manipulated? Some time I didn't dig into this. Now I looked at the datasheet and it says that usually there is no need for changing the charge current during the operation. Maxim recommends to setting it to a maximum safe value for the battery. The device will manage charge current on its own. However I agree that the charge current should be set... maybe once, to a maximum value appropriate for battery. Probably this should be done by max77693_charger in max77693_dt_init(). Best regards, Krzysztof > The fast charge current is set to the next possible value below the > maximum input current. > > Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de> > --- > drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++--- > 1 file changed, 42 insertions(+), 3 deletions(-) > > diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c > index cfbb951..e2f7584 100644 > --- a/drivers/regulator/max77693-regulator.c > +++ b/drivers/regulator/max77693-regulator.c > @@ -54,14 +54,19 @@ struct chg_reg_data { > unsigned int linear_mask; > unsigned int uA_step; > unsigned int min_sel; > + > + bool set_fast; > + unsigned int fast_reg; > + unsigned int fast_mask; > }; > > /* > * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA > * 0x00, 0x01, 0x2, 0x03 = 60 mA > * 0x04 ~ 0x7E = (60 + (X - 3) * 20) mA > - * Actually for MAX77693 the driver manipulates the maximum input current, > - * not the fast charge current (output). This should be fixed. > + * Actually for MAX77693 the driver manipulates the maximum input current > + * and the fast charge current (output) because the fast charge current > + * is not set. > * > * On MAX77843 the calculation formula is the same (except values). > * Fortunately it properly manipulates the fast charge current. > @@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, > const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev); > unsigned int chg_min_uA = rdev->constraints->min_uA; > int sel = 0; > + unsigned int data; > + int ret; > > while (chg_min_uA + reg_data->uA_step * sel < min_uA) > sel++; > @@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, > /* the first four codes for charger current are all 60mA */ > sel += reg_data->min_sel; > > - return regmap_write(rdev->regmap, reg_data->linear_reg, sel); > + ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel); > + if (ret < 0) > + return ret; > + > + if (reg_data->set_fast) { > + /* disable fast charge if minimum value */ > + if (sel == reg_data->min_sel) > + data = 0; > + else { > + /* > + * set the fast charge current to the closest value > + * below the input current > + */ > + ret = regmap_read(rdev->regmap, reg_data->fast_reg, > + &data); > + if (ret < 0) > + return ret; > + > + sel *= reg_data->uA_step / 1000; /* convert to mA */ > + data &= ~reg_data->fast_mask; > + data |= sel * 10 / 333; /* 0.1A/3 steps */ > + } > + > + ret = regmap_write(rdev->regmap, reg_data->fast_reg, data); > + if (ret < 0) > + return ret; > + } > + > + return 0; > } > /* end of CHARGER regulator ops */ > > @@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = { > .linear_mask = CHG_CNFG_09_CHGIN_ILIM_MASK, > .uA_step = 20000, > .min_sel = 3, > + .set_fast = true, > + .fast_reg = MAX77693_CHG_REG_CHG_CNFG_02, > + .fast_mask = CHG_CNFG_02_CC_MASK, > }; > > #define max77843_regulator_desc_esafeout(num) { \ > @@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = { > .linear_mask = MAX77843_CHG_FAST_CHG_CURRENT_MASK, > .uA_step = MAX77843_CHG_FAST_CHG_CURRENT_STEP, > .min_sel = 2, > + .set_fast = false, > }; > > static int max77693_pmic_probe(struct platform_device *pdev) > -- > 2.8.0.rc3 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski writes: > On Tue, Sep 27, 2016 at 01:31:09AM +0200, Wolfgang Wiedmeyer wrote: >> For MAX77693, the fast charge current also needs to be manipulated for >> proper charging. The fast charge current is only set in the case of >> the MAX77693 type, as the MAX77843 properly manipulates the fast >> charge current. > > Are you sure it has to be manipulated? Some time I didn't dig into this. > Now I looked at the datasheet and it says that usually there is no need > for changing the charge current during the operation. Maxim recommends > to setting it to a maximum safe value for the battery. The device will > manage charge current on its own. > > However I agree that the charge current should be set... maybe once, to > a maximum value appropriate for battery. Probably this should be done > by max77693_charger in max77693_dt_init(). When charging is disabled (e.g. by removing the USB cable) the charge current is not reset to zero. So if I expose the current by the CURRENT_NOW property, it incorrectly reports the current that was set when charging was enabled, although there is no charging going on anymore. So I felt the need to update the charge current every time the charger gets enabled or disabled. Initially, the charge current is set to zero, so I think it needs to be set at least at the beginning to enable charging. Thanks, Wolfgang > Best regards, > Krzysztof > > >> The fast charge current is set to the next possible value below the >> maximum input current. > > >> >> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de> >> --- >> drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++--- >> 1 file changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c >> index cfbb951..e2f7584 100644 >> --- a/drivers/regulator/max77693-regulator.c >> +++ b/drivers/regulator/max77693-regulator.c >> @@ -54,14 +54,19 @@ struct chg_reg_data { >> unsigned int linear_mask; >> unsigned int uA_step; >> unsigned int min_sel; >> + >> + bool set_fast; >> + unsigned int fast_reg; >> + unsigned int fast_mask; >> }; >> >> /* >> * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA >> * 0x00, 0x01, 0x2, 0x03 = 60 mA >> * 0x04 ~ 0x7E = (60 + (X - 3) * 20) mA >> - * Actually for MAX77693 the driver manipulates the maximum input current, >> - * not the fast charge current (output). This should be fixed. >> + * Actually for MAX77693 the driver manipulates the maximum input current >> + * and the fast charge current (output) because the fast charge current >> + * is not set. >> * >> * On MAX77843 the calculation formula is the same (except values). >> * Fortunately it properly manipulates the fast charge current. >> @@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, >> const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev); >> unsigned int chg_min_uA = rdev->constraints->min_uA; >> int sel = 0; >> + unsigned int data; >> + int ret; >> >> while (chg_min_uA + reg_data->uA_step * sel < min_uA) >> sel++; >> @@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, >> /* the first four codes for charger current are all 60mA */ >> sel += reg_data->min_sel; >> >> - return regmap_write(rdev->regmap, reg_data->linear_reg, sel); >> + ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel); >> + if (ret < 0) >> + return ret; >> + >> + if (reg_data->set_fast) { >> + /* disable fast charge if minimum value */ >> + if (sel == reg_data->min_sel) >> + data = 0; >> + else { >> + /* >> + * set the fast charge current to the closest value >> + * below the input current >> + */ >> + ret = regmap_read(rdev->regmap, reg_data->fast_reg, >> + &data); >> + if (ret < 0) >> + return ret; >> + >> + sel *= reg_data->uA_step / 1000; /* convert to mA */ >> + data &= ~reg_data->fast_mask; >> + data |= sel * 10 / 333; /* 0.1A/3 steps */ >> + } >> + >> + ret = regmap_write(rdev->regmap, reg_data->fast_reg, data); >> + if (ret < 0) >> + return ret; >> + } >> + >> + return 0; >> } >> /* end of CHARGER regulator ops */ >> >> @@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = { >> .linear_mask = CHG_CNFG_09_CHGIN_ILIM_MASK, >> .uA_step = 20000, >> .min_sel = 3, >> + .set_fast = true, >> + .fast_reg = MAX77693_CHG_REG_CHG_CNFG_02, >> + .fast_mask = CHG_CNFG_02_CC_MASK, >> }; >> >> #define max77843_regulator_desc_esafeout(num) { \ >> @@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = { >> .linear_mask = MAX77843_CHG_FAST_CHG_CURRENT_MASK, >> .uA_step = MAX77843_CHG_FAST_CHG_CURRENT_STEP, >> .min_sel = 2, >> + .set_fast = false, >> }; >> >> static int max77693_pmic_probe(struct platform_device *pdev) >> -- >> 2.8.0.rc3 >>
On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote: > When charging is disabled (e.g. by removing the USB cable) the charge > current is not reset to zero. So if I expose the current by the > CURRENT_NOW property, it incorrectly reports the current that was set > when charging was enabled, although there is no charging going on > anymore. So I felt the need to update the charge current every time the > charger gets enabled or disabled. > Initially, the charge current is set to zero, so I think it needs to be > set at least at the beginning to enable charging. Are you sure that the register value you're looking at is the actual charge current right now and not just the maximum that the charger will try to use depending on the conditions (supply available, battery state...)? It seems like you're acting as though it's the latter but that's not what the chip is doing.
Mark Brown writes: > On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote: > >> When charging is disabled (e.g. by removing the USB cable) the charge >> current is not reset to zero. So if I expose the current by the >> CURRENT_NOW property, it incorrectly reports the current that was set >> when charging was enabled, although there is no charging going on >> anymore. So I felt the need to update the charge current every time the >> charger gets enabled or disabled. >> Initially, the charge current is set to zero, so I think it needs to be >> set at least at the beginning to enable charging. > > Are you sure that the register value you're looking at is the actual > charge current right now and not just the maximum that the charger will > try to use depending on the conditions (supply available, battery > state...)? It seems like you're acting as though it's the latter but > that's not what the chip is doing. I was looking at the vendor code that was released for the Galaxy S3 and there the same register gets accessed for getting the current for the CURRENT_NOW property [1] and for setting the current [2]. So is this probably the wrong use of the CURRENT_NOW property because not the actual charge current is read but the maximum value that was set? Unfortunately, I don't have access to the datasheet and I didn't find it online so I don't know where the actual current can be accessed. Thanks, Wolfgang [1] https://code.fossencdi.org/kernel_samsung_smdk4412.git/tree/drivers/battery/max77693_charger.c#n531 [2] https://code.fossencdi.org/kernel_samsung_smdk4412.git/tree/drivers/battery/max77693_charger.c#n552
On 09/27/2016 07:51 PM, Wolfgang Wiedmeyer wrote: > > Mark Brown writes: > >> On Tue, Sep 27, 2016 at 03:50:42PM +0200, Wolfgang Wiedmeyer wrote: >> >>> When charging is disabled (e.g. by removing the USB cable) the charge >>> current is not reset to zero. So if I expose the current by the >>> CURRENT_NOW property, it incorrectly reports the current that was set >>> when charging was enabled, although there is no charging going on >>> anymore. So I felt the need to update the charge current every time the >>> charger gets enabled or disabled. >>> Initially, the charge current is set to zero, so I think it needs to be >>> set at least at the beginning to enable charging. >> >> Are you sure that the register value you're looking at is the actual >> charge current right now and not just the maximum that the charger will >> try to use depending on the conditions (supply available, battery >> state...)? It seems like you're acting as though it's the latter but >> that's not what the chip is doing. > > I was looking at the vendor code that was released for the Galaxy S3 and > there the same register gets accessed for getting the current for > the CURRENT_NOW property [1] and for setting the current [2]. So is this > probably the wrong use of the CURRENT_NOW property because not the > actual charge current is read but the maximum value that was > set? Yes, reading from this register will give only information about currently set charge current. Not the real current. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/regulator/max77693-regulator.c b/drivers/regulator/max77693-regulator.c index cfbb951..e2f7584 100644 --- a/drivers/regulator/max77693-regulator.c +++ b/drivers/regulator/max77693-regulator.c @@ -54,14 +54,19 @@ struct chg_reg_data { unsigned int linear_mask; unsigned int uA_step; unsigned int min_sel; + + bool set_fast; + unsigned int fast_reg; + unsigned int fast_mask; }; /* * MAX77693 CHARGER regulator - Min : 20mA, Max : 2580mA, step : 20mA * 0x00, 0x01, 0x2, 0x03 = 60 mA * 0x04 ~ 0x7E = (60 + (X - 3) * 20) mA - * Actually for MAX77693 the driver manipulates the maximum input current, - * not the fast charge current (output). This should be fixed. + * Actually for MAX77693 the driver manipulates the maximum input current + * and the fast charge current (output) because the fast charge current + * is not set. * * On MAX77843 the calculation formula is the same (except values). * Fortunately it properly manipulates the fast charge current. @@ -100,6 +105,8 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, const struct chg_reg_data *reg_data = rdev_get_drvdata(rdev); unsigned int chg_min_uA = rdev->constraints->min_uA; int sel = 0; + unsigned int data; + int ret; while (chg_min_uA + reg_data->uA_step * sel < min_uA) sel++; @@ -110,7 +117,35 @@ static int max77693_chg_set_current_limit(struct regulator_dev *rdev, /* the first four codes for charger current are all 60mA */ sel += reg_data->min_sel; - return regmap_write(rdev->regmap, reg_data->linear_reg, sel); + ret = regmap_write(rdev->regmap, reg_data->linear_reg, sel); + if (ret < 0) + return ret; + + if (reg_data->set_fast) { + /* disable fast charge if minimum value */ + if (sel == reg_data->min_sel) + data = 0; + else { + /* + * set the fast charge current to the closest value + * below the input current + */ + ret = regmap_read(rdev->regmap, reg_data->fast_reg, + &data); + if (ret < 0) + return ret; + + sel *= reg_data->uA_step / 1000; /* convert to mA */ + data &= ~reg_data->fast_mask; + data |= sel * 10 / 333; /* 0.1A/3 steps */ + } + + ret = regmap_write(rdev->regmap, reg_data->fast_reg, data); + if (ret < 0) + return ret; + } + + return 0; } /* end of CHARGER regulator ops */ @@ -197,6 +232,9 @@ static const struct chg_reg_data max77693_chg_reg_data = { .linear_mask = CHG_CNFG_09_CHGIN_ILIM_MASK, .uA_step = 20000, .min_sel = 3, + .set_fast = true, + .fast_reg = MAX77693_CHG_REG_CHG_CNFG_02, + .fast_mask = CHG_CNFG_02_CC_MASK, }; #define max77843_regulator_desc_esafeout(num) { \ @@ -237,6 +275,7 @@ static const struct chg_reg_data max77843_chg_reg_data = { .linear_mask = MAX77843_CHG_FAST_CHG_CURRENT_MASK, .uA_step = MAX77843_CHG_FAST_CHG_CURRENT_STEP, .min_sel = 2, + .set_fast = false, }; static int max77693_pmic_probe(struct platform_device *pdev)
For MAX77693, the fast charge current also needs to be manipulated for proper charging. The fast charge current is only set in the case of the MAX77693 type, as the MAX77843 properly manipulates the fast charge current. The fast charge current is set to the next possible value below the maximum input current. Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de> --- drivers/regulator/max77693-regulator.c | 45 +++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-)