power: RFC: introduce a new power API
PATCH 1/5 power: RFC: introduce a new power API
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.
--- 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(-)
drivers/power/power_supply_core.c +9 -0
@@ -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; }
drivers/power/power_supply_leds.c +18 -0
@@ -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);
drivers/power/power_supply_sysfs.c +123 -0
169 lines not on this page · raw message
include/linux/power_supply.h +96 -0
126 lines not on this page · raw message
Signature · 1 line
1.5.3.5
Quoted 12 lines · Andres Salomon
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner. For example, rather than having a function with a huge switch statement (asolpc_batterycurrently 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.
Quoted 2 lines
If there is interest in this API, I'll convert the rest of the
power_supplydrivers over to it and resubmit patches.
Looks sane enough to me. If Anton has no objections, I'll merge it.
Quoted 2 lines
Ignore the
psy->num_propertiesindentation below; that was done so patch #4 wasn't stupidly large.
Interesting... but I suppose it makes sense.
Signature · 1 line
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.
Quoted 3 lines · David Woodhouse
Quoted 2 lines · Andres Salomon
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.
I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 3 lines
Quoted 3 lines
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).
Quoted 4 lines
Quoted 2 lines
If there is interest in this API, I'll convert the rest of the
power_supplydrivers over to it and resubmit patches.Looks sane enough to me.
Heh..
Quoted 1 line
If Anton has no objections, I'll merge it.
Sorry, lots of objections.
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 14 lines · On Mon, 17 Dec 2007 08:51:23 +0300 Anton Vorontsov <[email protected]>…
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.
Quoted 3 lines · David Woodhouse
Quoted 2 lines · Andres Salomon
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.
Quoted 8 lines
I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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.
Quoted 12 lines
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.
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
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?
Quoted 15 lines
Quoted 3 lines
Quoted 3 lines
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
_muchmore_space than these dozen bytes!)
Quoted 5 lines
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.
Quoted 30 lines
(quoted diff) +25
-0+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; +}
Quoted 2 lines
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..
Quoted 11 lines
I don't see S/N function. I'm sure it could be implemented easily using today's properties approach.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
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.
Quoted 6 lines
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).
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
If there is interest in this API, I'll convert the rest of the
power_supplydrivers over to it and resubmit patches.
Quoted 8 lines
Quoted 1 line
Looks sane enough to me.
Heh..
Quoted 1 line
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:
[...]
Quoted 42 lines
Quoted 4 lines
Quoted 3 lines · Andres Salomon
Quoted 2 lines
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.Quoted 8 lines
I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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_tds2760_battery_get_status(struct device *dev,
structdevice_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); \ }
Quoted 5 lines
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!
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 15 lines · 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:
[...]Quoted 13 lines
Quoted 12 lines
Quoted 3 lines · Andres Salomon
Quoted 2 lines
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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.
[...]
Quoted 7 lines
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?
drivers/power/olpc_battery.c +86 -77
@@ -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 , };
Quoted 27 lines · Andres Salomon
Quoted 15 lines · 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:
[...]Quoted 13 lines
Quoted 12 lines
Quoted 3 lines · Andres Salomon
Quoted 2 lines
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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.[...]
Quoted 7 lines
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.
Quoted 4 lines
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:
drivers/power/olpc_battery.c +3 -3
@@ -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 ;
drivers/power/power_supply_sysfs.c +4 -2
@@ -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); }
Quoted 28 lines · On Wed, 19 Dec 2007 15:35:46 +0300 Anton Vorontsov <[email protected]> wrote:
Quoted 27 lines · Andres Salomon
Quoted 15 lines · 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:
[...]Quoted 13 lines
Quoted 12 lines
Quoted 3 lines · Andres Salomon
Quoted 2 lines
This API has the
power_supplydrivers device their owndevice_attributelist; I find this to be a lot more flexible and cleaner.I don't see how this is more flexible and cleaner. See below.
Quoted 3 lines
Quoted 3 lines
For example,
rather than having a function with a huge switch statement (asolpc_batterycurrently 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.[...]
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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'.
Quoted 45 lines
drivers/power/olpc_battery.c +3
-3@@ -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 ;drivers/power/power_supply_sysfs.c +4
-2@@ -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:
[...]
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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?
Quoted 7 lines
Quoted 2 lines
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).
Quoted 2 lines
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?
Quoted 3 lines
For transitioning, we could certainly just use
val->strvalall 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.
Quoted 46 lines
Quoted 45 lines
drivers/power/olpc_battery.c +3
-3@@ -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 ;drivers/power/power_supply_sysfs.c +4
-2@@ -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); }
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 19 lines · 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:
[...]Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
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
_ledscase. And another example, though bad one, isdrivers/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.
Quoted 7 lines
Quoted 3 lines
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..
Quoted 5 lines
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.
Quoted 9 lines
Quoted 4 lines
For transitioning, we could certainly just use
val->strvalall 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.cdoing 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,
Quoted 25 lines · Andres Salomon
Quoted 22 lines · 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:
[...]Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
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?
Quoted 7 lines
Quoted 2 lines
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.
Quoted 6 lines
Quoted 3 lines
If you do so, class will end up converting everything back from strings to integers (as in
_ledscase. And another example, though bad one, isdrivers/power/apm_power.c).Yeah, I thought the
_ledsstr_to_intstuff 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.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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_supplyusers 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!
Quoted 2 lines
By passing them 'buf', they are allowed to fill it with whatever they want.
Why do they want silly things in the first place?
Quoted 3 lines
OTOH, if we force (for example)
CHARGE_FULLto 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.
- - - -
Quoted 3 lines
I can't think of a case w/
CHARGE_FULLwhere 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".
Quoted 1 line
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.
Quoted 1 line
Maybe we want to return a hex number?
No way. What is that for? Name the property.
Quoted 1 line
Perhaps we want to pad the number with 0s? etc..
For what kind of property? Does it exist?
Quoted 10 lines
Quoted 5 lines
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.
Quoted 11 lines
Quoted 8 lines
Quoted 3 lines
For transitioning, we could certainly just use
val->strvalall 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.cdoing 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.
Quoted 2 lines
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.
Quoted 7 lines
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_attributeinpower_supply_sysfs.c, we eithera) force
power_supplycore to determine what type/format each property, and don't allowpower_supplydrivers to override that/specify their own types, or
Correct.
Quoted 2 lines
b) include a lot of complexity to allow
power_supplydrivers 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,
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2
Quoted 2 lines · On Thu, 20 Dec 2007 18:07:16 +0300 Anton Vorontsov <[email protected]> wrote:
Hi Andres,
[...]
Quoted 7 lines
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.
Quoted 11 lines · Andres Salomon
Quoted 2 lines · On Thu, 20 Dec 2007 18:07:16 +0300 Anton Vorontsov <[email protected]> wrote:
Hi Andres,
[...]
Quoted 5 lines
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.
Quoted 5 lines
Quoted 2 lines
Good luck,
Ok, I'm really tired of arguing about this.
Me either, you wouldn't believe.
Quoted 3 lines
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.
Quoted 1 line
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:
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.
Signature · 4 lines
Anton Vorontsov email: [email protected] backup email: [email protected] irc://irc.freenode.net/bd2