[PATCH v4 0/4] power: supply: Add new "charge_types" property

[PATCH v4 0/4] power: supply: Add new "charge_types" property

[Hans de Goede]

Hi,

As first discussed here:
https://lore.kernel.org/linux-pm/49993a42-aa91-46bf-acef-4a089db4c2db@redhat.com/

Some power_supply devices have a writable charge_type property, but
userspace cannot know which charge_type values are valid to write.

This series adds a new "charge_types" property, which 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 allows userspace to find out which charge-types are supported.

Patch 1/4 does some prep work and patch 2/4 is the core implementation
of "charge_types" property support.

Patch 3/4 is a standard psy-driver user of the new "charge_types"
property support. Patch 4/4 shows an ACPI battery extension driver
with support for this property using the new show()/store() helpers.

Changes in v4:
- Rebase on top of latest sre/linux-power-supply.git/for-next
- Only print current value in uevent using new uevent flag passed to
  power_supply_format_property()
- Put new POWER_SUPPLY_PROP_CHARGE_TYPES enum power_supply_property entry
  directly after the existing POWER_SUPPLY_PROP_CHARGE_TYPE entry

Changes in v3:
- Rebase on top of v6.13-rc1

Changes in v2:
- Replace spaces with '_' instead of surrounding the text-value by ""
- 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 '_'
  in charge_types output to Documentation/ABI/testing/sysfs-class-power
- Use power_supply_match_string() in power_supply_charge_types_parse()

Regards,

Hans


Hans de Goede (4):
  power: supply: power_supply_show_enum_with_available(): Replace spaces
    with '_'
  power: supply: core: Add new "charge_types" property
  power: supply: bq24190_charger: Add support for "charge_types"
    property
  platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse()
    helpers

 Documentation/ABI/testing/sysfs-class-power | 20 ++++++
 drivers/platform/x86/dell/dell-laptop.c     | 54 +++++++--------
 drivers/power/supply/bq24190_charger.c      |  7 ++
 drivers/power/supply/power_supply_sysfs.c   | 73 +++++++++++++++++++--
 include/linux/power_supply.h                | 23 ++++++-
 5 files changed, 143 insertions(+), 34 deletions(-)

-- 
2.47.1

[PATCH v4 1/4] power: supply: power_supply_show_enum_with_available(): Replace spaces with '_'

[Hans de Goede]

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() replace these spaces with
'_' when showing the available text-values. After this the output for
a battery which supports "Long Life" will be e.g.:

Fast [Standard] Long_Life

or:

Fast Standard [Long_Life]

Modify power_supply_store_property() to accept both the original text-value
with space and the alternative value with the spaces replaced by '_'.
This allows users to write the value with '_' after seeing this on reading
the property.

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2
- Replace spaces with '_' instead of surrounding the text-value by ""
---
 drivers/power/supply/power_supply_sysfs.c | 37 ++++++++++++++++++++---
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 99bfe1f03eb8..b7b29ce61c34 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -237,23 +237,52 @@ static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
 	return  to_ps_attr(attr) - power_supply_attrs;
 }
 
