Hi Hans!
On 2024-12-11 17:11:49+0100, Hans de Goede wrote:
> On 10-Dec-24 5:50 PM, Thomas Weißschuh wrote:
> > On 2024-12-09 21:40:49+0100, Hans de Goede wrote:
> >> Add a new "charge_types" property, this is identical to "charge_type" but
> >> reading returns a list of supported charge-types with the currently active
> >> type surrounded by square brackets, e.g.:
> >>
> >> Fast [Standard] "Long_Life"
> >>
> >> This has the advantage over the existing "charge_type" property that this
> >> allows userspace to find out which charge-types are supported for writable
> >> charge_type properties.
> >>
> >> Drivers which already support "charge_type" can easily add support for
> >> this by setting power_supply_desc.charge_types to a bitmask representing
> >> valid charge_type values. The existing "charge_type" get_property() and
> >> set_property() code paths can be re-used for "charge_types".
> >
> > IMO it would be nice to have the psy core implement "charge_type" in
> > terms of "charge_types" if that is available.
> > (And reject a custom "charge_type" in that case)
>
> I'm afraid that adding support to the core for that is going to be
> tricky / ugly. While as supporting both in the same driver is trivial.
I didn't look into the implementation. If you say it is ugly I believe
you.
> Also this raises the question if we even want new drivers to have
> charge_type at all ? For old drivers we want to keep it to not break
> userspace ABI. But for new drivers arguably only having
> the new charge_types attribute is better ?
Makes sense.
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> - Add "Check charge_types to get the values supported by the battery."
> >> to Documentation/ABI/testing/sysfs-class-power
> >> - Add a note about labels with spaces having these replaced by '_'
> >> to Documentation/ABI/testing/sysfs-class-power
> >> - Use power_supply_match_string() in power_supply_charge_types_parse()
> >> ---
> >> Documentation/ABI/testing/sysfs-class-power | 20 ++++++++++++
> >> drivers/power/supply/power_supply_sysfs.c | 34 +++++++++++++++++++++
> >> include/linux/power_supply.h | 23 +++++++++++++-
> >> 3 files changed, 76 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> >> index 45180b62d426..4421815cfb40 100644
> >> --- a/Documentation/ABI/testing/sysfs-class-power
> >> +++ b/Documentation/ABI/testing/sysfs-class-power
> >> @@ -407,10 +407,30 @@ Description:
> >>
> >> Access: Read, Write
> >>
> >> + Reading this returns the current active value, e.g. 'Standard'.
> >> + Check charge_types to get the values supported by the battery.
> >> +
> >> Valid values:
> >> "Unknown", "N/A", "Trickle", "Fast", "Standard",
> >> "Adaptive", "Custom", "Long Life", "Bypass"
> >>
> >> +What: /sys/class/power_supply/<supply_name>/charge_types
> >> +Date: November 2024
> >> +Contact: linux-pm@xxxxxxxxxxxxxxx
> >> +Description:
> >> + Identical to charge_type but reading returns a list of supported
> >> + charge-types with the currently active type surrounded by square
> >> + brackets, e.g.: "Fast [Standard] Long_Life".
> >> +
> >> + power_supply class devices may support both charge_type and
> >> + charge_types for backward compatibility. In this case both will
> >> + always have the same active value and the active value can be
> >> + changed by writing either property.
> >> +
> >> + Note charge-types which contain a space such as "Long Life" will
> >> + have the space replaced by a '_' resulting in e.g. "Long_Life".
> >> + When writing charge-types both variants are accepted.
> >> +
> >> What: /sys/class/power_supply/<supply_name>/charge_term_current
> >> Date: July 2014
> >> Contact: linux-pm@xxxxxxxxxxxxxxx
> >> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> >> index 034e1848f1ca..48a9bf791a38 100644
> >> --- a/drivers/power/supply/power_supply_sysfs.c
> >> +++ b/drivers/power/supply/power_supply_sysfs.c
> >> @@ -182,6 +182,8 @@ static struct power_supply_attr power_supply_attrs[] __ro_after_init = {
> >> POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
> >> POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> >> POWER_SUPPLY_ENUM_ATTR(CHARGE_BEHAVIOUR),
> >> + /* Same enum value texts as "charge_type" without the 's' at the end */
> >> + _POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPES, POWER_SUPPLY_CHARGE_TYPE_TEXT),
> >> POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
> >> POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
> >> POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> >> @@ -335,6 +337,10 @@ static ssize_t power_supply_show_property(struct device *dev,
> >> ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> >> value.intval, buf);
> >> break;
> >> + case POWER_SUPPLY_PROP_CHARGE_TYPES:
> >
> > This should only show a single value for uevent. The same as
> > charge_behaviours above (done in a recent change).
>
> Ack, I'll look at what you have done for charge_behaviors and replicate
> that for v4 of the series.
>
> <snip>
>
> >> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> >> index b98106e1a90f..60c7f669ec3e 100644
> >> --- a/include/linux/power_supply.h
> >> +++ b/include/linux/power_supply.h
> >> @@ -40,7 +40,7 @@ enum {
> >> };
> >>
> >> /* What algorithm is the charger using? */
> >> -enum {
> >> +enum power_supply_charge_type {
> >> POWER_SUPPLY_CHARGE_TYPE_UNKNOWN = 0,
> >> POWER_SUPPLY_CHARGE_TYPE_NONE,
> >> POWER_SUPPLY_CHARGE_TYPE_TRICKLE, /* slow speed */
> >> @@ -135,6 +135,7 @@ enum power_supply_property {
> >> POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, /* in percents! */
> >> POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, /* in percents! */
> >> POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> >> + POWER_SUPPLY_PROP_CHARGE_TYPES,
> >
> > Could the related enum values be kept together?
>
> I but this after POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR since it
> is somewhat related to it, also see your uevent remark.
>
> But since the enum values are kernel internal only this
> can easily be changed. I assume you want this to be
> directly after POWER_SUPPLY_PROP_CHARGE_TYPE instead ?
That was my idea. But it doesn't matter much anyways.
Regards,
Thomas
