Re: [PATCH 1/5] power: RFC: introduce a new power API
From: Andres Salomon
Date: Tue Dec 18 2007 - 02:10:24 EST
On Mon, 17 Dec 2007 14:24:16 +0300
Anton Vorontsov <cbou@xxxxxxx> wrote:
> On Mon, Dec 17, 2007 at 02:41:39AM -0500, Andres Salomon wrote:
> [...]
> > > > On Sun, 2007-12-16 at 21:24 -0500, Andres Salomon wrote:
> > > > > This API has the power_supply drivers device their own device_attribute
> > > > > list; I find this to be a lot more flexible and cleaner.
> > >
> > > I don't see how this is more flexible and cleaner. See below.
> > >
> > > > > For example,
> > > > > rather than having a function with a huge switch statement (as olpc_battery
> > > > > currently has), we have separate callback functions.
> > >
> > > Is this an improvement? Look into ds2760_battery.c. I scared to
> > > imagine what it will look like after conversion.
> >
[...]
>
> I see your point now. Basically, now I'm encourage to think just one
> more time: is there third (better) option in addition to current and
> this? I still hope there is some not obvious, but elegant solution.
> If there isn't, I'm ready to surrender and will help with everything
> I can.
>
Hm. It occurs to me that there's nothing keeping us from having a
single callback for the driver properties. Keeping the other patches
the same, do you prefer the following approach versus what was originally
in patch#3?
diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index af7a231..1c63dcb 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -50,50 +50,71 @@
* Power
*********************************************************************/
-static int olpc_ac_get_prop(struct power_supply *psy,
- enum power_supply_property psp,
- union power_supply_propval *val)
+static ssize_t olpc_ac_is_online(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
- int ret = 0;
+ int ret;
uint8_t status;
- switch (psp) {
- case POWER_SUPPLY_PROP_ONLINE:
- ret = olpc_ec_cmd(EC_BAT_STATUS, NULL, 0, &status, 1);
- if (ret)
- return ret;
-
- val->intval = !!(status & BAT_STAT_AC);
- break;
- default:
- ret = -EINVAL;
- break;
- }
+ ret = olpc_ec_cmd(EC_BAT_STATUS, NULL, 0, &status, 1);
+ if (!ret)
+ sprintf(buf, "%d\n", !!(status & BAT_STAT_AC));
return ret;
}
-static enum power_supply_property olpc_ac_props[] = {
- POWER_SUPPLY_PROP_ONLINE,
+static struct device_attribute olpc_ac_props[] = {
+ POWER_SUPPLY_ONLINE(olpc_ac_is_online),
+ POWER_SUPPLY_END
};
static struct power_supply olpc_ac = {
.name = "olpc-ac",
.type = POWER_SUPPLY_TYPE_MAINS,
- .properties = olpc_ac_props,
- .num_properties = ARRAY_SIZE(olpc_ac_props),
- .get_property = olpc_ac_get_prop,
+ .props = (struct device_attribute **) &olpc_ac_props,
};
/*********************************************************************
* Battery properties
*********************************************************************/
-static int olpc_bat_get_property(struct power_supply *psy,
- enum power_supply_property psp,
- union power_supply_propval *val)
+
+enum olpc_props {
+ /* should retain the same order as olpc_bat_props */
+ OLPC_PROP_STATUS = 0,
+ OLPC_PROP_PRESENT,
+ OLPC_PROP_HEALTH,
+ OLPC_PROP_TECHNOLOGY,
+ OLPC_PROP_VOLTAGE_AVG,
+ OLPC_PROP_CURRENT_AVG,
+ OLPC_PROP_CAPACITY,
+ OLPC_PROP_TEMP,
+ OLPC_PROP_TEMP_AMBIENT,
+ OLPC_PROP_MANUFACTURER,
+}
+
+static int olpc_bat_get_property(struct device *dev,
+ struct device_attribute *attr, char *buf);
+
+static struct device_attribute olpc_bat_props[] = {
+ POWER_SUPPLY_STATUS(olpc_bat_get_property),
+ POWER_SUPPLY_PRESENT(olpc_bat_get_property),
+ POWER_SUPPLY_HEALTH(olpc_bat_get_property),
+ POWER_SUPPLY_TECHNOLOGY(olpc_bat_get_property),
+ POWER_SUPPLY_VOLT_AVG(olpc_bat_get_property),
+ POWER_SUPPLY_CURR_AVG(olpc_bat_get_property),
+ POWER_SUPPLY_CAPACITY(olpc_bat_get_property),
+ POWER_SUPPLY_TEMP(olpc_bat_get_property),
+ POWER_SUPPLY_TEMP_AMB(olpc_bat_get_property),
+ POWER_SUPPLY_MFR(olpc_bat_get_property),
+ POWER_SUPPLY_END
+};
+
+static int olpc_bat_get_property(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
int ret = 0;
int16_t ec_word;
uint8_t ec_byte;
+ ptrdiff_t prop = attr - olpc_bat_props;
ret = olpc_ec_cmd(EC_BAT_STATUS, NULL, 0, &ec_byte, 1);
if (ret)
@@ -105,37 +126,38 @@ static int olpc_bat_get_property(struct power_supply *psy,
It doesn't matter though -- the EC will return the last-known
information, and it's as if we just ran that _little_ bit faster
and managed to read it out before the battery went away. */
- if (!(ec_byte & BAT_STAT_PRESENT) && psp != POWER_SUPPLY_PROP_PRESENT)
+ if (!(ec_byte & BAT_STAT_PRESENT) && prop != OLPC_PROP_PRESENT)
return -ENODEV;
- switch (psp) {
- case POWER_SUPPLY_PROP_STATUS:
+ switch (prop) {
+ case OLPC_PROP_STATUS:
if (olpc_platform_info.ecver > 0x44) {
if (ec_byte & BAT_STAT_CHARGING)
- val->intval = POWER_SUPPLY_STATUS_CHARGING;
+ ret = POWER_SUPPLY_STATUS_CHARGING;
else if (ec_byte & BAT_STAT_DISCHARGING)
- val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ ret = POWER_SUPPLY_STATUS_DISCHARGING;
else if (ec_byte & BAT_STAT_FULL)
- val->intval = POWER_SUPPLY_STATUS_FULL;
+ ret = POWER_SUPPLY_STATUS_FULL;
else /* er,... */
- val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+ ret = POWER_SUPPLY_STATUS_NOT_CHARGING;
} else {
/* Older EC didn't report charge/discharge bits */
if (!(ec_byte & BAT_STAT_AC)) /* No AC means discharging */
- val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+ ret = POWER_SUPPLY_STATUS_DISCHARGING;
else if (ec_byte & BAT_STAT_FULL)
- val->intval = POWER_SUPPLY_STATUS_FULL;
+ ret = POWER_SUPPLY_STATUS_FULL;
else /* Not _necessarily_ true but EC doesn't tell all yet */
- val->intval = POWER_SUPPLY_STATUS_CHARGING;
- break;
+ ret = POWER_SUPPLY_STATUS_CHARGING;
}
- case POWER_SUPPLY_PROP_PRESENT:
- val->intval = !!(ec_byte & BAT_STAT_PRESENT);
+ ret = power_supply_status_str(ret, buf);
+ break;
+ case OLPC_PROP_PRESENT:
+ ret = sprintf(buf, "%d\n", !!(ec_byte & BAT_STAT_PRESENT));
break;
- case POWER_SUPPLY_PROP_HEALTH:
+ case OLPC_PROP_HEALTH:
if (ec_byte & BAT_STAT_DESTROY)
- val->intval = POWER_SUPPLY_HEALTH_DEAD;
+ ret = POWER_SUPPLY_HEALTH_DEAD;
else {
ret = olpc_ec_cmd(EC_BAT_ERRCODE, NULL, 0, &ec_byte, 1);
if (ret)
@@ -143,22 +165,22 @@ static int olpc_bat_get_property(struct power_supply *psy,
switch (ec_byte) {
case 0:
- val->intval = POWER_SUPPLY_HEALTH_GOOD;
+ ret = POWER_SUPPLY_HEALTH_GOOD;
break;
case BAT_ERR_OVERTEMP:
- val->intval = POWER_SUPPLY_HEALTH_OVERHEAT;
+ ret = POWER_SUPPLY_HEALTH_OVERHEAT;
break;
case BAT_ERR_OVERVOLTAGE:
- val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ ret = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
break;
case BAT_ERR_INFOFAIL:
case BAT_ERR_OUT_OF_CONTROL:
case BAT_ERR_ID_FAIL:
case BAT_ERR_ACR_FAIL:
- val->intval = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
+ ret = POWER_SUPPLY_HEALTH_UNSPEC_FAILURE;
break;
default:
@@ -166,9 +188,10 @@ static int olpc_bat_get_property(struct power_supply *psy,
return -EIO;
}
}
+ ret = power_supply_health_str(ret, buf);
break;
- case POWER_SUPPLY_PROP_MANUFACTURER:
+ case OLPC_PROP_MANUFACTURER:
ec_byte = BAT_ADDR_MFR_TYPE;
ret = olpc_ec_cmd(EC_BAT_EEPROM, &ec_byte, 1, &ec_byte, 1);
if (ret)
@@ -176,17 +199,17 @@ static int olpc_bat_get_property(struct power_supply *psy,
switch (ec_byte >> 4) {
case 1:
- val->strval = "Gold Peak";
+ strcpy(buf, "Gold Peak\n");
break;
case 2:
- val->strval = "BYD";
+ strcpy(buf, "BYD\n");
break;
default:
- val->strval = "Unknown";
+ strcpy(buf, "Unknown\n");
break;
}
break;
- case POWER_SUPPLY_PROP_TECHNOLOGY:
+ case OLPC_PROP_TECHNOLOGY:
ec_byte = BAT_ADDR_MFR_TYPE;
ret = olpc_ec_cmd(EC_BAT_EEPROM, &ec_byte, 1, &ec_byte, 1);
if (ret)
@@ -194,52 +217,53 @@ static int olpc_bat_get_property(struct power_supply *psy,
switch (ec_byte & 0xf) {
case 1:
- val->intval = POWER_SUPPLY_TECHNOLOGY_NiMH;
+ ret = POWER_SUPPLY_TECHNOLOGY_NiMH;
break;
case 2:
- val->intval = POWER_SUPPLY_TECHNOLOGY_LiFe;
+ ret = POWER_SUPPLY_TECHNOLOGY_LiFe;
break;
default:
- val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
+ ret = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
break;
}
+ ret = power_supply_technology_str(ret, buf);
break;
- case POWER_SUPPLY_PROP_VOLTAGE_AVG:
+ case OLPC_PROP_VOLTAGE_AVG:
ret = olpc_ec_cmd(EC_BAT_VOLTAGE, NULL, 0, (void *)&ec_word, 2);
if (ret)
return ret;
ec_word = be16_to_cpu(ec_word);
- val->intval = ec_word * 9760L / 32;
+ ret = sprintf(buf, "%d\n", ec_word * 9760L / 32);
break;
- case POWER_SUPPLY_PROP_CURRENT_AVG:
+ case OLPC_PROP_CURRENT_AVG:
ret = olpc_ec_cmd(EC_BAT_CURRENT, NULL, 0, (void *)&ec_word, 2);
if (ret)
return ret;
ec_word = be16_to_cpu(ec_word);
- val->intval = ec_word * 15625L / 120;
+ ret = sprintf(buf, "%d\n", ec_word * 15625L / 120);
break;
- case POWER_SUPPLY_PROP_CAPACITY:
+ case OLPC_PROP_CAPACITY:
ret = olpc_ec_cmd(EC_BAT_SOC, NULL, 0, &ec_byte, 1);
if (ret)
return ret;
- val->intval = ec_byte;
+ sprintf(buf, "%d\n", ec_byte);
break;
- case POWER_SUPPLY_PROP_TEMP:
+ case OLPC_PROP_TEMP:
ret = olpc_ec_cmd(EC_BAT_TEMP, NULL, 0, (void *)&ec_word, 2);
if (ret)
return ret;
ec_word = be16_to_cpu(ec_word);
- val->intval = ec_word * 100 / 256;
+ ret = sprintf(buf, "%d\n", ec_word * 100 / 256);
break;
- case POWER_SUPPLY_PROP_TEMP_AMBIENT:
+ case OLPC_PROP_TEMP_AMBIENT:
ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
if (ret)
return ret;
ec_word = be16_to_cpu(ec_word);
- val->intval = ec_word * 100 / 256;
+ ret = sprintf(buf, "%d\n", ec_word * 100 / 256);
break;
default:
ret = -EINVAL;
@@ -249,19 +273,6 @@ static int olpc_bat_get_property(struct power_supply *psy,
return ret;
}
-static enum power_supply_property olpc_bat_props[] = {
- POWER_SUPPLY_PROP_STATUS,
- POWER_SUPPLY_PROP_PRESENT,
- POWER_SUPPLY_PROP_HEALTH,
- POWER_SUPPLY_PROP_TECHNOLOGY,
- POWER_SUPPLY_PROP_VOLTAGE_AVG,
- POWER_SUPPLY_PROP_CURRENT_AVG,
- POWER_SUPPLY_PROP_CAPACITY,
- POWER_SUPPLY_PROP_TEMP,
- POWER_SUPPLY_PROP_TEMP_AMBIENT,
- POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
/*********************************************************************
* Initialisation
*********************************************************************/
@@ -269,9 +280,7 @@ static enum power_supply_property olpc_bat_props[] = {
static struct platform_device *bat_pdev;
static struct power_supply olpc_bat = {
- .properties = olpc_bat_props,
- .num_properties = ARRAY_SIZE(olpc_bat_props),
- .get_property = olpc_bat_get_property,
+ .props = (struct device_attribute **) &olpc_bat_props,
.use_for_apm = 1,
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/