+static void power_supply_escape_spaces(const char *str, char *buf, size_t bufsize)
+{
+	strscpy(buf, str, bufsize);
+	strreplace(buf, ' ', '_');
+}
+
+static int power_supply_match_string(const char * const *array, size_t n, const char *s)
+{
+	int ret;
+
+	/* First try an exact match */
+	ret = __sysfs_match_string(array, n, s);
+	if (ret >= 0)
+		return ret;
+
+	/* Second round, try matching with spaces replaced by '_' */
+	for (size_t i = 0; i < n; i++) {
+		char buf[32];
+
+		power_supply_escape_spaces(array[i], buf, sizeof(buf));
+		if (sysfs_streq(buf, s))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
 static ssize_t power_supply_show_enum_with_available(
 			struct device *dev, const char * const labels[], int label_count,
 			unsigned int available_values, int value, char *buf)
 {
 	bool match = false, available, active;
+	char escaped_label[32];
 	ssize_t count = 0;
 	int i;
 
 	for (i = 0; i < label_count; i++) {
 		available = available_values & BIT(i);
 		active = i == value;
+		power_supply_escape_spaces(labels[i], escaped_label, sizeof(escaped_label));
 
 		if (available && active) {
-			count += sysfs_emit_at(buf, count, "[%s] ", labels[i]);
+			count += sysfs_emit_at(buf, count, "[%s] ", escaped_label);
 			match = true;
 		} else if (available) {
-			count += sysfs_emit_at(buf, count, "%s ", labels[i]);
+			count += sysfs_emit_at(buf, count, "%s ", escaped_label);
 		}
 	}
 
@@ -344,8 +373,8 @@ static ssize_t power_supply_store_property(struct device *dev,
 
 	ret = -EINVAL;
 	if (ps_attr->text_values_len > 0) {
-		ret = __sysfs_match_string(ps_attr->text_values,
-					   ps_attr->text_values_len, buf);
+		ret = power_supply_match_string(ps_attr->text_values,
+						ps_attr->text_values_len, buf);
 	}
 
 	/*
-- 
2.47.1

[PATCH v4 2/4] power: supply: core: Add new "charge_types" property

[Hans de Goede]

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".

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Rebase on top of latest sre/linux-power-supply.git/for-next
- Only print current value in uevent using new uevent flag passed to
  power_supply_format_property()
- Put new POWER_SUPPLY_PROP_CHARGE_TYPES enum power_supply_property entry
  directly after the existing POWER_SUPPLY_PROP_CHARGE_TYPE entry

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   | 36 +++++++++++++++++++++
 include/linux/power_supply.h                | 23 ++++++++++++-
 3 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index 45180b62d426..74050dfb5fc0 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:		December 2024
+Contact:	linux-pm@vger.kernel.org
+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@vger.kernel.org
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index b7b29ce61c34..9497bc12f4cc 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),
@@ -339,6 +341,12 @@ static ssize_t power_supply_format_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:
+		if (uevent) /* no possible values in uevents */
+			goto default_format;
+		ret = power_supply_charge_types_show(dev, psy->desc->charge_types,
+						     value.intval, buf);
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sysfs_emit(buf, "%s\n", value.strval);
 		break;
@@ -556,3 +564,31 @@ int power_supply_charge_behaviour_parse(unsigned int available_behaviours, const
 	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(power_supply_charge_behaviour_parse);
+
+ssize_t power_supply_charge_types_show(struct device *dev,
+				       unsigned int available_types,
+				       enum power_supply_charge_type current_type,
+				       char *buf)
+{
+	return power_supply_show_enum_with_available(
+				dev, POWER_SUPPLY_CHARGE_TYPE_TEXT,
+				ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT),
+				available_types, current_type, buf);
+}
+EXPORT_SYMBOL_GPL(power_supply_charge_types_show);
+
+int power_supply_charge_types_parse(unsigned int available_types, const char *buf)
+{
+	int i = power_supply_match_string(POWER_SUPPLY_CHARGE_TYPE_TEXT,
+					  ARRAY_SIZE(POWER_SUPPLY_CHARGE_TYPE_TEXT),
+					  buf);
+
+	if (i < 0)
+		return i;
+
+	if (available_types & BIT(i))
+		return i;
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(power_supply_charge_types_parse);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index b98106e1a90f..a841b814743b 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 */
@@ -99,6 +99,7 @@ enum power_supply_property {
 	/* Properties of type `int' */
 	POWER_SUPPLY_PROP_STATUS = 0,
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_CHARGE_TYPES,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_ONLINE,
@@ -245,6 +246,7 @@ struct power_supply_desc {
 	const char *name;
 	enum power_supply_type type;
 	u8 charge_behaviours;
+	u32 charge_types;
 	u32 usb_types;
 	const enum power_supply_property *properties;
 	size_t num_properties;
@@ -944,6 +946,11 @@ ssize_t power_supply_charge_behaviour_show(struct device *dev,
 					   char *buf);
 
 int power_supply_charge_behaviour_parse(unsigned int available_behaviours, const char *buf);
+ssize_t power_supply_charge_types_show(struct device *dev,
+				       unsigned int available_types,
+				       enum power_supply_charge_type current_type,
+				       char *buf);
+int power_supply_charge_types_parse(unsigned int available_types, const char *buf);
 #else
 static inline
 ssize_t power_supply_charge_behaviour_show(struct device *dev,
@@ -959,6 +966,20 @@ static inline int power_supply_charge_behaviour_parse(unsigned int available_beh
 {
 	return -EOPNOTSUPP;
 }
+
+static inline
+ssize_t power_supply_charge_types_show(struct device *dev,
+				       unsigned int available_types,
+				       enum power_supply_charge_type current_type,
+				       char *buf)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int power_supply_charge_types_parse(unsigned int available_types, const char *buf)
+{
+	return -EOPNOTSUPP;
+}
 #endif
 
 #endif /* __LINUX_POWER_SUPPLY_H__ */
-- 
2.47.1

[PATCH v4 3/4] power: supply: bq24190_charger: Add support for "charge_types" property

[Hans de Goede]

The bq24190 power_supply class device has a writeable "charge_type"
property, add support for the new "charge_types" property. Reading this
returns a list of supported charge-types with the currently active type
surrounded by square brackets, allowing userspace to find out which
enum power_supply_charge_type values are supported.

This has been tested on a GPD win gaming-handheld.

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 3b97f7884967..4d96352af34c 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1313,6 +1313,7 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+	case POWER_SUPPLY_PROP_CHARGE_TYPES:
 		ret = bq24190_charger_get_charge_type(bdi, val);
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
@@ -1393,6 +1394,7 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = bq24190_charger_set_temp_alert_max(bdi, val);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+	case POWER_SUPPLY_PROP_CHARGE_TYPES:
 		ret = bq24190_charger_set_charge_type(bdi, val);
 		break;
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
@@ -1421,6 +1423,7 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
+	case POWER_SUPPLY_PROP_CHARGE_TYPES:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
 	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
@@ -1469,6 +1472,7 @@ static void bq24190_charger_external_power_changed(struct power_supply *psy)
 
 static enum power_supply_property bq24190_charger_properties[] = {
 	POWER_SUPPLY_PROP_CHARGE_TYPE,
+	POWER_SUPPLY_PROP_CHARGE_TYPES,
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_STATUS,
@@ -1498,6 +1502,9 @@ static const struct power_supply_desc bq24190_charger_desc = {
 	.set_property		= bq24190_charger_set_property,
 	.property_is_writeable	= bq24190_charger_property_is_writeable,
 	.external_power_changed	= bq24190_charger_external_power_changed,
+	.charge_types		= BIT(POWER_SUPPLY_CHARGE_TYPE_NONE)    |
+				  BIT(POWER_SUPPLY_CHARGE_TYPE_TRICKLE) |
+				  BIT(POWER_SUPPLY_CHARGE_TYPE_FAST),
 };
 
 /* Battery power supply property routines */
-- 
2.47.1

[PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Hans de Goede]

Make battery_modes a map between tokens and enum power_supply_charge_type
values instead of between tokens and strings and use the new
power_supply_charge_types_show/_parse() helpers for show()/store()
to ensure that things are handled in the same way as in other drivers.

This also changes battery_supported_modes to be a bitmap of charge-types
(enum power_supply_charge_type values) rather then a bitmap of indices
into battery_modes[].

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++-------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
index 5671bd0deee7..9a4cfcb8bbe0 100644
--- a/drivers/platform/x86/dell/dell-laptop.c
+++ b/drivers/platform/x86/dell/dell-laptop.c
@@ -103,15 +103,15 @@ static bool mute_led_registered;
 
 struct battery_mode_info {
 	int token;
-	const char *label;
+	enum power_supply_charge_type charge_type;
 };
 
 static const struct battery_mode_info battery_modes[] = {
-	{ BAT_PRI_AC_MODE_TOKEN,   "Trickle" },
-	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
-	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
-	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
-	{ BAT_CUSTOM_MODE_TOKEN,   "Custom" },
+	{ BAT_PRI_AC_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_TRICKLE },
+	{ BAT_EXPRESS_MODE_TOKEN,  POWER_SUPPLY_CHARGE_TYPE_FAST },
+	{ BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD },
+	{ BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE },
+	{ BAT_CUSTOM_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_CUSTOM },
 };
 static u32 battery_supported_modes;
 
@@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev,
 		struct device_attribute *attr,
 		char *buf)
 {
-	ssize_t count = 0;
+	enum power_supply_charge_type charge_type;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
-		bool active;
+		charge_type = battery_modes[i].charge_type;
 
-		if (!(battery_supported_modes & BIT(i)))
+		if (!(battery_supported_modes & BIT(charge_type)))
 			continue;
 
-		active = dell_battery_mode_is_active(battery_modes[i].token);
-		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
-				battery_modes[i].label);
+		if (!dell_battery_mode_is_active(battery_modes[i].token))
+			continue;
+
+		return power_supply_charge_types_show(dev, battery_supported_modes,
+						      charge_type, buf);
 	}
 
-	/* convert the last space to a newline */
-	if (count > 0)
-		count--;
-	count += sysfs_emit_at(buf, count, "\n");
-
-	return count;
+	/* No active mode found */
+	return -EIO;
 }
 
 static ssize_t charge_types_store(struct device *dev,
 		struct device_attribute *attr,
 		const char *buf, size_t size)
 {
-	bool matched = false;
-	int err, i;
+	int charge_type, err, i;
+
+	charge_type = power_supply_charge_types_parse(battery_supported_modes, buf);
+	if (charge_type < 0)
+		return charge_type;
 
 	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
-		if (!(battery_supported_modes & BIT(i)))
-			continue;
-
-		if (sysfs_streq(battery_modes[i].label, buf)) {
-			matched = true;
+		if (battery_modes[i].charge_type == charge_type)
 			break;
-		}
 	}
-	if (!matched)
-		return -EINVAL;
+	if (i == ARRAY_SIZE(battery_modes))
+		return -EIO;
 
 	err = dell_battery_set_mode(battery_modes[i].token);
 	if (err)
@@ -2430,7 +2426,7 @@ static u32 __init battery_get_supported_modes(void)
 
 	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
 		if (dell_smbios_find_token(battery_modes[i].token))
-			modes |= BIT(i);
+			modes |= BIT(battery_modes[i].charge_type);
 	}
 
 	return modes;
-- 
2.47.1

Re: [PATCH v4 2/4] power: supply: core: Add new "charge_types" property

[Thomas Weißschuh]

On 2024-12-11 18:44: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".
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

[..]

Re: (subset) [PATCH v4 0/4] power: supply: Add new "charge_types" property

[Sebastian Reichel]

On Wed, 11 Dec 2024 18:44:47 +0100, Hans de Goede wrote:
> As first discussed here:
> https://lore.kernel.org/linux-pm/49993a42-aa91-46bf-acef-4a089db4c2db@redhat.com/
> 
> Some power_supply devices have a writable charge_type property, but
> userspace cannot know which charge_type values are valid to write.
> 
> This series adds a new "charge_types" property, which 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.:
> 
> [...]

Applied, thanks!

[2/4] power: supply: core: Add new "charge_types" property
      commit: d24bf99214b199c25f9c2cb04b3a4993d1c7ab60
[3/4] power: supply: bq24190_charger: Add support for "charge_types" property
      commit: 5d417a5e7ade02a7b75cd886d8afe3e9025e7e25

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4478 bytes --]

On Wed, 11 Dec 2024, Hans de Goede wrote:

> Make battery_modes a map between tokens and enum power_supply_charge_type
> values instead of between tokens and strings and use the new
> power_supply_charge_types_show/_parse() helpers for show()/store()
> to ensure that things are handled in the same way as in other drivers.
> 
> This also changes battery_supported_modes to be a bitmap of charge-types
> (enum power_supply_charge_type values) rather then a bitmap of indices
> into battery_modes[].
> 
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++-------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 5671bd0deee7..9a4cfcb8bbe0 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -103,15 +103,15 @@ static bool mute_led_registered;
>  
>  struct battery_mode_info {
>  	int token;
> -	const char *label;
> +	enum power_supply_charge_type charge_type;
>  };
>  
>  static const struct battery_mode_info battery_modes[] = {
> -	{ BAT_PRI_AC_MODE_TOKEN,   "Trickle" },
> -	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
> -	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
> -	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
> -	{ BAT_CUSTOM_MODE_TOKEN,   "Custom" },
> +	{ BAT_PRI_AC_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_TRICKLE },
> +	{ BAT_EXPRESS_MODE_TOKEN,  POWER_SUPPLY_CHARGE_TYPE_FAST },
> +	{ BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD },
> +	{ BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE },
> +	{ BAT_CUSTOM_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_CUSTOM },
>  };
>  static u32 battery_supported_modes;
>  
> @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev,
>  		struct device_attribute *attr,
>  		char *buf)
>  {
> -	ssize_t count = 0;
> +	enum power_supply_charge_type charge_type;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> -		bool active;
> +		charge_type = battery_modes[i].charge_type;
>  
> -		if (!(battery_supported_modes & BIT(i)))
> +		if (!(battery_supported_modes & BIT(charge_type)))
>  			continue;
>  
> -		active = dell_battery_mode_is_active(battery_modes[i].token);
> -		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
> -				battery_modes[i].label);
> +		if (!dell_battery_mode_is_active(battery_modes[i].token))
> +			continue;
> +
> +		return power_supply_charge_types_show(dev, battery_supported_modes,
> +						      charge_type, buf);
>  	}
>  
> -	/* convert the last space to a newline */
> -	if (count > 0)
> -		count--;
> -	count += sysfs_emit_at(buf, count, "\n");
> -
> -	return count;
> +	/* No active mode found */
> +	return -EIO;
>  }
>  
>  static ssize_t charge_types_store(struct device *dev,
>  		struct device_attribute *attr,
>  		const char *buf, size_t size)
>  {
> -	bool matched = false;
> -	int err, i;
> +	int charge_type, err, i;
> +
> +	charge_type = power_supply_charge_types_parse(battery_supported_modes, buf);
> +	if (charge_type < 0)
> +		return charge_type;
>  
>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> -		if (!(battery_supported_modes & BIT(i)))
> -			continue;
> -
> -		if (sysfs_streq(battery_modes[i].label, buf)) {
> -			matched = true;
> +		if (battery_modes[i].charge_type == charge_type)
>  			break;
> -		}
>  	}
> -	if (!matched)
> -		return -EINVAL;
> +	if (i == ARRAY_SIZE(battery_modes))
> +		return -EIO;

Hi Hans,

Is this errno change helpful/correct? There is zero I/O done before 
reaching this point, just input validation, so why does it return errno 
that is "I/O error"? If you want to differentiate from -EINVAL, I suggest 
using -ENOENT (but I personally think -EINVAL would be fine as well 
because it's still an invalid argument even if it passed one stage of 
the input checks).

-- 
 i.


>  
>  	err = dell_battery_set_mode(battery_modes[i].token);
>  	if (err)
> @@ -2430,7 +2426,7 @@ static u32 __init battery_get_supported_modes(void)
>  
>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>  		if (dell_smbios_find_token(battery_modes[i].token))
> -			modes |= BIT(i);
> +			modes |= BIT(battery_modes[i].charge_type);
>  	}
>  
>  	return modes;
> 

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Hans de Goede]

