This API has the power_supply drivers device their own device_attribute
list; I find this to be a lot more flexible and cleaner. For example,
rather than having a function with a huge switch statement (as olpc_battery
currently has), we have separate callback functions. We're not limited
to drivers only being able to pass 'int' and 'char*'s in sysfs, we're
not forced to keep a global string around in memory (as is again the
case for olpc_battery's serial number code), we don't have ordering
restrictions w/ the return value being interpreted based upon where it's
located in the array... etc. The other API seems to encourage driver
authors to get their custom sysfs knobs into the list of sysfs knobs, and
this one doesn't.
If there is interest in this API, I'll convert the rest of the power_supply
drivers over to it and resubmit patches.
Ignore the psy->num_properties indentation below; that was done so patch #4
wasn't stupidly large.
Signed-off-by: Andres Salomon <[email protected]>
---
drivers/power/power_supply_core.c | 9 +++
drivers/power/power_supply_leds.c | 18 +++++
drivers/power/power_supply_sysfs.c | 123 ++++++++++++++++++++++++++++++++++++
include/linux/power_supply.h | 96 ++++++++++++++++++++++++++++
4 files changed, 246 insertions(+), 0 deletions(-)
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index a63b75c..09013e8 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -67,9 +67,18 @@ int power_supply_am_i_supplied(struct power_supply *psy)
for (i = 0; i < epsy->num_supplicants; i++) {
if (!strcmp(epsy->supplied_to[i], psy->name)) {
+ if (epsy->num_properties) {
if (epsy->get_property(epsy,
POWER_SUPPLY_PROP_ONLINE, &ret))
continue;
+ } else {
+ /* new API */
+ struct device_attribute *attr;
+ attr = power_supply_find_attr(epsy, "online");
+ if (!attr || power_supply_attr_run(epsy, attr,
+ str_to_int, &ret.intval))
+ continue;
+ }
if (ret.intval)
goto out;
}
diff --git a/drivers/power/power_supply_leds.c b/drivers/power/power_supply_leds.c
index 7f8f359..45539b7 100644
--- a/drivers/power/power_supply_leds.c
+++ b/drivers/power/power_supply_leds.c
@@ -17,9 +17,18 @@
static void power_supply_update_bat_leds(struct power_supply *psy)
{
union power_supply_propval status;
+ struct device_attribute *attr;
+ if (psy->num_properties) {
if (psy->get_property(psy, POWER_SUPPLY_PROP_STATUS, &status))
return;
+ } else {
+ /* new API */
+ attr = power_supply_find_attr(psy, "status");
+ if (!attr || power_supply_attr_run(psy, attr, str_to_int,
+ &status.intval))
+ return;
+ }
dev_dbg(psy->dev, "%s %d\n", __FUNCTION__, status.intval);
@@ -102,9 +111,18 @@ static void power_supply_remove_bat_triggers(struct power_supply *psy)
static void power_supply_update_gen_leds(struct power_supply *psy)
{
union power_supply_propval online;
+ struct device_attribute *attr;
+ if (psy->num_properties) {
if (psy->get_property(psy, POWER_SUPPLY_PROP_ONLINE, &online))
return;
+ } else {
+ /* new API */
+ attr = power_supply_find_attr(psy, "online");
+ if (!attr || power_supply_attr_run(psy, attr, str_to_int,
+ &online.intval))
+ return;
+ }
dev_dbg(psy->dev, "%s %d\n", __FUNCTION__, online.intval);
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 8efedba..3e44d16 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -138,19 +138,34 @@ int power_supply_create_attrs(struct power_supply *psy)
goto statics_failed;
}
+ if (psy->num_properties) {
for (j = 0; j < psy->num_properties; j++) {
rc = device_create_file(psy->dev,
&power_supply_attrs[psy->properties[j]]);
if (rc)
goto dynamics_failed;
}
+ } else {
+ /* new API */
+ for (j = 0; psy->props[j]; j++) {
+ rc = device_create_file(psy->dev, psy->props[j]);
+ if (rc)
+ goto dynamics_failed;
+ }
+ }
goto succeed;
dynamics_failed:
+ if (psy->num_properties) {
while (j--)
device_remove_file(psy->dev,
&power_supply_attrs[psy->properties[j]]);
+ } else {
+ /* new API */
+ while (j--)
+ device_remove_file(psy->dev, psy->props[j]);
+ }
statics_failed:
while (i--)
device_remove_file(psy->dev, &power_supply_static_attrs[i]);
@@ -165,9 +180,56 @@ void power_supply_remove_attrs(struct power_supply *psy)
for (i = 0; i < ARRAY_SIZE(power_supply_static_attrs); i++)
device_remove_file(psy->dev, &power_supply_static_attrs[i]);
+ if (psy->num_properties) {
for (i = 0; i < psy->num_properties; i++)
device_remove_file(psy->dev,
&power_supply_attrs[psy->properties[i]]);
+ } else {
+ /* new API */
+ for (i = 0; psy->props[i]; i++)
+ device_remove_file(psy->dev, psy->props[i]);
+ }
+}
+
+struct device_attribute *power_supply_find_attr(struct power_supply *psy,
+ const char *name)
+{
+ struct device_attribute **attr;
+
+ for (attr = psy->props; *attr; attr++) {
+ if (!strcmp((*attr)->attr.name, name))
+ return *attr;
+ }
+
+ return NULL;
+}
+
+/*
+ * Attempt to provide a nice way to call device_attribute show() callbacks.
+ * Allocate the buffer, run ->show(), and then call callback() to allow the
+ * caller to parse the buffer and fill in 'arg' (without having to worry
+ * about managing the buffer).
+ */
+int power_supply_attr_run(struct power_supply *psy,
+ struct device_attribute *attr,
+ void (*callback)(const char *buf, void *arg), void *arg)
+{
+ char *buf;
+ int ret = -ENOMEM;
+
+ buf = (char *) get_zeroed_page(GFP_KERNEL);
+ if (!buf)
+ goto out;
+
+ ret = attr->show(psy->dev, attr, buf);
+ if (ret < 0)
+ goto out;
+ callback(buf, arg);
+ ret = 0;
+out:
+ if (buf)
+ free_page((unsigned long) buf);
+ return ret;
}
static char *kstruprdup(const char *str, gfp_t gfp)
@@ -187,6 +249,52 @@ static char *kstruprdup(const char *str, gfp_t gfp)
return ret;
}
+/*
+ * This is a hack to stuff 4 args into a callback function that only
+ * takes 1 arg; add_uevent_arg_wrapper() below handles the details of it.
+ */
+struct uevent_arg {
+ struct device *dev;
+ struct kobj_uevent_env *env;
+ char *attrname;
+ int ret;
+};
+
+static void call_add_uevent_var(const char *buf, void *arg)
+{
+ struct uevent_arg *uarg = arg;
+ char *line;
+
+ line = strchr(buf, '\n');
+ if (line)
+ *line = 0;
+
+ dev_dbg(uarg->dev, "prop %s=%s\n", uarg->attrname, buf);
+ uarg->ret = add_uevent_var(uarg->env, "POWER_SUPPLY_%s=%s",
+ uarg->attrname, buf);
+}
+
+static int add_uevent_arg_wrapper(struct power_supply *psy,
+ struct device_attribute *attr, struct kobj_uevent_env *env)
+{
+ struct uevent_arg uarg = {
+ .dev = psy->dev,
+ .env = env,
+ };
+ int ret;
+
+ uarg.attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
+ if (!uarg.attrname)
+ return -ENOMEM;
+
+ ret = power_supply_attr_run(psy, attr, call_add_uevent_var, &uarg);
+ if (ret >= 0)
+ ret = uarg.ret;
+
+ kfree(uarg.attrname);
+ return ret;
+}
+
int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
{
struct power_supply *psy = dev_get_drvdata(dev);
@@ -239,6 +347,7 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
goto out;
}
+ if (psy->num_properties) {
dev_dbg(dev, "%zd dynamic props\n", psy->num_properties);
for (j = 0; j < psy->num_properties; j++) {
@@ -275,6 +384,20 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
if (ret)
goto out;
}
+ } else {
+ /* new API */
+
+ for (j = 0; psy->props[j]; j++) {
+ struct device_attribute *attr = psy->props[j];
+
+ ret = add_uevent_arg_wrapper(psy, attr, env);
+ /* When a battery is absent, we expect -ENODEV. Don't abort;
+ * send the uevent with at least the PRESENT=0 property. */
+ if (ret < 0 && ret != -ENODEV)
+ goto out;
+ }
+ dev_dbg(dev, "%zd dynamic props\n", j);
+ }
out:
free_page((unsigned long)prop_buf);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 358b38d..21251a3 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -38,6 +38,17 @@ enum {
POWER_SUPPLY_STATUS_FULL,
};
+static inline ssize_t power_supply_status_str(int status, char *buf)
+{
+ static char *status_text[] = {
+ "Unknown", "Charging", "Discharging", "Not charging", "Full"
+ };
+
+ BUG_ON(status < POWER_SUPPLY_STATUS_UNKNOWN ||
+ status > POWER_SUPPLY_STATUS_FULL);
+ return sprintf(buf, "%s\n", status_text[status]);
+}
+
enum {
POWER_SUPPLY_HEALTH_UNKNOWN = 0,
POWER_SUPPLY_HEALTH_GOOD,
@@ -47,6 +58,18 @@ enum {
POWER_SUPPLY_HEALTH_UNSPEC_FAILURE,
};
+static inline ssize_t power_supply_health_str(int health, char *buf)
+{
+ static char *health_text[] = {
+ "Unknown", "Good", "Overheat", "Dead", "Over voltage",
+ "Unspecified failure"
+ };
+
+ BUG_ON(health < POWER_SUPPLY_HEALTH_UNKNOWN ||
+ health > POWER_SUPPLY_HEALTH_UNSPEC_FAILURE);
+ return sprintf(buf, "%s\n", health_text[health]);
+}
+
enum {
POWER_SUPPLY_TECHNOLOGY_UNKNOWN = 0,
POWER_SUPPLY_TECHNOLOGY_NiMH,
@@ -56,6 +79,66 @@ enum {
POWER_SUPPLY_TECHNOLOGY_NiCd,
};
+static inline ssize_t power_supply_technology_str(int tech, char *buf)
+{
+ static char *technology_text[] = {
+ "Unknown", "NiMH", "Li-ion", "Li-poly", "LiFe", "NiCd"
+ };
+
+ BUG_ON(tech < POWER_SUPPLY_TECHNOLOGY_UNKNOWN ||
+ tech > POWER_SUPPLY_TECHNOLOGY_NiCd);
+ return sprintf(buf, "%s\n", technology_text[tech]);
+}
+
+/* suggested fields to support */
+#define POWER_SUPPLY_STATUS(cb) __ATTR(status, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_HEALTH(cb) __ATTR(health, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_PRESENT(cb) __ATTR(present, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ONLINE(cb) __ATTR(online, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_TECHNOLOGY(cb) __ATTR(technology, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_VOLT_MAX(cb) __ATTR(voltage_max_design, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_VOLT_MIN(cb) __ATTR(voltage_min_design, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_VOLT_NOW(cb) __ATTR(voltage_now, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_VOLT_AVG(cb) __ATTR(voltage_avg, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CURR_NOW(cb) __ATTR(current_now, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CURR_AVG(cb) __ATTR(current_avg, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CHG_FULL_DESIGN(cb) __ATTR(charge_full_design, \
+ S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CHG_EMPTY_DESIGN(cb) __ATTR(charge_empty_, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_CHG_FULL(cb) __ATTR(charge_full, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CHG_EMPTY(cb) __ATTR(charge_empty, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CHG_NOW(cb) __ATTR(charge_now, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_CHG_AVG(cb) __ATTR(charge_empty, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ENERGY_FULL_DESIGN(cb) __ATTR(energy_full_design, \
+ S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ENERGY_EMPTY_DESIGN(cb) __ATTR(energy_empty_design, \
+ S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ENERGY_FULL(cb) __ATTR(energy_full, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ENERGY_EMPTY(cb) __ATTR(energy_empty, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_ENERGY_NOW(cb) __ATTR(energy_now, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_ENERGY_AVG(cb) __ATTR(energy_avg, S_IRUGO, cb, NULL)
+/* capacity is in percents! */
+#define POWER_SUPPLY_CAPACITY(cb) __ATTR(capacity, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_TEMP(cb) __ATTR(temp, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_TEMP_AMB(cb) __ATTR(temp_ambient, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_TTE_NOW(cb) __ATTR(time_to_empty_now, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_TTE_AVG(cb) __ATTR(time_to_empty_avg, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_TTF_NOW(cb) __ATTR(time_to_full_now, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_TTF_AVG(cb) __ATTR(time_to_full_avg, S_IRUGO, \
+ cb, NULL)
+#define POWER_SUPPLY_MODEL(cb) __ATTR(model_name, S_IRUGO, cb, NULL)
+#define POWER_SUPPLY_MFR(cb) __ATTR(manufacturer, S_IRUGO, cb, NULL)
+
+/* be sure to end the property list with this! */
+#define POWER_SUPPLY_END __ATTR_NULL
+
enum power_supply_property {
/* Properties of type `int' */
POWER_SUPPLY_PROP_STATUS = 0,
@@ -110,6 +193,7 @@ struct power_supply {
enum power_supply_type type;
enum power_supply_property *properties;
size_t num_properties;
+ struct device_attribute **props;
char **supplied_to;
size_t num_supplicants;
@@ -164,6 +248,18 @@ extern int power_supply_register(struct device *parent,
struct power_supply *psy);
extern void power_supply_unregister(struct power_supply *psy);
+extern struct device_attribute *power_supply_find_attr(struct power_supply *psy,
+ const char *name);
+extern int power_supply_attr_run(struct power_supply *psy,
+ struct device_attribute *attr,
+ void (*callback)(const char *buf, void *arg), void *arg);
+
+/* helper function for power_supply_attr_run() */
+static inline void str_to_int(const char *buf, void *arg)
+{
+ *(int *)arg = simple_strtol(buf, NULL, 0);
+}
+
/* For APM emulation, think legacy userspace. */
extern struct class *power_supply_class;
--
1.5.3.5
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. For example,
> rather than having a function with a huge switch statement (as olpc_battery
> currently has), we have separate callback functions. We're not limited
> to drivers only being able to pass 'int' and 'char*'s in sysfs, we're
> not forced to keep a global string around in memory (as is again the
> case for olpc_battery's serial number code), we don't have ordering
> restrictions w/ the return value being interpreted based upon where it's
> located in the array... etc. The other API seems to encourage driver
> authors to get their custom sysfs knobs into the list of sysfs knobs, and
> this one doesn't.
>
> If there is interest in this API, I'll convert the rest of the power_supply
> drivers over to it and resubmit patches.
Looks sane enough to me. If Anton has no objections, I'll merge it.
> Ignore the psy->num_properties indentation below; that was done so patch #4
> wasn't stupidly large.
Interesting... but I suppose it makes sense.
--
dwmw2
Hello Andres, David,
Firstly, Andres, thank you for the efforts.
I quite foreseen what exactly you had in mind when we were
discussing the idea. With patches it's indeed easier to show
flaws of this approach.
On Sun, Dec 16, 2007 at 09:36:24PM -0500, David Woodhouse 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.
As for olpc's "huge switch statement", it could be split into
functions _without_ drastic changes to PSY class. As the bonus,
you will get _inlining_ of these functions by gcc, because
there will be just single user of these functions. With
"exported-via-pointers" functions you can't do that.
You have tons of similar functions with similar functionality, that
only differs by the data source. That scheme was in the early PSY
class I posted here this summer. And I turned it down, fortunately.
On a bet, I can convert "huge switch statement" to nicely look switch
statement. It will as nice as ds2760's.
The problem isn't in the PSY class.
> > We're not limited
> > to drivers only being able to pass 'int' and 'char*'s in sysfs,
You're not limited to "int" and "char *". Anything more than that
is unnecessary, so far.
> > we're
> > not forced to keep a global string around in memory (as is again the
> > case for olpc_battery's serial number code),
If battery chip can report strings, then you anyway must keep it in
the memory. The question is when to allocate memory and when to free
it. Side question is for how long to keep it.
Given that that string is small enough (dozen bytes), it's doesn't
matter for how long we'll allocate it. So, in most cases it's easy
to answer: allocate at probe, free at remove, so keep it for whole
battery lifetime. (In contrast, adding tons of functions will waste
_much more_ space than these dozen bytes!)
IIRC this is the main difficulty you're facing with current properties
approach. You've converted whole class to the something different..
but you didn't show a single user of that change. Sorry, olpc still
using hard-coded manufacturer string:
+static ssize_t olpc_bat_manufacturer(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+ uint8_t ec_byte;
+
+ ret = olpc_bat_get_status(&ec_byte);
+ if (ret)
+ return ret;
+
+ ec_byte = BAT_ADDR_MFR_TYPE;
+ ret = olpc_ec_cmd(EC_BAT_EEPROM, &ec_byte, 1, &ec_byte, 1);
+ if (ret)
+ return ret;
+ switch (ec_byte >> 4) {
+ case 1:
+ strcpy(buf, "Gold Peak");
break;
+ case 2:
+ strcpy(buf, "BYD");
break;
+ default:
+ strcpy(buf, "Unknown");
break;
+ }
+
+ return ret;
+}
In other words: all these strings can and should be static. Why
spend cpu cycles on strcpy'ing things that can be not strcpy'ed?
I don't see S/N function. I'm sure it could be implemented easily
using today's properties approach.
> > we don't have ordering
> > restrictions w/ the return value being interpreted based upon where it's
> > located in the array... etc.
What exact "restrictions" you're talking about? There are no
restrictions per se.
> > The other API seems to encourage driver
> > authors to get their custom sysfs knobs into the list of sysfs knobs, and
> > this one doesn't.
Yes, API is encouraging to add knobs, but not just any knobs. Only
ones that make sense as a property of a PSY (opposite to some kind
property of PSY driver itself). The count of such properties is
limited, physically.
I'm recalling question about raw data. No, PSY class isn't for raw
data you're getting from the firmware. Implement driver-specific
binary attribute, that will contain device-specific raw data.
Ideally, you should not export raw data at all (though, good idea
is to export them into the debugfs).
> > If there is interest in this API, I'll convert the rest of the power_supply
> > drivers over to it and resubmit patches.
>
> Looks sane enough to me.
Heh..
> If Anton has no objections, I'll merge it.
Sorry, lots of objections.
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Mon, 17 Dec 2007 08:51:23 +0300
Anton Vorontsov <[email protected]> wrote:
> Hello Andres, David,
>
> Firstly, Andres, thank you for the efforts.
>
> I quite foreseen what exactly you had in mind when we were
> discussing the idea. With patches it's indeed easier to show
> flaws of this approach.
>
>
> On Sun, Dec 16, 2007 at 09:36:24PM -0500, David Woodhouse 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.
Why? It would not look bad after conversion. Basically:
static ssize_t ds2760_battery_get_status(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct ds2760_device_info *di = to_ds2760_device_info(psy);
return power_supply_status_str(di->charge_status, buf);
}
static ssize_t ds2760_battery_get_voltage_now(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct ds2760_device_info *di = to_ds2760_device_info(psy);
ds2760_battery_read_status(di);
return sprintf(buf, "%d\n", di->voltage_uV);
}
...an so on.
If I wanted to get really clever, I could do:
#define DS2760_CALLBACK(name, fmt, var) \
static ssize_t ds2760_battery_get_##name(struct device *dev, \
struct device_attribute *attr, char *buf) \
{ \
struct ds2760_device_info *di = to_ds2760_device_info(psy); \
ds2760_battery_read_status(di); \
return sprintf(buf, fmt, var); \
}
DS2760_CALLBACK(voltage_now, "%d\n", di->voltage_uV)
DS2760_CALLBACK(current_now, "%d\n", di->current_uA)
etc.. but, I'm not trying to compress lines of code, I'm trying
to ensure things are readable.
>
> As for olpc's "huge switch statement", it could be split into
> functions _without_ drastic changes to PSY class. As the bonus,
> you will get _inlining_ of these functions by gcc, because
> there will be just single user of these functions. With
> "exported-via-pointers" functions you can't do that.
>
> You have tons of similar functions with similar functionality, that
> only differs by the data source. That scheme was in the early PSY
> class I posted here this summer. And I turned it down, fortunately.
>
>
> On a bet, I can convert "huge switch statement" to nicely look switch
> statement. It will as nice as ds2760's.
>
> The problem isn't in the PSY class.
>
We're still going to be stuck with a huge switch statement. Yes, it
wouldn't be *as* big, but ds2760_battery.c has a decently sized switch
statement, and olpc_battery.c has even more properties.
The huge switch statement is the least of my worries, though. Getting
rid of it is just a bonus.
> > > We're not limited
> > > to drivers only being able to pass 'int' and 'char*'s in sysfs,
>
> You're not limited to "int" and "char *". Anything more than that
> is unnecessary, so far.
>
See below about the eeprom dump. Originally, it was desired for this
to be a hex string; after that, binary. Of course, once I actually
started adding device_attributes to olpc_battery.c, I started wondering;
why not just make *all* the properties device_attributes? And, what if
I want to show something larger than a signed int? What if I have a
value that I want to pad with 0's?
> > > we're
> > > not forced to keep a global string around in memory (as is again the
> > > case for olpc_battery's serial number code),
>
> If battery chip can report strings, then you anyway must keep it in
> the memory. The question is when to allocate memory and when to free
> it. Side question is for how long to keep it.
>
> Given that that string is small enough (dozen bytes), it's doesn't
> matter for how long we'll allocate it. So, in most cases it's easy
> to answer: allocate at probe, free at remove, so keep it for whole
> battery lifetime. (In contrast, adding tons of functions will waste
> _much more_ space than these dozen bytes!)
>
>
> IIRC this is the main difficulty you're facing with current properties
> approach. You've converted whole class to the something different..
> but you didn't show a single user of that change. Sorry, olpc still
> using hard-coded manufacturer string:
>
Well, no, I was talking about the serial number string. It's not
upstream yet, it's just in OLPC's repo.
http://dev.laptop.org/git?p=olpc-2.6;a=commitdiff;h=f9b4313060ab9047942707da6d3084f7792e714c
Note bat_serial, 17 bytes sitting around. That's actually not that
bad, merely awkward. Worse (and what caused me to start reworking
the API) was a dump of the EEPROM contents in hex; basically, (0x80-0x20)*2.
So, a buffer of 192 bytes was required to be kept around in memory. As
we add more features, we end up w/ more and more wasted space.
> +static ssize_t olpc_bat_manufacturer(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int ret;
> + uint8_t ec_byte;
> +
> + ret = olpc_bat_get_status(&ec_byte);
> + if (ret)
> + return ret;
> +
> + ec_byte = BAT_ADDR_MFR_TYPE;
> + ret = olpc_ec_cmd(EC_BAT_EEPROM, &ec_byte, 1, &ec_byte, 1);
> + if (ret)
> + return ret;
>
> + switch (ec_byte >> 4) {
> + case 1:
> + strcpy(buf, "Gold Peak");
> break;
> + case 2:
> + strcpy(buf, "BYD");
> break;
> + default:
> + strcpy(buf, "Unknown");
> break;
> + }
> +
> + return ret;
> +}
>
> In other words: all these strings can and should be static. Why
> spend cpu cycles on strcpy'ing things that can be not strcpy'ed?
Sure, except we have to spend the CPU cycles copying them into the
buffer *anyways*. After all, that's how power_supply_sysfs gets
the returned char*'s into the sysfs buffer..
>
> I don't see S/N function. I'm sure it could be implemented easily
> using today's properties approach.
>
> > > we don't have ordering
> > > restrictions w/ the return value being interpreted based upon where it's
> > > located in the array... etc.
>
> What exact "restrictions" you're talking about? There are no
> restrictions per se.
>
The power_supply_properties enum; 'int's are listed first, 'char*'s are
listed second. power_supply_show_property() assumes that anything
after POWER_SUPPLY_PROP_MODEL_NAME in that list is a char*, and anything
before it is an integer. No other options are available, and if you
accidentally put an int property somewhere in the strings section; well,
too bad.
> > > The other API seems to encourage driver
> > > authors to get their custom sysfs knobs into the list of sysfs knobs, and
> > > this one doesn't.
>
> Yes, API is encouraging to add knobs, but not just any knobs. Only
> ones that make sense as a property of a PSY (opposite to some kind
> property of PSY driver itself). The count of such properties is
> limited, physically.
I'll reference
http://dev.laptop.org/git?p=olpc-2.6;a=commitdiff;h=f9b4313060ab9047942707da6d3084f7792e714c
again, because it's a good example of the problems w/ the current API.
Basically, we need to add POWER_SUPPLY_PROP_ACCUM_CURRENT to the core
driver. I don't see any other power_supply driver actually needing
that; however, the alternative is having magical #defines for those
that are in olpc_battery.c.. and we hope that the core enum list never
accidentally clobbers our internal defines. ie,
#define OLPC_PROP_ACCUM_CURRENT 0x30
Inside of olpc_bat_get_property:
case OLPC_PROP_ACCUM_CURRENT:
...
break;
And, we hope that include/linux/power_supply.h never grows to include 48
entries inside the power_supply_properties enum (otherwise, we could end
up with power_supply_show_property() mangling OLPC_PROP_ACCUM_CURRENT).
Realistic? Not overly. But, having to choose arbitrary numbers is
*not* a desirable way to do things, nor is requiring every single
property from every single driver to end up in the power_supply_properties
enum list.
The power_supply_class docs say, "Power supply class is extensible, and
allows to define drivers own attributes."; however, they don't say *how*
to define their own attributes, and I don't see any power_supply drivers
that are doing that. This leads me to believe that the API is not
suited for this sort of thing.
>
> I'm recalling question about raw data. No, PSY class isn't for raw
> data you're getting from the firmware. Implement driver-specific
> binary attribute, that will contain device-specific raw data.
> Ideally, you should not export raw data at all (though, good idea
> is to export them into the debugfs).
Sure, debugfs is an option; however, for a quick and dirty tests, one
of our hardware people wanted to be able to simply view the contents
of the eeprom (and it wasn't clear what format was best. My assumption
was that hex would be okay, but it turned out to be fairly unreadable.
Binary piped to od was much nicer).
>
> > > If there is interest in this API, I'll convert the rest of the power_supply
> > > drivers over to it and resubmit patches.
> >
> > Looks sane enough to me.
>
> Heh..
>
> > If Anton has no objections, I'll merge it.
>
> Sorry, lots of objections.
>
Yes, the callback API is slightly more verbose. I see that as a good
thing; I don't think the power_supply core should be magically turning
results into what it thinks they should be. I also think that moving
code out of the power_supply core, and into specific power_supply
drivers is a good idea. If I'm using pda_power.ko, I don't want all
the excess baggage that olpc_battery.ko might need (and therefore
had to include in power_supply_core).
Finally, to compare code size.. The current API:
-rw-r--r-- 1 dilinger dilinger 56768 2007-12-17 02:21 drivers/power/pda_power.o
-rw-r--r-- 1 dilinger dilinger 31830 2007-12-17 02:21 drivers/power/power_supply_core.o
-rw-r--r-- 1 dilinger dilinger 25120 2007-12-17 02:21 drivers/power/power_supply_leds.o
-rw-r--r-- 1 dilinger dilinger 30220 2007-12-17 02:21 drivers/power/power_supply_sysfs.o
The API I'm suggesting:
-rw-r--r-- 1 dilinger dilinger 55208 2007-12-17 02:19 drivers/power/pda_power.o
-rw-r--r-- 1 dilinger dilinger 30832 2007-12-17 02:19 drivers/power/power_supply_core.o
-rw-r--r-- 1 dilinger dilinger 24232 2007-12-17 02:19 drivers/power/power_supply_leds.o
-rw-r--r-- 1 dilinger dilinger 26680 2007-12-17 02:19 drivers/power/power_supply_sysfs.o
I think I can actually do even better, but I'm not going to bother unless
folks like the new API.
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.
>
> Why? It would not look bad after conversion. Basically:
>
> static ssize_t ds2760_battery_get_status(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct ds2760_device_info *di = to_ds2760_device_info(psy);
> return power_supply_status_str(di->charge_status, buf);
> }
> static ssize_t ds2760_battery_get_voltage_now(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct ds2760_device_info *di = to_ds2760_device_info(psy);
> ds2760_battery_read_status(di);
> return sprintf(buf, "%d\n", di->voltage_uV);
> }
>
> ....an so on.
>
> If I wanted to get really clever, I could do:
>
> #define DS2760_CALLBACK(name, fmt, var) \
> static ssize_t ds2760_battery_get_##name(struct device *dev, \
> struct device_attribute *attr, char *buf) \
> { \
> struct ds2760_device_info *di = to_ds2760_device_info(psy); \
> ds2760_battery_read_status(di); \
> return sprintf(buf, fmt, var); \
> }
>
> DS2760_CALLBACK(voltage_now, "%d\n", di->voltage_uV)
> DS2760_CALLBACK(current_now, "%d\n", di->current_uA)
>
> etc.. but, I'm not trying to compress lines of code, I'm trying
> to ensure things are readable.
Hehe, look: http://lkml.org/lkml/2007/4/11/397
These macros are indeed what I've tried to avoid, dozen open-coded
similar functions not a good option either. I also tried to avoid
"function per property" stuff...
[lots of sense snipped]
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.
Thanks!
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Mon, 17 Dec 2007 14:24:16 +0300
Anton Vorontsov <[email protected]> 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,
};
On Tue, Dec 18, 2007 at 02:10:01AM -0500, Andres Salomon wrote:
> On Mon, 17 Dec 2007 14:24:16 +0300
> Anton Vorontsov <[email protected]> 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?
Why so difficult? Maybe like this:
diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
index c998e68..00f0b71 100644
--- a/drivers/power/olpc_battery.c
+++ b/drivers/power/olpc_battery.c
@@ -176,13 +176,13 @@ static int olpc_bat_get_property(struct power_supply *psy,
switch (ec_byte >> 4) {
case 1:
- val->strval = "Gold Peak";
+ ret = sprintf(val->strval, "%s\n", "Gold Peak");
break;
case 2:
- val->strval = "BYD";
+ ret = sprintf(val->strval, "%s\n", "BYD");
break;
default:
- val->strval = "Unknown";
+ ret = sprintf(val->strval, "%s\n", "Unknown");
break;
}
break;
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index 249f61b..83e127d 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -54,7 +54,9 @@ static ssize_t power_supply_show_property(struct device *dev,
ssize_t ret;
struct power_supply *psy = dev_get_drvdata(dev);
const ptrdiff_t off = attr - power_supply_attrs;
- union power_supply_propval value;
+ union power_supply_propval value = {
+ .strval = buf,
+ };
ret = psy->get_property(psy, off, &value);
@@ -75,7 +77,7 @@ static ssize_t power_supply_show_property(struct device *dev,
return sprintf(buf, "%s\n",
capacity_level_text[value.intval]);
else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
- return sprintf(buf, "%s\n", value.strval);
+ return ret;
return sprintf(buf, "%d\n", value.intval);
}
On Wed, 19 Dec 2007 15:35:46 +0300
Anton Vorontsov <[email protected]> wrote:
> On Tue, Dec 18, 2007 at 02:10:01AM -0500, Andres Salomon wrote:
> > On Mon, 17 Dec 2007 14:24:16 +0300
> > Anton Vorontsov <[email protected]> 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?
>
> Why so difficult? Maybe like this:
>
The point is to get rid of 'propval', and having the core driver define
formats. That's one of the places where we ran into problems with the
current API; by having the core driver define what type a property should
be returning, we limit battery drivers to what they can display, as well
as encourage a lot of non-shared code to end up in the core driver. That's
the reason why we strcpy into 'buf', rather than val->strval.
For transitioning, we could certainly just use val->strval all of the time,
but there's not much point in doing that in the long term; we might as well
just pass around 'buf'.
> diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> index c998e68..00f0b71 100644
> --- a/drivers/power/olpc_battery.c
> +++ b/drivers/power/olpc_battery.c
> @@ -176,13 +176,13 @@ static int olpc_bat_get_property(struct power_supply *psy,
>
> switch (ec_byte >> 4) {
> case 1:
> - val->strval = "Gold Peak";
> + ret = sprintf(val->strval, "%s\n", "Gold Peak");
> break;
> case 2:
> - val->strval = "BYD";
> + ret = sprintf(val->strval, "%s\n", "BYD");
> break;
> default:
> - val->strval = "Unknown";
> + ret = sprintf(val->strval, "%s\n", "Unknown");
> break;
> }
> break;
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index 249f61b..83e127d 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -54,7 +54,9 @@ static ssize_t power_supply_show_property(struct device *dev,
> ssize_t ret;
> struct power_supply *psy = dev_get_drvdata(dev);
> const ptrdiff_t off = attr - power_supply_attrs;
> - union power_supply_propval value;
> + union power_supply_propval value = {
> + .strval = buf,
> + };
>
> ret = psy->get_property(psy, off, &value);
>
> @@ -75,7 +77,7 @@ static ssize_t power_supply_show_property(struct device *dev,
> return sprintf(buf, "%s\n",
> capacity_level_text[value.intval]);
> else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
> - return sprintf(buf, "%s\n", value.strval);
> + return ret;
>
> return sprintf(buf, "%d\n", value.intval);
> }
On Wed, Dec 19, 2007 at 01:02:41PM -0500, Andres Salomon wrote:
[...]
> > > 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?
> >
> > Why so difficult? Maybe like this:
> >
>
> The point is to get rid of 'propval', and having the core driver define
> formats. That's one of the places where we ran into problems with the
> current API; by having the core driver define what type a property should
> be returning, we limit battery drivers to what they can display, as well
Limiting is:
- I want to do A.
- I won't let you do that.
But you don't want convert and write integer attributes directly to
sysfs.
If you do so, class will end up converting everything back from
strings to integers (as in _leds case. And another example, though
bad one, is drivers/power/apm_power.c).
> as encourage a lot of non-shared code to end up in the core driver. That's
> the reason why we strcpy into 'buf', rather than val->strval.
When properties are pure integers why you so eager to fill sysfs by
yourself?
You don't want to pad "voltage" property with zeroes. Probably you
want free-form S/N property. Ok, it fits quite well in the purposed
scheme: fill strval.
Can you imagine an use case when this approach doesn't work?
> For transitioning, we could certainly just use val->strval all of the time,
> but there's not much point in doing that in the long term; we might as well
> just pass around 'buf'.
No, we don't need strval for every property. Most of them are integers,
and power_supply_sysfs.c doing its small job: representing internal
structure to sysfs, through strings. Strings aren't there in the
first place.
> > diff --git a/drivers/power/olpc_battery.c b/drivers/power/olpc_battery.c
> > index c998e68..00f0b71 100644
> > --- a/drivers/power/olpc_battery.c
> > +++ b/drivers/power/olpc_battery.c
> > @@ -176,13 +176,13 @@ static int olpc_bat_get_property(struct power_supply *psy,
> >
> > switch (ec_byte >> 4) {
> > case 1:
> > - val->strval = "Gold Peak";
> > + ret = sprintf(val->strval, "%s\n", "Gold Peak");
> > break;
> > case 2:
> > - val->strval = "BYD";
> > + ret = sprintf(val->strval, "%s\n", "BYD");
> > break;
> > default:
> > - val->strval = "Unknown";
> > + ret = sprintf(val->strval, "%s\n", "Unknown");
> > break;
> > }
> > break;
> > diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> > index 249f61b..83e127d 100644
> > --- a/drivers/power/power_supply_sysfs.c
> > +++ b/drivers/power/power_supply_sysfs.c
> > @@ -54,7 +54,9 @@ static ssize_t power_supply_show_property(struct device *dev,
> > ssize_t ret;
> > struct power_supply *psy = dev_get_drvdata(dev);
> > const ptrdiff_t off = attr - power_supply_attrs;
> > - union power_supply_propval value;
> > + union power_supply_propval value = {
> > + .strval = buf,
> > + };
> >
> > ret = psy->get_property(psy, off, &value);
> >
> > @@ -75,7 +77,7 @@ static ssize_t power_supply_show_property(struct device *dev,
> > return sprintf(buf, "%s\n",
> > capacity_level_text[value.intval]);
> > else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
> > - return sprintf(buf, "%s\n", value.strval);
> > + return ret;
> >
> > return sprintf(buf, "%d\n", value.intval);
> > }
>
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Wed, 19 Dec 2007 21:50:50 +0300
Anton Vorontsov <[email protected]> wrote:
> On Wed, Dec 19, 2007 at 01:02:41PM -0500, Andres Salomon wrote:
> [...]
> > > > 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?
> > >
> > > Why so difficult? Maybe like this:
> > >
> >
> > The point is to get rid of 'propval', and having the core driver define
> > formats. That's one of the places where we ran into problems with the
> > current API; by having the core driver define what type a property should
> > be returning, we limit battery drivers to what they can display, as well
>
> Limiting is:
> - I want to do A.
> - I won't let you do that.
>
> But you don't want convert and write integer attributes directly to
> sysfs.
>
> If you do so, class will end up converting everything back from
> strings to integers (as in _leds case. And another example, though
> bad one, is drivers/power/apm_power.c).
Yeah, I thought the _leds str_to_int stuff was ugly, but I didn't see
a way around it without sacrificing flexibility.
>
> > as encourage a lot of non-shared code to end up in the core driver. That's
> > the reason why we strcpy into 'buf', rather than val->strval.
>
> When properties are pure integers why you so eager to fill sysfs by
> yourself?
>
The issue is more about forcing power_supply users to return an integer.
By passing them 'buf', they are allowed to fill it with whatever they
want. OTOH, if we force (for example) CHARGE_FULL to return an integer,
then a driver that wants to format something differently doesn't have
the opportunity to do that. I can't think of a case w/ CHARGE_FULL
where we'd want to do that, but the point is more that we shouldn't
assume we'll always know better than someone who's writing a driver
for their particular hardware. Maybe we want to return a really large
number? Maybe we want to return a hex number? Perhaps we want to
pad the number with 0s? etc..
> You don't want to pad "voltage" property with zeroes. Probably you
> want free-form S/N property. Ok, it fits quite well in the purposed
> scheme: fill strval.
>
> Can you imagine an use case when this approach doesn't work?
Sure, using a strval is fine *as long* as there's a way to say "this
attribute should be passed a buf". By having this code in
power_supply_{sysfs,core}.c, we're ensuring that users do not have the
flexibility to define such things.
>
> > For transitioning, we could certainly just use val->strval all of the time,
> > but there's not much point in doing that in the long term; we might as well
> > just pass around 'buf'.
>
> No, we don't need strval for every property. Most of them are integers,
> and power_supply_sysfs.c doing its small job: representing internal
> structure to sysfs, through strings. Strings aren't there in the
> first place.
Yes, most are integers, but we could certainly end up with others.
I've already pointed out how the API falls down because it attempts
to be too restrictive.
One of the reasons I proposed this API is because it has sysfs call
the power supply attributes directly. If we attempt to have a wrapper
for struct device_attribute in power_supply_sysfs.c, we either a) force
power_supply core to determine what type/format each property, and
don't allow power_supply drivers to override that/specify their own
types, or b) include a lot of complexity to allow power_supply drivers
to specify what properties are returning. (a) is what the current
API does, and it ends up not being flexible enough, and I'm having
a tough time wrapping my head around how to implement (b).
My preferred choice of API has power_supply drivers have callbacks
that are directly called by sysfs.
Hi Andres,
On Wed, Dec 19, 2007 at 06:13:04PM -0500, Andres Salomon wrote:
> On Wed, 19 Dec 2007 21:50:50 +0300
> Anton Vorontsov <[email protected]> wrote:
>
> > On Wed, Dec 19, 2007 at 01:02:41PM -0500, Andres Salomon wrote:
> > [...]
> > > > > 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?
> > > >
> > > > Why so difficult? Maybe like this:
> > > >
> > >
> > > The point is to get rid of 'propval', and having the core driver define
> > > formats. That's one of the places where we ran into problems with the
> > > current API; by having the core driver define what type a property should
> > > be returning, we limit battery drivers to what they can display, as well
> >
> > Limiting is:
> > - I want to do A.
> > - I won't let you do that.
> >
> > But you don't want convert and write integer attributes directly to
> > sysfs.
> >
> > If you do so, class will end up converting everything back from
> > strings to integers (as in _leds case. And another example, though
> > bad one, is drivers/power/apm_power.c).
>
> Yeah, I thought the _leds str_to_int stuff was ugly, but I didn't see
> a way around it without sacrificing flexibility.
Please, stop saying "flexibility". ;-) Better "I needed this, that's
why I've done that".
You needed S/N property, I've gave you the solution without any
drastic or conceptual change to the whole subsystem.
Not that I don't want conceptual changes, I just don't see any need
for them. And no, I don't think that purposed changes are any
better then existing properties.
> > > as encourage a lot of non-shared code to end up in the core driver. That's
> > > the reason why we strcpy into 'buf', rather than val->strval.
> >
> > When properties are pure integers why you so eager to fill sysfs by
> > yourself?
> >
>
> The issue is more about forcing power_supply users to return an integer.
This isn't the issue. This is what userspace expects. And no, we don't
want to some driver return "too much" from the "current_now" property.
This isn't legitimate value for that property!
> By passing them 'buf', they are allowed to fill it with whatever they
> want.
Why do they want silly things in the first place?
> OTOH, if we force (for example) CHARGE_FULL to return an integer,
> then a driver that wants to format something differently doesn't have
> the opportunity to do that.
This is _good_. And we've done it expressly:
- - - Documentation/power_supply_class.txt
So, userspace gets predictable set of attributes and their units for any
kind of power supply, and can process/present them to a user in
consistent manner. Results for different power supplies and machines are
also directly comparable.
- - - -
> I can't think of a case w/ CHARGE_FULL where we'd want to do that, but
> the point is more that we shouldn't assume we'll always know better
> than someone who's writing a driver for their particular hardware.
See the quote above. If driver implements something special, it must
create its own attribute. Via device_create_file. So far none driver
(including OLPC) needed any special attribute that we wouldn't add
to the "core set".
> Maybe we want to return a really large number?
Did you reach int limit already? I can change the int to long long
in a jiffy. Or add new member to the union, with "bigint" name.
I don't see any problem if one day we will face int limit.
> Maybe we want to return a hex number?
No way. What is that for? Name the property.
> Perhaps we want to pad the number with 0s? etc..
For what kind of property? Does it exist?
> > You don't want to pad "voltage" property with zeroes. Probably you
> > want free-form S/N property. Ok, it fits quite well in the purposed
> > scheme: fill strval.
> >
> > Can you imagine an use case when this approach doesn't work?
>
> Sure, using a strval is fine *as long* as there's a way to say "this
> attribute should be passed a buf". By having this code in
> power_supply_{sysfs,core}.c, we're ensuring that users do not have the
> flexibility to define such things.
^^^^^^^^^^^
s/flexibility/anarchy/
Yes, we're disciplining drivers, so they will look uniform outside of
the kernel. If driver needs not to be uniform, it's free to define its
own internal attributes in _addition_ to uniform ones. And again,
no single driver ever implemented this yet, and no one ever asked to
add new attribute to the "core set". And obviously no one got a refuse
to add one.
If you want to add S/N attribute to the core set -- go ahead, this
is long awaited addition. S/N is okay to be a free-form, thus
strval is a correct choice for that property.
> > > For transitioning, we could certainly just use val->strval all of the time,
> > > but there's not much point in doing that in the long term; we might as well
> > > just pass around 'buf'.
> >
> > No, we don't need strval for every property. Most of them are integers,
> > and power_supply_sysfs.c doing its small job: representing internal
> > structure to sysfs, through strings. Strings aren't there in the
> > first place.
>
>
> Yes, most are integers, but we could certainly end up with others.
When we'll end up with "others", we'll implement another attribute.
If it needs to be free-form, it will use strval. If it will need
to be a hex number, it will be 'int' and _sysfs will represent it
as hex, for _all_ drivers.
> I've already pointed out how the API falls down because it attempts
> to be too restrictive.
You didn't point how that api falls down, really. You're talking
about maybes and what-ifs.
> One of the reasons I proposed this API is because it has sysfs call
> the power supply attributes directly. If we attempt to have a wrapper
> for struct device_attribute in power_supply_sysfs.c, we either
> a) force power_supply core to determine what type/format each property,
> and don't allow power_supply drivers to override that/specify their own
> types, or
Correct.
> b) include a lot of complexity to allow power_supply drivers
> to specify what properties are returning.
Then, what the power supply subsystem is for? Just place all the
drivers together in driver/power/, and let them create sysfs
attributes by their own. You'll get a medley, not the subsystem.
Good luck,
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2
On Thu, 20 Dec 2007 18:07:16 +0300
Anton Vorontsov <[email protected]> wrote:
> Hi Andres,
>
[...]
>
> Then, what the power supply subsystem is for? Just place all the
> drivers together in driver/power/, and let them create sysfs
> attributes by their own. You'll get a medley, not the subsystem.
>
> Good luck,
>
Ok, I'm really tired of arguing about this. I've pointed out why
the current API is inadequate, but you seem to be resisting
changing it. It's your subsystem, do what you like.
On Thu, Dec 20, 2007 at 11:00:30AM -0500, Andres Salomon wrote:
> On Thu, 20 Dec 2007 18:07:16 +0300
> Anton Vorontsov <[email protected]> wrote:
>
> > Hi Andres,
> >
> [...]
> >
> > Then, what the power supply subsystem is for? Just place all the
> > drivers together in driver/power/, and let them create sysfs
> > attributes by their own. You'll get a medley, not the subsystem.
> >
> > Good luck,
> >
>
>
> Ok, I'm really tired of arguing about this.
Me either, you wouldn't believe.
> I've pointed out why
> the current API is inadequate, but you seem to be resisting
> changing it.
No, you didn't point out anything. You didn't describe _why_ you
want to change the subsystem. No single reason except "flexibility"
that _no one_ is using.
> It's your subsystem, do what you like.
No, this isn't mine subsystem. I've written most of it, probably.
But now I'm just volunteering to co-maintain it (that is, the person
whom to blame if something goes wrong in power/). I'm also directly
interested in this subsystem, because I have the hardware on which
I'm using it.
But I'm not the last person you can ask to merge your changes. Whole
LKML is hearing us, and if you want to--ask David to push your
changes over me. Or Andrew. Or ask Linus at the last. They're are
much better programmers than I am, so whatever they'll do would be
the right thing. I'll just swallow it.
Though, on the current patches, please stamp my:
Nacked-by: Anton Vorontsov <[email protected]>
so the history will have written evidence that I did not agree on
the changes, so years later I could reply to the complaints: "Look,
I nacked this code, it wasn't me who checked this in!"
You have another option though: finally show up the user of the
purposed changes. Without single "flexible" word, please. Real,
existent user.
--
Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2