[PATCH] max17042_battery: add support for battery STATUS and CHARGE_TYPE

[PATCH] max17042_battery: add support for battery STATUS and CHARGE_TYPE

[Ramakrishna Pallala]

This patch adds the support to report the battery power supply attributes
STATUS and CHARGE_TYPE. This patch makes use of power_supply_get_external_attr()
API to get these attributes through power supply core.

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

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 74abc6c..56131f8 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -105,6 +105,8 @@ static void max17042_set_reg(struct i2c_client *client,
 
 static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CHARGE_TYPE,
 	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
 	POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN,
@@ -125,12 +127,29 @@ static int max17042_get_property(struct power_supply *psy,
 {
 	struct max17042_chip *chip = container_of(psy,
 				struct max17042_chip, battery);
+	struct power_supply_attr_query query;
 	int ret;
 
 	if (!chip->init_complete)
 		return -EAGAIN;
 
+	memset(&query, 0x0, sizeof(query));
+	query.property = psp;
+	query.type = POWER_SUPPLY_TYPE_BATTERY;
+
 	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = power_supply_get_external_attr(&query);
+		if (ret < 0)
+			return ret;
+		val->intval = query.res.intval;
+		break;
+	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+		ret = power_supply_get_external_attr(&query);
+		if (ret < 0)
+			return ret;
+		val->intval = query.res.intval;
+		break;
 	case POWER_SUPPLY_PROP_PRESENT:
 		ret = max17042_read_reg(chip->client, MAX17042_STATUS);
 		if (ret < 0)
-- 
1.7.0.4

Re: [PATCH] max17042_battery: add support for battery STATUS and CHARGE_TYPE

[Anton Vorontsov]

On Fri, Jul 27, 2012 at 10:26:21PM +0530, Ramakrishna Pallala wrote:
> This patch adds the support to report the battery power supply attributes
> STATUS and CHARGE_TYPE. This patch makes use of power_supply_get_external_attr()
> API to get these attributes through power supply core.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> ---
[...]
>  	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		ret = power_supply_get_external_attr(&query);
> +		if (ret < 0)
> +			return ret;
> +		val->intval = query.res.intval;
> +		break;

First of, thanks a lot for your work. And sorry that it took me quite
a while to review it, but these are fundamental changes, so I had to
thoughtfully consider all this.

This exact code clearly shows that it is not battery's property. Battery
itself cannot report it, right? OK, here is what
Documentation/power/power_supply_class.txt says:

A: Most likely, no. This class is designed to export properties which are
   directly measurable by the specific hardware available.

   Inferring not available properties using some heuristics or mathematical
   model is not subject of work for a battery driver. Such functionality
   should be factored out

So, you basically try to infer battery's properties from the charger hw.
This is surely doable, but no, I think we should not do this. Instead,
what we want is to make use of "supplied_to" mechanism of the power
supply class, and export it via sysfs. Then userland can see all the
power supply hierarchy, and thus see which hardware provides which data.

See my thoughts about exporting "supplied_to" to the sysfs:

	http://lkml.org/lkml/2011/6/22/258

So, we'll have:

/sys/
   class/
      power_supply/
         charger/
            supplied_to/battery -> ../../battery
         battery/

Sure, we'll have to teach userland to operate on this scheme, i.e.
it must be aware that batteries might be connected to an external
charger, and if so, userland must query* charging status from the
charger.

Do you see any problem with this approach?

Thanks!

p.s.

Actually, once userland read all the hierarchy, it can get properties
pretty easy, e.g. recursively walking the tree (pseudo code):

get_prop(psy, prop, *val)
{
	if (prop_exists(psy, prop))
		return psy->get_property(psy, prop, val);
	if (psy->supplier)
		return get_prop(psy->parent, prop, val);
	return ENODATA;
}

Note that the pseudo-code above is for the userland, not for the kernel.
(We can surely implement a similar function in the kernel, it just that
we don't need it, at least now. But the function would do the similar
thing you're trying to accomplish: get parent's properties.)

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

RE: [PATCH] max17042_battery: add support for battery STATUS and CHARGE_TYPE

[Pallala, Ramakrishna]

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

> On Fri, Jul 27, 2012 at 10:26:21PM +0530, Ramakrishna Pallala wrote:
> > This patch adds the support to report the battery power supply
> > attributes STATUS and CHARGE_TYPE. This patch makes use of
> > power_supply_get_external_attr() API to get these attributes through power
> supply core.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > ---
> [...]
> >  	switch (psp) {
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		ret = power_supply_get_external_attr(&query);
> > +		if (ret < 0)
> > +			return ret;
> > +		val->intval = query.res.intval;
> > +		break;
> 
> First of, thanks a lot for your work. And sorry that it took me quite a while to
> review it, but these are fundamental changes, so I had to thoughtfully consider
> all this.
> 
> This exact code clearly shows that it is not battery's property. Battery itself
> cannot report it, right? OK, here is what
> Documentation/power/power_supply_class.txt says:
> 
> A: Most likely, no. This class is designed to export properties which are
>    directly measurable by the specific hardware available.
> 
>    Inferring not available properties using some heuristics or mathematical
>    model is not subject of work for a battery driver. Such functionality
>    should be factored out
> 
> So, you basically try to infer battery's properties from the charger hw.
> This is surely doable, but no, I think we should not do this. Instead, what we
> want is to make use of "supplied_to" mechanism of the power supply class, and
> export it via sysfs. Then userland can see all the power supply hierarchy, and
> thus see which hardware provides which data.
> 
> See my thoughts about exporting "supplied_to" to the sysfs:
> 
> 	http://lkml.org/lkml/2011/6/22/258
> 
> So, we'll have:
> 
> /sys/
>    class/
>       power_supply/
>          charger/
>             supplied_to/battery -> ../../battery
>          battery/
> 
> Sure, we'll have to teach userland to operate on this scheme, i.e.
> it must be aware that batteries might be connected to an external charger, and
> if so, userland must query* charging status from the charger.
> 
> Do you see any problem with this approach?
> 
> Thanks!

Got your point. I will try this approach.
Thanks for your time :-)

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¥