Hi,

On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote:
> On Wed, 11 Dec 2024, Hans de Goede wrote:
> 
>> Make battery_modes a map between tokens and enum power_supply_charge_type
>> values instead of between tokens and strings and use the new
>> power_supply_charge_types_show/_parse() helpers for show()/store()
>> to ensure that things are handled in the same way as in other drivers.
>>
>> This also changes battery_supported_modes to be a bitmap of charge-types
>> (enum power_supply_charge_type values) rather then a bitmap of indices
>> into battery_modes[].
>>
>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++-------------
>>  1 file changed, 25 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
>> index 5671bd0deee7..9a4cfcb8bbe0 100644
>> --- a/drivers/platform/x86/dell/dell-laptop.c
>> +++ b/drivers/platform/x86/dell/dell-laptop.c
>> @@ -103,15 +103,15 @@ static bool mute_led_registered;
>>  
>>  struct battery_mode_info {
>>  	int token;
>> -	const char *label;
>> +	enum power_supply_charge_type charge_type;
>>  };
>>  
>>  static const struct battery_mode_info battery_modes[] = {
>> -	{ BAT_PRI_AC_MODE_TOKEN,   "Trickle" },
>> -	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
>> -	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
>> -	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
>> -	{ BAT_CUSTOM_MODE_TOKEN,   "Custom" },
>> +	{ BAT_PRI_AC_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_TRICKLE },
>> +	{ BAT_EXPRESS_MODE_TOKEN,  POWER_SUPPLY_CHARGE_TYPE_FAST },
>> +	{ BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD },
>> +	{ BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE },
>> +	{ BAT_CUSTOM_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_CUSTOM },
>>  };
>>  static u32 battery_supported_modes;
>>  
>> @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev,
>>  		struct device_attribute *attr,
>>  		char *buf)
>>  {
>> -	ssize_t count = 0;
>> +	enum power_supply_charge_type charge_type;
>>  	int i;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>> -		bool active;
>> +		charge_type = battery_modes[i].charge_type;
>>  
>> -		if (!(battery_supported_modes & BIT(i)))
>> +		if (!(battery_supported_modes & BIT(charge_type)))
>>  			continue;
>>  
>> -		active = dell_battery_mode_is_active(battery_modes[i].token);
>> -		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
>> -				battery_modes[i].label);
>> +		if (!dell_battery_mode_is_active(battery_modes[i].token))
>> +			continue;
>> +
>> +		return power_supply_charge_types_show(dev, battery_supported_modes,
>> +						      charge_type, buf);
>>  	}
>>  
>> -	/* convert the last space to a newline */
>> -	if (count > 0)
>> -		count--;
>> -	count += sysfs_emit_at(buf, count, "\n");
>> -
>> -	return count;
>> +	/* No active mode found */
>> +	return -EIO;
>>  }
>>  
>>  static ssize_t charge_types_store(struct device *dev,
>>  		struct device_attribute *attr,
>>  		const char *buf, size_t size)
>>  {
>> -	bool matched = false;
>> -	int err, i;
>> +	int charge_type, err, i;
>> +
>> +	charge_type = power_supply_charge_types_parse(battery_supported_modes, buf);
>> +	if (charge_type < 0)
>> +		return charge_type;
>>  
>>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
>> -		if (!(battery_supported_modes & BIT(i)))
>> -			continue;
>> -
>> -		if (sysfs_streq(battery_modes[i].label, buf)) {
>> -			matched = true;
>> +		if (battery_modes[i].charge_type == charge_type)
>>  			break;
>> -		}
>>  	}
>> -	if (!matched)
>> -		return -EINVAL;
>> +	if (i == ARRAY_SIZE(battery_modes))
>> +		return -EIO;
> 
> Hi Hans,
> 
> Is this errno change helpful/correct?

