[PATCH] smb347_charger: cleaned battery power supply attributes

[PATCH] smb347_charger: cleaned battery power supply attributes

[Ramakrishna Pallala]

CURRENT_NOW and VOLTAGE_NOW should be instantaneous readings
from power supply(ex: battery).

smb347 charger driver reports charge voltage for VOLTAGE_NOW
and charge current for CURRENT_NOW attributes which are not
instantaneous readings.

This patch removes the battery VOLTAGE_NOW and CURRENT_NOW
properties from the driver and also removes hw_to_current()
which is not required anymore.

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

diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
index cf31b31..09d19d2 100644
--- a/drivers/power/smb347-charger.c
+++ b/drivers/power/smb347-charger.c
@@ -195,14 +195,6 @@ static const unsigned int ccc_tbl[] = {
 	1200000,
 };
 
-/* Convert register value to current using lookup table */
-static int hw_to_current(const unsigned int *tbl, size_t size, unsigned int val)
-{
-	if (val >= size)
-		return -EINVAL;
-	return tbl[val];
-}
-
 /* Convert current to register value using lookup table */
 static int current_to_hw(const unsigned int *tbl, size_t size, unsigned int val)
 {
@@ -891,7 +883,6 @@ static int smb347_battery_get_property(struct power_supply *psy,
 	struct smb347_charger *smb =
 			container_of(psy, struct smb347_charger, battery);
 	const struct smb347_charger_platform_data *pdata = smb->pdata;
-	unsigned int v;
 	int ret;
 
 	ret = smb347_update_ps_status(smb);
@@ -943,44 +934,6 @@ static int smb347_battery_get_property(struct power_supply *psy,
 		val->intval = pdata->battery_info.voltage_max_design;
 		break;
 
-	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
-		if (!smb347_is_ps_online(smb))
-			return -ENODATA;
-		ret = regmap_read(smb->regmap, STAT_A, &v);
-		if (ret < 0)
-			return ret;
-
-		v &= STAT_A_FLOAT_VOLTAGE_MASK;
-		if (v > 0x3d)
-			v = 0x3d;
-
-		val->intval = 3500000 + v * 20000;
-		break;
-
-	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		if (!smb347_is_ps_online(smb))
-			return -ENODATA;
-
-		ret = regmap_read(smb->regmap, STAT_B, &v);
-		if (ret < 0)
-			return ret;
-
-		/*
-		 * The current value is composition of FCC and PCC values
-		 * and we can detect which table to use from bit 5.
-		 */
-		if (v & 0x20) {
-			val->intval = hw_to_current(fcc_tbl,
-						    ARRAY_SIZE(fcc_tbl),
-						    v & 7);
-		} else {
-			v >>= 3;
-			val->intval = hw_to_current(pcc_tbl,
-						    ARRAY_SIZE(pcc_tbl),
-						    v & 7);
-		}
-		break;
-
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		val->intval = pdata->battery_info.charge_full_design;
 		break;
@@ -1002,8 +955,6 @@ static enum power_supply_property smb347_battery_properties[] = {
 	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 };
-- 
1.7.0.4

Re: [PATCH] smb347_charger: cleaned battery power supply attributes

[Anton Vorontsov]

On Sat, May 05, 2012 at 09:33:54PM +0530, Ramakrishna Pallala wrote:
> CURRENT_NOW and VOLTAGE_NOW should be instantaneous readings
> from power supply(ex: battery).
> 
> smb347 charger driver reports charge voltage for VOLTAGE_NOW
> and charge current for CURRENT_NOW attributes which are not
> instantaneous readings.

Em. While charging, I guess charge current == instantaneous battery
current? Should we then report it in such cases? And while discharging
and not charging, assuming that HW can't report the real battery
current flow, we can return some fancy errno code, like ENODATA?

Thanks,

> This patch removes the battery VOLTAGE_NOW and CURRENT_NOW
> properties from the driver and also removes hw_to_current()
> which is not required anymore.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> ---
>  drivers/power/smb347-charger.c |   49 ----------------------------------------
>  1 files changed, 0 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/power/smb347-charger.c b/drivers/power/smb347-charger.c
> index cf31b31..09d19d2 100644
> --- a/drivers/power/smb347-charger.c
> +++ b/drivers/power/smb347-charger.c
> @@ -195,14 +195,6 @@ static const unsigned int ccc_tbl[] = {
>  	1200000,
>  };
>  
> -/* Convert register value to current using lookup table */
> -static int hw_to_current(const unsigned int *tbl, size_t size, unsigned int val)
> -{
> -	if (val >= size)
> -		return -EINVAL;
> -	return tbl[val];
> -}
> -
>  /* Convert current to register value using lookup table */
>  static int current_to_hw(const unsigned int *tbl, size_t size, unsigned int val)
>  {
> @@ -891,7 +883,6 @@ static int smb347_battery_get_property(struct power_supply *psy,
>  	struct smb347_charger *smb =
>  			container_of(psy, struct smb347_charger, battery);
>  	const struct smb347_charger_platform_data *pdata = smb->pdata;
> -	unsigned int v;
>  	int ret;
>  
>  	ret = smb347_update_ps_status(smb);
> @@ -943,44 +934,6 @@ static int smb347_battery_get_property(struct power_supply *psy,
>  		val->intval = pdata->battery_info.voltage_max_design;
>  		break;
>  
> -	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> -		if (!smb347_is_ps_online(smb))
> -			return -ENODATA;
> -		ret = regmap_read(smb->regmap, STAT_A, &v);
> -		if (ret < 0)
> -			return ret;
> -
> -		v &= STAT_A_FLOAT_VOLTAGE_MASK;
> -		if (v > 0x3d)
> -			v = 0x3d;
> -
> -		val->intval = 3500000 + v * 20000;
> -		break;
> -
> -	case POWER_SUPPLY_PROP_CURRENT_NOW:
> -		if (!smb347_is_ps_online(smb))
> -			return -ENODATA;
> -
> -		ret = regmap_read(smb->regmap, STAT_B, &v);
> -		if (ret < 0)
> -			return ret;
> -
> -		/*
> -		 * The current value is composition of FCC and PCC values
> -		 * and we can detect which table to use from bit 5.
> -		 */
> -		if (v & 0x20) {
> -			val->intval = hw_to_current(fcc_tbl,
> -						    ARRAY_SIZE(fcc_tbl),
> -						    v & 7);
> -		} else {
> -			v >>= 3;
> -			val->intval = hw_to_current(pcc_tbl,
> -						    ARRAY_SIZE(pcc_tbl),
> -						    v & 7);
> -		}
> -		break;
> -
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		val->intval = pdata->battery_info.charge_full_design;
>  		break;
> @@ -1002,8 +955,6 @@ static enum power_supply_property smb347_battery_properties[] = {
>  	POWER_SUPPLY_PROP_TECHNOLOGY,
>  	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
>  	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> -	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> -	POWER_SUPPLY_PROP_CURRENT_NOW,
>  	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
>  	POWER_SUPPLY_PROP_MODEL_NAME,
>  };
> -- 
> 1.7.0.4

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH] smb347_charger: cleaned battery power supply attributes

[Pallala, Ramakrishna]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1453 bytes --]

Hi Anton,

> On Sat, May 05, 2012 at 09:33:54PM +0530, Ramakrishna Pallala wrote:
> > CURRENT_NOW and VOLTAGE_NOW should be instantaneous readings from
> > power supply(ex: battery).
> >
> > smb347 charger driver reports charge voltage for VOLTAGE_NOW and
> > charge current for CURRENT_NOW attributes which are not instantaneous
> > readings.
> 
> Em. While charging, I guess charge current == instantaneous battery current? Should we
> then report it in such cases? And while discharging and not charging, assuming that HW
> can't report the real battery current flow, we can return some fancy errno code, like
> ENODATA?

Battery current depends on the following things during charging:
1. is the battery in constant current(CC) charge mode or in constant voltage(CV) charge mode?
during CC mode battery current will be close to charge current(with no load) but it will get
reduced gradually as the battery charging approaches CV mode. same for battery voltage.
in CC mode battery voltage will be less than charge voltage and as we approach CV mode 
battery voltage be close to charge voltage setting.

2. instantaneous battery current & voltage also depends on the load current.

we should read battery current & voltage from either PMIC/ADC channels or from fuel gauge chip.

Thanks,
Ram
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] smb347_charger: cleaned battery power supply attributes

[Mika Westerberg]

On Sat, May 05, 2012 at 09:33:54PM +0530, Ramakrishna Pallala wrote:
> CURRENT_NOW and VOLTAGE_NOW should be instantaneous readings
> from power supply(ex: battery).
> 
> smb347 charger driver reports charge voltage for VOLTAGE_NOW
> and charge current for CURRENT_NOW attributes which are not
> instantaneous readings.
> 
> This patch removes the battery VOLTAGE_NOW and CURRENT_NOW
> properties from the driver and also removes hw_to_current()
> which is not required anymore.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>

I agree. As the chip only reports voltage and current what is configured
(and not the actual values) these attributes are pretty much useless and I
should've never introduced them in the first place. A separate Fuel Gauge
or similar is right place to get the actual values.

As long as Anton is ok with this, you can add my

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH] smb347_charger: cleaned battery power supply attributes

[Anton Vorontsov]

On Sun, May 06, 2012 at 02:14:12PM +0300, Mika Westerberg wrote:
> On Sat, May 05, 2012 at 09:33:54PM +0530, Ramakrishna Pallala wrote:
> > CURRENT_NOW and VOLTAGE_NOW should be instantaneous readings
> > from power supply(ex: battery).
> > 
> > smb347 charger driver reports charge voltage for VOLTAGE_NOW
> > and charge current for CURRENT_NOW attributes which are not
> > instantaneous readings.
> > 
> > This patch removes the battery VOLTAGE_NOW and CURRENT_NOW
> > properties from the driver and also removes hw_to_current()
> > which is not required anymore.
> > 
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> 
> I agree. As the chip only reports voltage and current what is configured
> (and not the actual values) these attributes are pretty much useless and I
> should've never introduced them in the first place. A separate Fuel Gauge
> or similar is right place to get the actual values.
> 
> As long as Anton is ok with this, you can add my
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Applied, thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com