linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] smb347_charger: fix battery status reporting logic for charger faults
@ 2012-07-20 13:52 Ramakrishna Pallala
  2012-08-13  8:16 ` Mika Westerberg
  0 siblings, 1 reply; 3+ messages in thread
From: Ramakrishna Pallala @ 2012-07-20 13:52 UTC (permalink / raw)
  To: linux-kernel
  Cc: Anton Vorontsov, Anton Vorontsov, Ramakrishna Pallala, Mika Westerberg

This patch checks for charger status register for determining the
battery charging status and reports Discharing/Charging/Not Charging/Full
accordingly.

This patch also adds the interrupt support for Safety Timer Expiration.
This interrupt is helpful in debugging the cause for charger fault.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/power/smb347-charger.c |   96 +++++++++++++++++++++++++++++++++------
 1 files changed, 81 insertions(+), 15 deletions(-)

diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
index 332dd01..b6a8c59 100644
--- a/drivers/power/smb347-charger.c
+++ b/drivers/power/smb347-charger.c
@@ -80,6 +80,7 @@
 #define CFG_FAULT_IRQ_DCIN_UV			BIT(2)
 #define CFG_STATUS_IRQ				0x0d
 #define CFG_STATUS_IRQ_TERMINATION_OR_TAPER	BIT(4)
+#define CFG_STATUS_IRQ_CHARGE_TIMEOUT		BIT(7)
 #define CFG_ADDRESS				0x0e
 
 /* Command registers */
@@ -96,6 +97,9 @@
 #define IRQSTAT_C_TERMINATION_STAT		BIT(0)
 #define IRQSTAT_C_TERMINATION_IRQ		BIT(1)
 #define IRQSTAT_C_TAPER_IRQ			BIT(3)
+#define IRQSTAT_D				0x38
+#define IRQSTAT_D_CHARGE_TIMEOUT_STAT		BIT(2)
+#define IRQSTAT_D_CHARGE_TIMEOUT_IRQ		BIT(3)
 #define IRQSTAT_E				0x39
 #define IRQSTAT_E_USBIN_UV_STAT			BIT(0)
 #define IRQSTAT_E_USBIN_UV_IRQ			BIT(1)
@@ -109,8 +113,10 @@
 #define STAT_B					0x3c
 #define STAT_C					0x3d
 #define STAT_C_CHG_ENABLED			BIT(0)
+#define STAT_C_HOLDOFF_STAT			BIT(3)
 #define STAT_C_CHG_MASK				0x06
 #define STAT_C_CHG_SHIFT			1
+#define STAT_C_CHG_TERM				BIT(5)
 #define STAT_C_CHARGER_ERROR			BIT(6)
 #define STAT_E					0x3f
 
@@ -701,7 +707,7 @@ fail:
 static irqreturn_t smb347_interrupt(int irq, void *data)
 {
 	struct smb347_charger *smb = data;
-	unsigned int stat_c, irqstat_e, irqstat_c;
+	unsigned int stat_c, irqstat_c, irqstat_d, irqstat_e;
 	bool handled = false;
 	int ret;
 
@@ -717,6 +723,12 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 		return IRQ_NONE;
 	}
 
+	ret = regmap_read(smb->regmap, IRQSTAT_D, &irqstat_d);
+	if (ret < 0) {
+		dev_warn(smb->dev, "reading IRQSTAT_D failed\n");
+		return IRQ_NONE;
+	}
+
 	ret = regmap_read(smb->regmap, IRQSTAT_E, &irqstat_e);
 	if (ret < 0) {
 		dev_warn(smb->dev, "reading IRQSTAT_E failed\n");
@@ -724,13 +736,11 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	}
 
 	/*
-	 * If we get charger error we report the error back to user and
-	 * disable charging.
+	 * If we get charger error we report the error back to user.
+	 * If the error is recovered charging will resume again.
 	 */
 	if (stat_c & STAT_C_CHARGER_ERROR) {
-		dev_err(smb->dev, "error in charger, disabling charging\n");
-
-		smb347_charging_disable(smb);
+		dev_err(smb->dev, "charging stopped due to charger error\n");
 		power_supply_changed(&smb->battery);
 		handled = true;
 	}
@@ -743,6 +753,20 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
 	if (irqstat_c & (IRQSTAT_C_TERMINATION_IRQ | IRQSTAT_C_TAPER_IRQ)) {
 		if (irqstat_c & IRQSTAT_C_TERMINATION_STAT)
 			power_supply_changed(&smb->battery);
+		dev_dbg(smb->dev, "going to HW maintenance mode\n");
+		handled = true;
+	}
+
+	/*
+	 * If we got a charger timeout INT that means the charge
+	 * full is not detected with in charge timeout value.
+	 */
+	if (irqstat_d & IRQSTAT_D_CHARGE_TIMEOUT_IRQ) {
+		dev_dbg(smb->dev, "total Charge Timeout INT recieved\n");
+
+		if (irqstat_d & IRQSTAT_D_CHARGE_TIMEOUT_STAT)
+			dev_warn(smb->dev, "charging stopped due to timeout\n");
+		power_supply_changed(&smb->battery);
 		handled = true;
 	}
 
@@ -776,6 +800,7 @@ static int smb347_irq_set(struct smb347_charger *smb, bool enable)
 	 * Enable/disable interrupts for:
 	 *	- under voltage
 	 *	- termination current reached
+	 *	- charger timeout
 	 *	- charger error
 	 */
 	ret = regmap_update_bits(smb->regmap, CFG_FAULT_IRQ, 0xff,
@@ -784,7 +809,8 @@ static int smb347_irq_set(struct smb347_charger *smb, bool enable)
 		goto fail;
 
 	ret = regmap_update_bits(smb->regmap, CFG_STATUS_IRQ, 0xff,
-			enable ? CFG_STATUS_IRQ_TERMINATION_OR_TAPER : 0);
+			enable ? (CFG_STATUS_IRQ_TERMINATION_OR_TAPER |
+					CFG_STATUS_IRQ_CHARGE_TIMEOUT) : 0);
 	if (ret < 0)
 		goto fail;
 
@@ -988,6 +1014,50 @@ static enum power_supply_property smb347_usb_properties[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 };
 
+static int smb347_get_charging_status(struct smb347_charger *smb)
+{
+	int ret, status;
+	unsigned int val;
+
+	if (!smb)
+		return -ENODEV;
+
+	if (!smb347_is_ps_online(smb))
+		return POWER_SUPPLY_STATUS_DISCHARGING;
+
+	ret = regmap_read(smb->regmap, STAT_C, &val);
+	if (ret < 0)
+		return ret;
+
+	if ((val & STAT_C_CHARGER_ERROR) ||
+		(val & STAT_C_HOLDOFF_STAT)) {
+		/* set to NOT CHARGING upon charger error
+		 * or charging has stopped.
+		 */
+		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+	} else {
+		if ((val & STAT_C_CHG_MASK) >> STAT_C_CHG_SHIFT) {
+			/* set to charging if battery is in pre-charge,
+			 * fast charge or taper charging mode.
+			 */
+			status = POWER_SUPPLY_STATUS_CHARGING;
+		} else if (val & STAT_C_CHG_TERM) {
+			/* set the status to FULL if battery is not in pre
+			 * charge, fast charge or taper charging mode AND
+			 * charging is terminated at least once.
+			 */
+			status = POWER_SUPPLY_STATUS_FULL;
+		} else {
+			/* in this case no charger error or termination
+			 * occured but charging is not in progress!!!
+			 */
+			status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		}
+	}
+
+	return status;
+}
+
 static int smb347_battery_get_property(struct power_supply *psy,
 				       enum power_supply_property prop,
 				       union power_supply_propval *val)
@@ -1003,14 +1073,10 @@ static int smb347_battery_get_property(struct power_supply *psy,
 
 	switch (prop) {
 	case POWER_SUPPLY_PROP_STATUS:
-		if (!smb347_is_ps_online(smb)) {
-			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
-			break;
-		}
-		if (smb347_charging_status(smb))
-			val->intval = POWER_SUPPLY_STATUS_CHARGING;
-		else
-			val->intval = POWER_SUPPLY_STATUS_FULL;
+		ret = smb347_get_charging_status(smb);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] smb347_charger: fix battery status reporting logic for charger faults
  2012-07-20 13:52 [PATCH] smb347_charger: fix battery status reporting logic for charger faults Ramakrishna Pallala
@ 2012-08-13  8:16 ` Mika Westerberg
  2012-08-15  2:26   ` Pallala, Ramakrishna
  0 siblings, 1 reply; 3+ messages in thread
From: Mika Westerberg @ 2012-08-13  8:16 UTC (permalink / raw)
  To: Ramakrishna Pallala; +Cc: linux-kernel, Anton Vorontsov, Anton Vorontsov

On Fri, Jul 20, 2012 at 07:22:48PM +0530, Ramakrishna Pallala wrote:
> This patch checks for charger status register for determining the
> battery charging status and reports Discharing/Charging/Not Charging/Full
> accordingly.
> 
> This patch also adds the interrupt support for Safety Timer Expiration.
> This interrupt is helpful in debugging the cause for charger fault.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

Few nitpicks below, otherwise looks good to me.

> ---
>  drivers/power/smb347-charger.c |   96 +++++++++++++++++++++++++++++++++------
>  1 files changed, 81 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
> index 332dd01..b6a8c59 100644
> --- a/drivers/power/smb347-charger.c
> +++ b/drivers/power/smb347-charger.c
> @@ -80,6 +80,7 @@
>  #define CFG_FAULT_IRQ_DCIN_UV			BIT(2)
>  #define CFG_STATUS_IRQ				0x0d
>  #define CFG_STATUS_IRQ_TERMINATION_OR_TAPER	BIT(4)
> +#define CFG_STATUS_IRQ_CHARGE_TIMEOUT		BIT(7)
>  #define CFG_ADDRESS				0x0e
>  
>  /* Command registers */
> @@ -96,6 +97,9 @@
>  #define IRQSTAT_C_TERMINATION_STAT		BIT(0)
>  #define IRQSTAT_C_TERMINATION_IRQ		BIT(1)
>  #define IRQSTAT_C_TAPER_IRQ			BIT(3)
> +#define IRQSTAT_D				0x38
> +#define IRQSTAT_D_CHARGE_TIMEOUT_STAT		BIT(2)
> +#define IRQSTAT_D_CHARGE_TIMEOUT_IRQ		BIT(3)
>  #define IRQSTAT_E				0x39
>  #define IRQSTAT_E_USBIN_UV_STAT			BIT(0)
>  #define IRQSTAT_E_USBIN_UV_IRQ			BIT(1)
> @@ -109,8 +113,10 @@
>  #define STAT_B					0x3c
>  #define STAT_C					0x3d
>  #define STAT_C_CHG_ENABLED			BIT(0)
> +#define STAT_C_HOLDOFF_STAT			BIT(3)
>  #define STAT_C_CHG_MASK				0x06
>  #define STAT_C_CHG_SHIFT			1
> +#define STAT_C_CHG_TERM				BIT(5)
>  #define STAT_C_CHARGER_ERROR			BIT(6)
>  #define STAT_E					0x3f
>  
> @@ -701,7 +707,7 @@ fail:
>  static irqreturn_t smb347_interrupt(int irq, void *data)
>  {
>  	struct smb347_charger *smb = data;
> -	unsigned int stat_c, irqstat_e, irqstat_c;
> +	unsigned int stat_c, irqstat_c, irqstat_d, irqstat_e;
>  	bool handled = false;
>  	int ret;
>  
> @@ -717,6 +723,12 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
>  		return IRQ_NONE;
>  	}
>  
> +	ret = regmap_read(smb->regmap, IRQSTAT_D, &irqstat_d);
> +	if (ret < 0) {
> +		dev_warn(smb->dev, "reading IRQSTAT_D failed\n");
> +		return IRQ_NONE;
> +	}
> +
>  	ret = regmap_read(smb->regmap, IRQSTAT_E, &irqstat_e);
>  	if (ret < 0) {
>  		dev_warn(smb->dev, "reading IRQSTAT_E failed\n");
> @@ -724,13 +736,11 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
>  	}
>  
>  	/*
> -	 * If we get charger error we report the error back to user and
> -	 * disable charging.
> +	 * If we get charger error we report the error back to user.
> +	 * If the error is recovered charging will resume again.
>  	 */
>  	if (stat_c & STAT_C_CHARGER_ERROR) {
> -		dev_err(smb->dev, "error in charger, disabling charging\n");
> -
> -		smb347_charging_disable(smb);
> +		dev_err(smb->dev, "charging stopped due to charger error\n");
>  		power_supply_changed(&smb->battery);
>  		handled = true;
>  	}
> @@ -743,6 +753,20 @@ static irqreturn_t smb347_interrupt(int irq, void *data)
>  	if (irqstat_c & (IRQSTAT_C_TERMINATION_IRQ | IRQSTAT_C_TAPER_IRQ)) {
>  		if (irqstat_c & IRQSTAT_C_TERMINATION_STAT)
>  			power_supply_changed(&smb->battery);
> +		dev_dbg(smb->dev, "going to HW maintenance mode\n");
> +		handled = true;
> +	}
> +
> +	/*
> +	 * If we got a charger timeout INT that means the charge
> +	 * full is not detected with in charge timeout value.
> +	 */
> +	if (irqstat_d & IRQSTAT_D_CHARGE_TIMEOUT_IRQ) {
> +		dev_dbg(smb->dev, "total Charge Timeout INT recieved\n");

received not recieved

> +
> +		if (irqstat_d & IRQSTAT_D_CHARGE_TIMEOUT_STAT)
> +			dev_warn(smb->dev, "charging stopped due to timeout\n");
> +		power_supply_changed(&smb->battery);
>  		handled = true;
>  	}
>  
> @@ -776,6 +800,7 @@ static int smb347_irq_set(struct smb347_charger *smb, bool enable)
>  	 * Enable/disable interrupts for:
>  	 *	- under voltage
>  	 *	- termination current reached
> +	 *	- charger timeout
>  	 *	- charger error
>  	 */
>  	ret = regmap_update_bits(smb->regmap, CFG_FAULT_IRQ, 0xff,
> @@ -784,7 +809,8 @@ static int smb347_irq_set(struct smb347_charger *smb, bool enable)
>  		goto fail;
>  
>  	ret = regmap_update_bits(smb->regmap, CFG_STATUS_IRQ, 0xff,
> -			enable ? CFG_STATUS_IRQ_TERMINATION_OR_TAPER : 0);
> +			enable ? (CFG_STATUS_IRQ_TERMINATION_OR_TAPER |
> +					CFG_STATUS_IRQ_CHARGE_TIMEOUT) : 0);
>  	if (ret < 0)
>  		goto fail;
>  
> @@ -988,6 +1014,50 @@ static enum power_supply_property smb347_usb_properties[] = {
>  	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>  };
>  
> +static int smb347_get_charging_status(struct smb347_charger *smb)
> +{
> +	int ret, status;
> +	unsigned int val;
> +
> +	if (!smb)
> +		return -ENODEV;

I don't think the above check is necessary.

> +
> +	if (!smb347_is_ps_online(smb))
> +		return POWER_SUPPLY_STATUS_DISCHARGING;
> +
> +	ret = regmap_read(smb->regmap, STAT_C, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((val & STAT_C_CHARGER_ERROR) ||
> +		(val & STAT_C_HOLDOFF_STAT)) {
> +		/* set to NOT CHARGING upon charger error
> +		 * or charging has stopped.
> +		 */

Please use block comments defined in Documentation/CodingStyle.

> +		status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +	} else {
> +		if ((val & STAT_C_CHG_MASK) >> STAT_C_CHG_SHIFT) {
> +			/* set to charging if battery is in pre-charge,
> +			 * fast charge or taper charging mode.
> +			 */

ditto

> +			status = POWER_SUPPLY_STATUS_CHARGING;
> +		} else if (val & STAT_C_CHG_TERM) {
> +			/* set the status to FULL if battery is not in pre
> +			 * charge, fast charge or taper charging mode AND
> +			 * charging is terminated at least once.
> +			 */
> +			status = POWER_SUPPLY_STATUS_FULL;
> +		} else {
> +			/* in this case no charger error or termination
> +			 * occured but charging is not in progress!!!
> +			 */

ditto

> +			status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		}
> +	}
> +
> +	return status;
> +}
> +
>  static int smb347_battery_get_property(struct power_supply *psy,
>  				       enum power_supply_property prop,
>  				       union power_supply_propval *val)
> @@ -1003,14 +1073,10 @@ static int smb347_battery_get_property(struct power_supply *psy,
>  
>  	switch (prop) {
>  	case POWER_SUPPLY_PROP_STATUS:
> -		if (!smb347_is_ps_online(smb)) {
> -			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> -			break;
> -		}
> -		if (smb347_charging_status(smb))
> -			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> -		else
> -			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		ret = smb347_get_charging_status(smb);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = ret;
>  		break;
>  
>  	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> -- 
> 1.7.0.4

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH] smb347_charger: fix battery status reporting logic for charger faults
  2012-08-13  8:16 ` Mika Westerberg
@ 2012-08-15  2:26   ` Pallala, Ramakrishna
  0 siblings, 0 replies; 3+ messages in thread
From: Pallala, Ramakrishna @ 2012-08-15  2:26 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: linux-kernel, Anton Vorontsov, Anton Vorontsov



> On Fri, Jul 20, 2012 at 07:22:48PM +0530, Ramakrishna Pallala wrote:
> > This patch checks for charger status register for determining the
> > battery charging status and reports Discharing/Charging/Not
> > Charging/Full accordingly.
> >
> > This patch also adds the interrupt support for Safety Timer Expiration.
> > This interrupt is helpful in debugging the cause for charger fault.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> 
> Few nitpicks below, otherwise looks good to me.

Thanks for the comments. I will fix them.

Thanks,
Ram


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2012-08-15  2:26 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-20 13:52 [PATCH] smb347_charger: fix battery status reporting logic for charger faults Ramakrishna Pallala
2012-08-13  8:16 ` Mika Westerberg
2012-08-15  2:26   ` Pallala, Ramakrishna

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).