power_supply_charge_types_parse() already checks that the user-input
is one of the values advertised in the passed in battery_supported_modes,
so when we loop to translate the enum power_supply_charge_type value
returned by power_supply_charge_types_parse() then we should find
a matching entry in battery_modes[] if not something is wrong at
the driver level. So not -EINVAL, since this is a driver issue not
a user input issue.

> There is zero I/O done before 
> reaching this point, just input validation, so why does it return errno 
> that is "I/O error"? If you want to differentiate from -EINVAL, I suggest 
> using -ENOENT (but I personally think -EINVAL would be fine as well 
> because it's still an invalid argument even if it passed one stage of 
> the input checks).

-ENOENT instead of -EIO works for me.

Shall I send out a new version with that changed?

Note that merging this requires the earlier patches from this
series which have been merged into:

https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next

so this either requires an immutable tag from Sebastian for you to merge,
or this should be merged through Sebastian's tree.

Regards,

Hans

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5632 bytes --]

On Tue, 17 Dec 2024, Hans de Goede wrote:
> On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote:
> > On Wed, 11 Dec 2024, Hans de Goede wrote:
> > 
> >> Make battery_modes a map between tokens and enum power_supply_charge_type
> >> values instead of between tokens and strings and use the new
> >> power_supply_charge_types_show/_parse() helpers for show()/store()
> >> to ensure that things are handled in the same way as in other drivers.
> >>
> >> This also changes battery_supported_modes to be a bitmap of charge-types
> >> (enum power_supply_charge_type values) rather then a bitmap of indices
> >> into battery_modes[].
> >>
> >> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/dell/dell-laptop.c | 54 ++++++++++++-------------
> >>  1 file changed, 25 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> >> index 5671bd0deee7..9a4cfcb8bbe0 100644
> >> --- a/drivers/platform/x86/dell/dell-laptop.c
> >> +++ b/drivers/platform/x86/dell/dell-laptop.c
> >> @@ -103,15 +103,15 @@ static bool mute_led_registered;
> >>  
> >>  struct battery_mode_info {
> >>  	int token;
> >> -	const char *label;
> >> +	enum power_supply_charge_type charge_type;
> >>  };
> >>  
> >>  static const struct battery_mode_info battery_modes[] = {
> >> -	{ BAT_PRI_AC_MODE_TOKEN,   "Trickle" },
> >> -	{ BAT_EXPRESS_MODE_TOKEN,  "Fast" },
> >> -	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
> >> -	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
> >> -	{ BAT_CUSTOM_MODE_TOKEN,   "Custom" },
> >> +	{ BAT_PRI_AC_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_TRICKLE },
> >> +	{ BAT_EXPRESS_MODE_TOKEN,  POWER_SUPPLY_CHARGE_TYPE_FAST },
> >> +	{ BAT_STANDARD_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_STANDARD },
> >> +	{ BAT_ADAPTIVE_MODE_TOKEN, POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE },
> >> +	{ BAT_CUSTOM_MODE_TOKEN,   POWER_SUPPLY_CHARGE_TYPE_CUSTOM },
> >>  };
> >>  static u32 battery_supported_modes;
> >>  
> >> @@ -2261,46 +2261,42 @@ static ssize_t charge_types_show(struct device *dev,
> >>  		struct device_attribute *attr,
> >>  		char *buf)
> >>  {
> >> -	ssize_t count = 0;
> >> +	enum power_supply_charge_type charge_type;
> >>  	int i;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> >> -		bool active;
> >> +		charge_type = battery_modes[i].charge_type;
> >>  
> >> -		if (!(battery_supported_modes & BIT(i)))
> >> +		if (!(battery_supported_modes & BIT(charge_type)))
> >>  			continue;
> >>  
> >> -		active = dell_battery_mode_is_active(battery_modes[i].token);
> >> -		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
> >> -				battery_modes[i].label);
> >> +		if (!dell_battery_mode_is_active(battery_modes[i].token))
> >> +			continue;
> >> +
> >> +		return power_supply_charge_types_show(dev, battery_supported_modes,
> >> +						      charge_type, buf);
> >>  	}
> >>  
> >> -	/* convert the last space to a newline */
> >> -	if (count > 0)
> >> -		count--;
> >> -	count += sysfs_emit_at(buf, count, "\n");
> >> -
> >> -	return count;
> >> +	/* No active mode found */
> >> +	return -EIO;
> >>  }
> >>  
> >>  static ssize_t charge_types_store(struct device *dev,
> >>  		struct device_attribute *attr,
> >>  		const char *buf, size_t size)
> >>  {
> >> -	bool matched = false;
> >> -	int err, i;
> >> +	int charge_type, err, i;
> >> +
> >> +	charge_type = power_supply_charge_types_parse(battery_supported_modes, buf);
> >> +	if (charge_type < 0)
> >> +		return charge_type;
> >>  
> >>  	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> >> -		if (!(battery_supported_modes & BIT(i)))
> >> -			continue;
> >> -
> >> -		if (sysfs_streq(battery_modes[i].label, buf)) {
> >> -			matched = true;
> >> +		if (battery_modes[i].charge_type == charge_type)
> >>  			break;
> >> -		}
> >>  	}
> >> -	if (!matched)
> >> -		return -EINVAL;
> >> +	if (i == ARRAY_SIZE(battery_modes))
> >> +		return -EIO;
> > 
> > Hi Hans,
> > 
> > Is this errno change helpful/correct?
> 
> power_supply_charge_types_parse() already checks that the user-input
> is one of the values advertised in the passed in battery_supported_modes,
> so when we loop to translate the enum power_supply_charge_type value
> returned by power_supply_charge_types_parse() then we should find
> a matching entry in battery_modes[] if not something is wrong at
> the driver level. So not -EINVAL, since this is a driver issue not
> a user input issue.
> 
> > There is zero I/O done before 
> > reaching this point, just input validation, so why does it return errno 
> > that is "I/O error"? If you want to differentiate from -EINVAL, I suggest 
> > using -ENOENT (but I personally think -EINVAL would be fine as well 
> > because it's still an invalid argument even if it passed one stage of 
> > the input checks).
> 
> -ENOENT instead of -EIO works for me.
> 
> Shall I send out a new version with that changed?
>
> Note that merging this requires the earlier patches from this
> series which have been merged into:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
> 
> so this either requires an immutable tag from Sebastian for you to merge,
> or this should be merged through Sebastian's tree.

