linux-kernel - Re: [PATCH 5/5] max17042: Change capacity property to use reported SOC register

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120313183452.GB15964@oksana.dev.rtsoft.ru>
Date:	Tue, 13 Mar 2012 22:34:52 +0400
From:	Anton Vorontsov <cbouatmailru@...il.com>
To:	dirk.brandewie@...il.com
Cc:	linux-kernel@...r.kernel.org, dg77.kim@...sung.com,
	kyungmin.park@...sung.com, myungjoo.ham@...sung.com,
	Jason.Wortham@...im-ic.com, bruce.e.robertson@...el.com,
	Karol Lewandowski <k.lewandowsk@...sung.com>
Subject: Re: [PATCH 5/5] max17042: Change capacity property to use reported
 SOC register

On Tue, Jan 24, 2012 at 09:26:08AM -0800, dirk.brandewie@...il.com wrote:
> From: Dirk Brandewie <dirk.brandewie@...il.com>
> 
> The SOC register (0dh) reports the state of charge before empty
> compensation adjustments are applied.  The max value reported by this
> register will decrease as the battery ages.
> 
> Use the RepSOC register (06h) to report the capacity of the
> battery. RepSOC contains a filtered version of the battery capacity
> after empty compensation adjustments have been applied.
> 
> Reported-by: Gary Keyes <gary.e.keyes@...el.com>
> 
> Signed-off-by: Dirk Brandewie <dirk.brandewie@...il.com>
> ---
>  drivers/power/max17042_battery.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> index 6e96b58..2194278 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -160,7 +160,7 @@ static int max17042_get_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
>  		val->intval = max17042_read_reg(chip->client,
> -				MAX17042_SOC) / 256;
> +				MAX17042_RepSOC) / 256;

Applied. But I don't get it: PROP_CAPACITY should report
percents! And it looks that it reports something very
different.

Also, if you look down the file, you'll see this:

        case POWER_SUPPLY_PROP_CHARGE_FULL:
                ret = max17042_read_reg(chip->client, MAX17042_RepSOC);
                if (ret < 0)
                        return ret;

                if ((ret >> 8) >= MAX17042_BATTERY_FULL)
                        val->intval = 1;
                else if (ret >= 0)
                        val->intval = 0;
                break;

Wut? This is also wrong. PROP_CHARGE_FULL reports uAh, not "fully
charged" boolean status.

Please, read Documentation/power/power_supply_class.txt:

~ ~ ~ ~ ~ ~ ~  Charge/Energy/Capacity - how to not confuse  ~ ~ ~ ~ ~ ~ ~
~                                                                       ~
~ Because both "charge" (µAh) and "energy" (µWh) represents "capacity"  ~
~ of battery, this class distinguish these terms. Don't mix them!       ~
~                                                                       ~
~ CHARGE_* attributes represents capacity in µAh only.                  ~
~ ENERGY_* attributes represents capacity in µWh only.                  ~
~ CAPACITY attribute represents capacity in *percents*, from 0 to 100.  ~
~                                                                       ~
~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~ ~

The driver is seriously broken, very. I'd even consider adding
'depends on BROKEN', as the driver might be the source of the
confusion for drivers based on this one and for userland devs.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@...il.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/