diff mbox series

[RFC] Limiting charge current on Droid 4 (and N900)

Message ID 20200615140557.GA22781@duo.ucw.cz (mailing list archive)
State New, archived
Headers show
Series [RFC] Limiting charge current on Droid 4 (and N900) | expand

Commit Message

Pavel Machek June 15, 2020, 2:05 p.m. UTC 
Hi!

Droid 4 has same problem as N900: it is often neccessary to manually
tweak current draw from USB, for example when using thin charging cable.

N900 creates unique attribute by hand, but I believe
POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
converted?)

Comments? Would the patch be acceptable after fixing whitespace?

Best regards,
									Pavel

Signed-off-by: Pavel Machek <pavel@ucw.cz>

Comments

Merlijn Wajer June 27, 2020, 11:01 a.m. UTC | #1 
Hi Pavel,

On 15/06/2020 16:05, Pavel Machek wrote:
> Hi!
> 
> Droid 4 has same problem as N900: it is often neccessary to manually
> tweak current draw from USB, for example when using thin charging cable.
> 
> N900 creates unique attribute by hand, but I believe
> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
> converted?)
> 
> Comments? Would the patch be acceptable after fixing whitespace?

I'm not very knowledgeable on batteries - but the patch looks good to me.

Could you perhaps explain what exactly this fixes? I've seen some
interesting behaviour when plugging a Droid 4 into a PC (or wall
charger, really): the led blinks for a few seconds until it stabilises.

And then there's the issue where, once the battery is full, it will
switch between charging and discharging every few seconds/minutes.

Cheers,
Merlijn