Yes, please send a new version with -ENOENT as I was going to ask
Sebastian to take this patch but noticed this small errno thing while 
reading the patch one more time before acking it.

-- 
 i.

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Thomas Weißschuh]

Hi Hans,

On 2024-12-17 16:18:47+0100, Hans de Goede wrote:
> On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote:
> > On Wed, 11 Dec 2024, Hans de Goede wrote:
> > 
> >> Make battery_modes a map between tokens and enum power_supply_charge_type
> >> values instead of between tokens and strings and use the new
> >> power_supply_charge_types_show/_parse() helpers for show()/store()
> >> to ensure that things are handled in the same way as in other drivers.
> >>
> >> This also changes battery_supported_modes to be a bitmap of charge-types
> >> (enum power_supply_charge_type values) rather then a bitmap of indices
> >> into battery_modes[].
> >>
> >> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

[..]

> Note that merging this requires the earlier patches from this
> series which have been merged into:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
> 
> so this either requires an immutable tag from Sebastian for you to merge,
> or this should be merged through Sebastian's tree.

If this goes in via the psy tree, you could already make it a power
supply extension. The necessary code is in psy/for-next.

Not necessary obviously.


Thomas

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

Hi,

On Tue, Dec 17, 2024 at 04:18:47PM +0100, Hans de Goede wrote:
> Note that merging this requires the earlier patches from this
> series which have been merged into:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
> 
> so this either requires an immutable tag from Sebastian for you to merge,
> or this should be merged through Sebastian's tree.

