| Message ID | 20240908192303.151562-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: Add new "charge_types" property | expand |
Hi Sebastian, On 9/17/24 9:38 AM, Sebastian Reichel wrote: > Hello Hans, > > On Sun, Sep 08, 2024 at 09:23:00PM GMT, Hans de Goede wrote: >> Some enum style power-supply properties have text-values / labels for some >> of the enum values containing a space, e.g. "Long Life" for >> POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. >> >> Make power_supply_show_enum_with_available() surround these label with "" >> when the label is not for the active enum value to make it clear that this >> is a single label and not 2 different labels for 2 different enum values. >> >> After this the output for a battery which support "Long Life" will be e.g.: >> >> Fast [Standard] "Long Life" >> >> or: >> >> Fast Standard [Long Life] >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- > > This looks annoying from parsing point of view. Maybe we can just > replace " " with "_" and guarantee that space is a value separator > at the cost of the values not being exactly the same as the existing > charge_type sysfs file? My thinking here was that if a parser wants to see if a certain option is available it can just do a strstr() and parsing for the active value does not change. But yes a parser which wants to tokenize the string to get all possible values as separate tokens will become harder to write with this. I did consider moving to using a "_" one problem there is that this means that echo-ing "Long_Life" to set the value should then work. Which would require special handling in the generic store() function. I guess we could makle I guess an easy solution here would be to define a second set of POWER_SUPPLY_CHARGE_TYPE_TEXT[] strings aptly named POWER_SUPPLY_CHARGE_TYPES_TEXT[] (with the extra s). This can then simply contain Long_Life instead of Long Life, downside of this would be that writing "Long Life" will then not work. So charge_type then takes "Long Life" and charge_types "Long_Life" which is less then ideal. The best I can come up with is to replace " " with _ when showing and in power_supply_store_property() add some special handling for charge_types like this: /* Accept "Long_Life" as alt for "Long Life" for "charge_types" */ if (dev_attr_psp(attr) == POWER_SUPPLY_PROP_CHARGE_TYPES && sysfs_streq(buf, "Long_Life")) buf = "Long Life"; ret = -EINVAL; if (ps_attr->text_values_len > 0) { ret = __sysfs_match_string(ps_attr->text_values, ps_attr->text_values_len, buf); } This isn't pretty, but this way we don't need to define a second set of POWER_SUPPLY_CHARGE_TYPES_TEXT[] values, duplicating those (and needing to manually keep them in sync), while accepting both "Long Life" and "Long_Life". Note that a generic replacing of _ with space or the other way around in store() will not work because we already allow/use "Not Charging" but also "PD_DRP" so replacing either _ or space with the other will break one of those. > Do you know if there is prior examples for this in the kernel by > chance? I'm not aware of any examples where a sysfs attr show() uses the show all available values with the active one surrounding by [ ] where one of the values has a space in it. It is quite easy to avoid the values having spaces if this format is used from the start. Regards, Hans
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c index 16b3c5880cd8..ac42784eab11 100644 --- a/drivers/power/supply/power_supply_sysfs.c +++ b/drivers/power/supply/power_supply_sysfs.c @@ -253,7 +253,10 @@ static ssize_t power_supply_show_enum_with_available( count += sysfs_emit_at(buf, count, "[%s] ", labels[i]); match = true; } else if (available) { - count += sysfs_emit_at(buf, count, "%s ", labels[i]); + if (strchr(labels[i], ' ')) + count += sysfs_emit_at(buf, count, "\"%s\" ", labels[i]); + else + count += sysfs_emit_at(buf, count, "%s ", labels[i]); } }
Some enum style power-supply properties have text-values / labels for some of the enum values containing a space, e.g. "Long Life" for POWER_SUPPLY_CHARGE_TYPE_LONGLIFE. Make power_supply_show_enum_with_available() surround these label with "" when the label is not for the active enum value to make it clear that this is a single label and not 2 different labels for 2 different enum values. After this the output for a battery which support "Long Life" will be e.g.: Fast [Standard] "Long Life" or: Fast Standard [Long Life] Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/power/supply/power_supply_sysfs.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)