> 
> Best regards,
> 									Pavel
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
> index b16848cfb58c..39a00716372f 100644
> --- a/drivers/power/supply/cpcap-battery.c
> +++ b/drivers/power/supply/cpcap-battery.c
> @@ -3,7 +3,7 @@
>   *
>   * Copyright (C) 2017 Tony Lindgren <tony@atomide.com>
>   *
> - * Some parts of the code based on earlie Motorola mapphone Linux kernel
> + * Some parts of the code based on earlier Motorola mapphone Linux kernel
>   * drivers:
>   *
>   * Copyright (C) 2009-2010 Motorola, Inc.
> diff --git a/drivers/power/supply/cpcap-charger.c b/drivers/power/supply/cpcap-charger.c
> index cf1e05b511d9..292d7a31c663 100644
> --- a/drivers/power/supply/cpcap-charger.c
> +++ b/drivers/power/supply/cpcap-charger.c
> @@ -89,6 +89,8 @@
>   * CPCAP_REG_CRM charge currents. These seem to match MC13783UG.pdf
>   * values in "Table 8-3. Charge Path Regulator Current Limit
>   * Characteristics" for the nominal values.
> + *
> + * Except 70mA and 1.596A and unlimited, these are simply 88.7mA / step.
>   */
>  #define CPCAP_REG_CRM_ICHRG(val)	(((val) & 0xf) << 0)
>  #define CPCAP_REG_CRM_ICHRG_0A000	CPCAP_REG_CRM_ICHRG(0x0)
> @@ -147,6 +149,8 @@ struct cpcap_charger_ddata {
>  	int status;
>  	int state;
>  	int voltage;
> +	int limit_current;
> +
>  	int last_current;
>  	int last_current_retries;
>  };
> @@ -175,6 +179,7 @@ static enum power_supply_property cpcap_charger_props[] = {
>  	POWER_SUPPLY_PROP_STATUS,
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>  	POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
>  };
> @@ -238,6 +243,9 @@ static int cpcap_charger_get_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_STATUS:
>  		val->intval = ddata->status;
>  		break;
> +  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		val->intval = ddata->limit_current;
> +		break;
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>  		val->intval = ddata->voltage;
>  		break;
> @@ -308,6 +316,25 @@ cpcap_charger_get_bat_const_charge_voltage(struct cpcap_charger_ddata *ddata)
>  	return voltage;
>  }
>  
> +static int cpcap_charger_current_to_regval(int microamp)
> +{
> +	int miliamp = microamp/1000;
> +	int res;
> +	if (miliamp < 0)
> +		return -EINVAL;
> +	if (miliamp < 70)
> +		return CPCAP_REG_CRM_ICHRG(0x0);
> +	if (miliamp < 177)
> +		return CPCAP_REG_CRM_ICHRG(0x1);
> +	if (miliamp > 1596)
> +		return CPCAP_REG_CRM_ICHRG(0xe);
> +
> +	res = microamp / 88666;
> +	if (res > 0xd)
> +		res = 0xd;
> +	return CPCAP_REG_CRM_ICHRG(res);
> +}
> +
>  static int cpcap_charger_set_property(struct power_supply *psy,
>  				      enum power_supply_property psp,
>  				      const union power_supply_propval *val)
> @@ -316,6 +343,12 @@ static int cpcap_charger_set_property(struct power_supply *psy,
>  	int voltage, batvolt;
>  
>  	switch (psp) {
> +  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> +		if (cpcap_charger_current_to_regval(val->intval) < 0)
> +			return -EINVAL;
> +		ddata->limit_current = val->intval;
> +		schedule_delayed_work(&ddata->detect_work, 0);
> +		break;
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>  		voltage = cpcap_charger_match_voltage(val->intval);
>  		batvolt = cpcap_charger_get_bat_const_charge_voltage(ddata);
> @@ -335,6 +368,7 @@ static int cpcap_charger_property_is_writeable(struct power_supply *psy,
>  					       enum power_supply_property psp)
>  {
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>  	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>  		return 1;
>  	default:
> @@ -657,23 +691,21 @@ static void cpcap_usb_detect(struct work_struct *work)
>  
>  	if (!ddata->feeding_vbus && cpcap_charger_vbus_valid(ddata) &&
>  	    s.chrgcurr1) {
> -		int max_current;
> -		int vchrg;
> +		int max_current = 532000;
> +		int vchrg, ichrg;
>  
>  		if (cpcap_charger_battery_found(ddata))
> -			max_current = CPCAP_REG_CRM_ICHRG_1A596;
> -		else
> -			max_current = CPCAP_REG_CRM_ICHRG_0A532;
> +			max_current = 1596000;
>  
>  		switch (ddata->state) {
>  		case CPCAP_CHARGER_DETECTING:
>  			ddata->last_current_retries = 0;
>  			break;
>  		case CPCAP_CHARGER_DISCONNECTED:
> -			if (ddata->last_current > CPCAP_REG_CRM_ICHRG_0A532) {
> +			if (ddata->last_current > 532000) {
>  				/* Attempt current 3 times before lowering */
>  				if (ddata->last_current_retries++ >= 3) {
> -					ddata->last_current--;
> +					ddata->last_current -= 100000;
>  					ddata->last_current_retries = 0;
>  					/* Wait a bit for voltage to ramp up */
>  					usleep_range(40000, 50000);
> @@ -688,11 +720,16 @@ static void cpcap_usb_detect(struct work_struct *work)
>  			break;
>  		}
>  
> +		if (max_current > ddata->limit_current)
> +			max_current = ddata->limit_current;
> +
>  		ddata->last_current = max_current;
> +
> +		ichrg = cpcap_charger_current_to_regval(max_current);
>  		vchrg = cpcap_charger_voltage_to_regval(ddata->voltage);
>  		error = cpcap_charger_set_state(ddata,
>  						CPCAP_REG_CRM_VCHRG(vchrg),
> -						max_current, 0);
> +						ichrg, 0);
>  		if (error)
>  			goto out_err;
>  		cpcap_charger_update_state(ddata, CPCAP_CHARGER_CHARGING);
> @@ -864,6 +901,7 @@ static int cpcap_charger_probe(struct platform_device *pdev)
>  
>  	ddata->dev = &pdev->dev;
>  	ddata->voltage = 4200000;
> +	ddata->limit_current = 532000;
>  
>  	ddata->reg = dev_get_regmap(ddata->dev->parent, NULL);
>  	if (!ddata->reg)
>
Tony Lindgren June 29, 2020, 3:55 p.m. UTC | #2 
* Pavel Machek <pavel@ucw.cz> [200615 07:06]:
> Hi!
> 
> Droid 4 has same problem as N900: it is often neccessary to manually
> tweak current draw from USB, for example when using thin charging cable.
> 
> N900 creates unique attribute by hand, but I believe
> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
> converted?)
> 
> Comments? Would the patch be acceptable after fixing whitespace?

Looks OK to me. Until we have better charger vs host vs usb3 charging hub
detection in place this seems like a good thing to do.

Regards,

Tony
Pavel Machek June 29, 2020, 9:49 p.m. UTC | #3 
Hi!

> > Droid 4 has same problem as N900: it is often neccessary to manually
> > tweak current draw from USB, for example when using thin charging cable.
> > 
> > N900 creates unique attribute by hand, but I believe
> > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
> > converted?)
> > 
> > Comments? Would the patch be acceptable after fixing whitespace?
> 
> I'm not very knowledgeable on batteries - but the patch looks good to me.
> 
> Could you perhaps explain what exactly this fixes? I've seen some
> interesting behaviour when plugging a Droid 4 into a PC (or wall
> charger, really): the led blinks for a few seconds until it
> stabilises.

With this patch, we'll limit charging to 0.5A by default, unless
overrident by user. So you should not see green LED blinking, unless
you manually select bigger current than charger can handle.

> And then there's the issue where, once the battery is full, it will
> switch between charging and discharging every few seconds/minutes.

This will definitely not help with this one.

Best regards,
									Pavel
Tony Lindgren Aug. 20, 2020, 4:15 a.m. UTC | #4 
* Tony Lindgren <tony@atomide.com> [200629 18:46]:
> * Pavel Machek <pavel@ucw.cz> [200615 07:06]:
> > Hi!
> > 
> > Droid 4 has same problem as N900: it is often neccessary to manually
> > tweak current draw from USB, for example when using thin charging cable.
> > 
> > N900 creates unique attribute by hand, but I believe
> > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
> > converted?)
> > 
> > Comments? Would the patch be acceptable after fixing whitespace?
> 
> Looks OK to me. Until we have better charger vs host vs usb3 charging hub
> detection in place this seems like a good thing to do.

FYI, I'm cleaning up the pending charger and battery patches to send out
for review. So that includes my earlier RFC battery status patches, and
Spinal's additions, and this patch. It will likely be several days before
I have the series ready for posting though.

Regards,

Tony
Pavel Machek Aug. 20, 2020, 6:58 a.m. UTC | #5 
Hi!

> > > Droid 4 has same problem as N900: it is often neccessary to manually
> > > tweak current draw from USB, for example when using thin charging cable.
> > > 
> > > N900 creates unique attribute by hand, but I believe
> > > POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT looks suitable. (Should N900 be
> > > converted?)
> > > 
> > > Comments? Would the patch be acceptable after fixing whitespace?
> > 
> > Looks OK to me. Until we have better charger vs host vs usb3 charging hub
> > detection in place this seems like a good thing to do.
> 
> FYI, I'm cleaning up the pending charger and battery patches to send out
> for review. So that includes my earlier RFC battery status patches, and
> Spinal's additions, and this patch. It will likely be several days before
> I have the series ready for posting though.

Thanks for heads-up.

I had issue when I could not charge _empty_ droid4 battery from
powerbank. Green light was blinking and current was going up and down
but never stabilized. I thought I had dmesg from that but can't find
it now :-(.

Plus, I left D4 on charger overnight, and found battery empty in the
morning. It apparently charged in the evening but then discharged.

Best regards,
									Pavel
diff mbox series

Patch

diff --git a/drivers/power/supply/cpcap-battery.c b/drivers/power/supply/cpcap-battery.c
index b16848cfb58c..39a00716372f 100644
--- a/drivers/power/supply/cpcap-battery.c
+++ b/drivers/power/supply/cpcap-battery.c
@@ -3,7 +3,7 @@ 
  *
  * Copyright (C) 2017 Tony Lindgren <tony@atomide.com>
  *
- * Some parts of the code based on earlie Motorola mapphone Linux kernel
+ * Some parts of the code based on earlier Motorola mapphone Linux kernel
  * drivers:
  *
  * Copyright (C) 2009-2010 Motorola, Inc.
diff --git a/drivers/power/supply/cpcap-charger.c b/drivers/power/supply/cpcap-charger.c
index cf1e05b511d9..292d7a31c663 100644
--- a/drivers/power/supply/cpcap-charger.c
+++ b/drivers/power/supply/cpcap-charger.c
@@ -89,6 +89,8 @@ 
  * CPCAP_REG_CRM charge currents. These seem to match MC13783UG.pdf
  * values in "Table 8-3. Charge Path Regulator Current Limit
  * Characteristics" for the nominal values.
+ *
+ * Except 70mA and 1.596A and unlimited, these are simply 88.7mA / step.
  */
 #define CPCAP_REG_CRM_ICHRG(val)	(((val) & 0xf) << 0)
 #define CPCAP_REG_CRM_ICHRG_0A000	CPCAP_REG_CRM_ICHRG(0x0)
@@ -147,6 +149,8 @@  struct cpcap_charger_ddata {
 	int status;
 	int state;
 	int voltage;
+	int limit_current;
+
 	int last_current;
 	int last_current_retries;
 };
@@ -175,6 +179,7 @@  static enum power_supply_property cpcap_charger_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 };
@@ -238,6 +243,9 @@  static int cpcap_charger_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_STATUS:
 		val->intval = ddata->status;
 		break;
+  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		val->intval = ddata->limit_current;
+		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		val->intval = ddata->voltage;
 		break;
@@ -308,6 +316,25 @@  cpcap_charger_get_bat_const_charge_voltage(struct cpcap_charger_ddata *ddata)
 	return voltage;
 }
 
+static int cpcap_charger_current_to_regval(int microamp)
+{
+	int miliamp = microamp/1000;
+	int res;
+	if (miliamp < 0)
+		return -EINVAL;
+	if (miliamp < 70)
+		return CPCAP_REG_CRM_ICHRG(0x0);
+	if (miliamp < 177)
+		return CPCAP_REG_CRM_ICHRG(0x1);
+	if (miliamp > 1596)
+		return CPCAP_REG_CRM_ICHRG(0xe);
+
+	res = microamp / 88666;
+	if (res > 0xd)
+		res = 0xd;
+	return CPCAP_REG_CRM_ICHRG(res);
+}
+
 static int cpcap_charger_set_property(struct power_supply *psy,
 				      enum power_supply_property psp,
 				      const union power_supply_propval *val)
@@ -316,6 +343,12 @@  static int cpcap_charger_set_property(struct power_supply *psy,
 	int voltage, batvolt;
 
 	switch (psp) {
+  	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		if (cpcap_charger_current_to_regval(val->intval) < 0)
+			return -EINVAL;
+		ddata->limit_current = val->intval;
+		schedule_delayed_work(&ddata->detect_work, 0);
+		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		voltage = cpcap_charger_match_voltage(val->intval);
 		batvolt = cpcap_charger_get_bat_const_charge_voltage(ddata);
@@ -335,6 +368,7 @@  static int cpcap_charger_property_is_writeable(struct power_supply *psy,
 					       enum power_supply_property psp)
 {
 	switch (psp) {
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 		return 1;
 	default:
@@ -657,23 +691,21 @@  static void cpcap_usb_detect(struct work_struct *work)
 
 	if (!ddata->feeding_vbus && cpcap_charger_vbus_valid(ddata) &&
 	    s.chrgcurr1) {
-		int max_current;
-		int vchrg;
+		int max_current = 532000;
+		int vchrg, ichrg;
 
 		if (cpcap_charger_battery_found(ddata))
-			max_current = CPCAP_REG_CRM_ICHRG_1A596;
-		else
-			max_current = CPCAP_REG_CRM_ICHRG_0A532;
+			max_current = 1596000;
 
 		switch (ddata->state) {
 		case CPCAP_CHARGER_DETECTING:
 			ddata->last_current_retries = 0;
 			break;
 		case CPCAP_CHARGER_DISCONNECTED:
-			if (ddata->last_current > CPCAP_REG_CRM_ICHRG_0A532) {
+			if (ddata->last_current > 532000) {
 				/* Attempt current 3 times before lowering */
 				if (ddata->last_current_retries++ >= 3) {
-					ddata->last_current--;
+					ddata->last_current -= 100000;
 					ddata->last_current_retries = 0;
 					/* Wait a bit for voltage to ramp up */
 					usleep_range(40000, 50000);
@@ -688,11 +720,16 @@  static void cpcap_usb_detect(struct work_struct *work)
 			break;
 		}
 
+		if (max_current > ddata->limit_current)
+			max_current = ddata->limit_current;
+
 		ddata->last_current = max_current;
+
+		ichrg = cpcap_charger_current_to_regval(max_current);
 		vchrg = cpcap_charger_voltage_to_regval(ddata->voltage);
 		error = cpcap_charger_set_state(ddata,
 						CPCAP_REG_CRM_VCHRG(vchrg),
-						max_current, 0);
+						ichrg, 0);
 		if (error)
 			goto out_err;
 		cpcap_charger_update_state(ddata, CPCAP_CHARGER_CHARGING);
@@ -864,6 +901,7 @@  static int cpcap_charger_probe(struct platform_device *pdev)
 
 	ddata->dev = &pdev->dev;
 	ddata->voltage = 4200000;
+	ddata->limit_current = 532000;
 
 	ddata->reg = dev_get_regmap(ddata->dev->parent, NULL);
 	if (!ddata->reg)