Unfortunately I cannot easily create an immutable tag, which does
not pull in quite a bit of other clutter from my for-next branch
(or require a big rebase of everything in my for-next branch, which
I usually try to avoid). I noticed too late that it would have been
a good idea to merge all of this through a topic branch.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 4/4] platform/x86: dell-laptop: Use power_supply_charge_types_show/_parse() helpers

[Hans de Goede]

Hi,

On 17-Dec-24 7:49 PM, Thomas Weißschuh wrote:
> Hi Hans,
> 
> On 2024-12-17 16:18:47+0100, Hans de Goede wrote:
>> On 17-Dec-24 1:01 PM, Ilpo Järvinen wrote:
>>> On Wed, 11 Dec 2024, Hans de Goede wrote:
>>>
>>>> Make battery_modes a map between tokens and enum power_supply_charge_type
>>>> values instead of between tokens and strings and use the new
>>>> power_supply_charge_types_show/_parse() helpers for show()/store()
>>>> to ensure that things are handled in the same way as in other drivers.
>>>>
>>>> This also changes battery_supported_modes to be a bitmap of charge-types
>>>> (enum power_supply_charge_type values) rather then a bitmap of indices
>>>> into battery_modes[].
>>>>
>>>> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> [..]
> 
>> Note that merging this requires the earlier patches from this
>> series which have been merged into:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/log/?h=for-next
>>
>> so this either requires an immutable tag from Sebastian for you to merge,
>> or this should be merged through Sebastian's tree.
> 
> If this goes in via the psy tree, you could already make it a power
> supply extension. The necessary code is in psy/for-next.

Yes I noticed that the power-supply extension support was just merged,
that is great. Thank you for your work on that!

> Not necessary obviously.

Right I'm afraid I don't have time to work on converting this to
a power-supply extension atm, so lets go with this incremental
improvement for now.

Regards,

Hans

p.s.

IK do have hw to test this on, so if someone else were to do a conversion
to the new power-supply extension model I would be happy to test.