[PATCH v4 0/9] power: supply: extension API

[PATCH v4 0/9] power: supply: extension API

[Thomas Weißschuh]

Introduce a mechanism for drivers to extend the properties implemented
by a power supply.

Motivation
----------

Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.

While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:

* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
  formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
  normal power supply driver.
* ~Properties can not be properly overriden.~
  (Overriding is now explicitly forbidden)

The proposed extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".

The API is meant to be used via battery_hook_register(), the same way as
the current extensions.

When testing, please enable lockdep to make sure the locking is correct.

Contents
--------

* Patch 1 to 6 are preparation patches.
* Patch 7 implements the extension API itself.
* Patch 8 adds extension support to test_power.c
* Patch 9 converts the in-tree cros_charge-control driver to the
  extension API.

Open issues
-----------

* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
  it also be gated behind this or another config?
* Is an rw_semaphore acceptable?

[0] https://lore.kernel.org/lkml/20240528-cros_ec-charge-control-v2-0-81fb27e1cff4@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v4:
- Drop RFC state
- Integrate locking commit
- Reregister hwmon device
- Link to v3: https://lore.kernel.org/r/20240904-power-supply-extensions-v3-0-62efeb93f8ec@weissschuh.net

Changes in v3:
- Make naming more consistent
- Readd locking
- Allow multiple active extensions
- Allow passing a "void *ext_data" when registering
- Switch example driver from system76 to cros_charge-control
- Link to v2: https://lore.kernel.org/r/20240608-power-supply-extensions-v2-0-2dcd35b012ad@weissschuh.net

Changes in v2:
- Drop locking patch, let's figure out the API first
- Allow registration of multiple extensions
- Pass extension to extension callbacks as parameter
- Disallow property overlap between extensions and core psy
- Drop system76/pdx86 maintainers, as the system76 changes are only RFC
  state anyways
- Link to v1: https://lore.kernel.org/r/20240606-power-supply-extensions-v1-0-b45669290bdc@weissschuh.net

---
Thomas Weißschuh (9):
      power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
      power: supply: core: rename psy_has_property() to psy_desc_has_property()
      power: supply: core: introduce power_supply_has_property()
      power: supply: hwmon: prepare for power supply extensions
      power: supply: sysfs: prepare for power supply extensions
      power: supply: sysfs: rework uevent property loop
      power: supply: core: implement extension API
      power: supply: test-power: implement a power supply extension
      power: supply: cros_charge-control: use power_supply extensions

 drivers/power/supply/cros_charge-control.c | 205 +++++++++++------------------
 drivers/power/supply/power_supply.h        |  16 +++
 drivers/power/supply/power_supply_core.c   | 175 ++++++++++++++++++++++--
 drivers/power/supply/power_supply_hwmon.c  |  50 ++++---
 drivers/power/supply/power_supply_sysfs.c  |  85 ++++++------
 drivers/power/supply/test_power.c          | 102 ++++++++++++++
 include/linux/power_supply.h               |  32 +++++
 7 files changed, 456 insertions(+), 209 deletions(-)
---
base-commit: 83bce34420eaf91506957703bf9a31d8581ed6cb
change-id: 20240602-power-supply-extensions-07d949f509d9

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v4 1/9] power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR

[Thomas Weißschuh]

Currently an uevent contains the same string representation of a
property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this
is specially formatted to indicate all possible values.
This doesn't make sense for uevents and complicates parsing.
Instead only include the currently active value in uevents.

As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
this change is not a problem.
Soon the property will actually be used so fix the formatting before
that happens.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available(
 	return count;
 }
 
-static ssize_t power_supply_show_property(struct device *dev,
-					  struct device_attribute *attr,
-					  char *buf) {
+static ssize_t power_supply_format_property(struct device *dev,
+					    bool uevent,
+					    struct device_attribute *attr,
+					    char *buf)
+{
 	ssize_t ret;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const struct power_supply_attr *ps_attr = to_ps_attr(attr);
@@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 				psy->desc->usb_types, value.intval, buf);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		if (uevent) /* no possible values in uevents */
+			goto default_format;
 		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
 							 value.intval, buf);
 		break;
@@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 		ret = sysfs_emit(buf, "%s\n", value.strval);
 		break;
 	default:
+default_format:
 		if (ps_attr->text_values_len > 0 &&
 				value.intval < ps_attr->text_values_len && value.intval >= 0) {
 			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
@@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev,
 	return ret;
 }
 
+static ssize_t power_supply_show_property(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	return power_supply_format_property(dev, false, attr, buf);
+}
+
 static ssize_t power_supply_store_property(struct device *dev,
 					   struct device_attribute *attr,
 					   const char *buf, size_t count) {
@@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
 	pwr_attr = &power_supply_attrs[prop];
 	dev_attr = &pwr_attr->dev_attr;
 
-	ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf);
+	ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
 	if (ret == -ENODEV || ret == -ENODATA) {
 		/*
 		 * When a battery is absent, we expect -ENODEV. Don't abort;

-- 
2.47.0

[PATCH v4 2/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()

[Thomas Weißschuh]

The function only takes a desc as parameter, align the naming.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/power_supply_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 16085eff008442ecd04016cd12c2854a0230d9c6..2f61f6b90b99f40ee04a6d63ebc20036f0447102 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1180,8 +1180,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
 }
 EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
 
-static bool psy_has_property(const struct power_supply_desc *psy_desc,
-			     enum power_supply_property psp)
+static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
+				  enum power_supply_property psp)
 {
 	bool found = false;
 	int i;
@@ -1206,7 +1206,7 @@ int power_supply_get_property(struct power_supply *psy,
 		return -ENODEV;
 	}
 
-	if (psy_has_property(psy->desc, psp))
+	if (psy_desc_has_property(psy->desc, psp))
 		return psy->desc->get_property(psy, psp, val);
 	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
 		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1300,7 +1300,7 @@ static int psy_register_thermal(struct power_supply *psy)
 		return 0;
 
 	/* Register battery zone device psy reports temperature */
-	if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+	if (psy_desc_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
 		/* Prefer our hwmon device and avoid duplicates */
 		struct thermal_zone_params tzp = {
 			.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)

-- 
2.47.0

[PATCH v4 3/9] power: supply: core: introduce power_supply_has_property()

[Thomas Weißschuh]

Introduce a helper to check if a power supply implements a certain
property. It will be used by the sysfs and hwmon code to remove similar
open-coded checks.
It also paves the way for the extension API to hook into.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/power_supply.h      |  2 ++
 drivers/power/supply/power_supply_core.c | 12 ++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 7434a6f2477504ee6c0a3c7420e1444387b41180..5dabbd895538003096b62d03fdd0201b82b090e6 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -15,6 +15,8 @@ struct power_supply;
 
 extern int power_supply_property_is_writeable(struct power_supply *psy,
 					      enum power_supply_property psp);
+extern bool power_supply_has_property(struct power_supply *psy,
+				      enum power_supply_property psp);
 
 #ifdef CONFIG_SYSFS
 
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 2f61f6b90b99f40ee04a6d63ebc20036f0447102..502b07468b93dfb7f5a6c2092588d931a7d015f2 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1196,6 +1196,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
 	return found;
 }
 
+bool power_supply_has_property(struct power_supply *psy,
+			       enum power_supply_property psp)
+{
+	if (psy_desc_has_property(psy->desc, psp))
+		return true;
+
+	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
+		return true;
+
+	return false;
+}
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)

-- 
2.47.0

[PATCH v4 4/9] power: supply: hwmon: prepare for power supply extensions

[Thomas Weißschuh]

The upcoming extension API will add properties which are not part of the
the power_supply_desc.
Use power_supply_has_property() so the properties from extensions are
also checked.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/power_supply_hwmon.c | 50 +++++++++++++++----------------
 1 file changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 01be04903d7d2465ae2acb9eeb0b55a87868bb87..95245e6a6baa3e85ae8551e71f4f7905639a3325 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -349,9 +349,28 @@ static const struct hwmon_chip_info power_supply_hwmon_chip_info = {
 	.info = power_supply_hwmon_info,
 };
 
+static const enum power_supply_property power_supply_hwmon_props[] = {
+	POWER_SUPPLY_PROP_CURRENT_AVG,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_POWER_AVG,
+	POWER_SUPPLY_PROP_POWER_NOW,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_TEMP_MAX,
+	POWER_SUPPLY_PROP_TEMP_MIN,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
+	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
+	POWER_SUPPLY_PROP_TEMP_AMBIENT,
+	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
+	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_AVG,
+	POWER_SUPPLY_PROP_VOLTAGE_MIN,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
 int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 {
-	const struct power_supply_desc *desc = psy->desc;
 	struct power_supply_hwmon *psyhw;
 	struct device *dev = &psy->dev;
 	struct device *hwmon;
@@ -377,32 +396,11 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 		goto error;
 	}
 
-	for (i = 0; i < desc->num_properties; i++) {
-		const enum power_supply_property prop = desc->properties[i];
-
-		switch (prop) {
-		case POWER_SUPPLY_PROP_CURRENT_AVG:
-		case POWER_SUPPLY_PROP_CURRENT_MAX:
-		case POWER_SUPPLY_PROP_CURRENT_NOW:
-		case POWER_SUPPLY_PROP_POWER_AVG:
-		case POWER_SUPPLY_PROP_POWER_NOW:
-		case POWER_SUPPLY_PROP_TEMP:
-		case POWER_SUPPLY_PROP_TEMP_MAX:
-		case POWER_SUPPLY_PROP_TEMP_MIN:
-		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
-		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
-		case POWER_SUPPLY_PROP_TEMP_AMBIENT:
-		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
-		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
-		case POWER_SUPPLY_PROP_VOLTAGE_AVG:
-		case POWER_SUPPLY_PROP_VOLTAGE_MIN:
-		case POWER_SUPPLY_PROP_VOLTAGE_MAX:
-		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+	for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
+		const enum power_supply_property prop = power_supply_hwmon_props[i];
+
+		if (power_supply_has_property(psy, prop))
 			set_bit(prop, psyhw->props);
-			break;
-		default:
-			break;
-		}
 	}
 
 	name = psy->desc->name;

-- 
2.47.0

[PATCH v4 5/9] power: supply: sysfs: prepare for power supply extensions

[Thomas Weißschuh]

The upcoming extension API will add properties which are not part of the
the power_supply_desc.
Use power_supply_has_property() so the properties from extensions are
also checked.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f..bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -378,7 +378,6 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	struct device *dev = kobj_to_dev(kobj);
 	struct power_supply *psy = dev_get_drvdata(dev);
 	umode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
-	int i;
 
 	if (!power_supply_attrs[attrno].prop_name)
 		return 0;
@@ -386,19 +385,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	if (attrno == POWER_SUPPLY_PROP_TYPE)
 		return mode;
 
-	for (i = 0; i < psy->desc->num_properties; i++) {
-		int property = psy->desc->properties[i];
-
-		if (property == attrno) {
-			if (power_supply_property_is_writeable(psy, property) > 0)
-				mode |= S_IWUSR;
-
-			return mode;
-		}
-	}
-
-	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
+	if (power_supply_has_property(psy, attrno)) {
+		if (power_supply_property_is_writeable(psy, attrno) > 0)
+			mode |= S_IWUSR;
 		return mode;
+	}
 
 	return 0;
 }

-- 
2.47.0

[PATCH v4 6/9] power: supply: sysfs: rework uevent property loop

[Thomas Weißschuh]

Instead of looping through all properties known to be supported by the
psy, loop over all known properties and decide based on the return value
of power_supply_get_property() whether the property existed.

This makes the code shorter now and even more so when power supply
extensions are added.
It also simplifies the locking, as it can all happen inside
power_supply_get_property().

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

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167..99bfe1f03eb8326d38c4e2831c9670313b42e425 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -289,7 +289,7 @@ static ssize_t power_supply_format_property(struct device *dev,
 				dev_dbg_ratelimited(dev,
 					"driver has no data for `%s' property\n",
 					attr->attr.name);
-			else if (ret != -ENODEV && ret != -EAGAIN)
+			else if (ret != -ENODEV && ret != -EAGAIN && ret != -EINVAL)
 				dev_err_ratelimited(dev,
 					"driver failed to report `%s' property: %zd\n",
 					attr->attr.name, ret);
@@ -441,7 +441,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
 	dev_attr = &pwr_attr->dev_attr;
 
 	ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
-	if (ret == -ENODEV || ret == -ENODATA) {
+	if (ret == -ENODEV || ret == -ENODATA || ret == -EINVAL) {
 		/*
 		 * When a battery is absent, we expect -ENODEV. Don't abort;
 		 * send the uevent with at least the PRESENT=0 property
@@ -462,11 +462,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
 
 int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 {
-	const struct power_supply *psy = dev_get_drvdata(dev);
-	const enum power_supply_property *battery_props =
-		power_supply_battery_info_properties;
-	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
-					 sizeof(unsigned long) + 1] = {0};
+	struct power_supply *psy = dev_get_drvdata(dev);
 	int ret = 0, j;
 	char *prop_buf;
 
@@ -494,22 +490,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	if (ret)
 		goto out;
 
-	for (j = 0; j < psy->desc->num_properties; j++) {
-		set_bit(psy->desc->properties[j], psy_drv_properties);
-		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
-				      prop_buf);
-		if (ret)
-			goto out;
-	}
-
-	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
-		if (test_bit(battery_props[j], psy_drv_properties))
-			continue;
-		if (!power_supply_battery_info_has_prop(psy->battery_info,
-				battery_props[j]))
-			continue;
-		ret = add_prop_uevent(dev, env, battery_props[j],
-			      prop_buf);
+	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
+		ret = add_prop_uevent(dev, env, j, prop_buf);
 		if (ret)
 			goto out;
 	}

-- 
2.47.0

[PATCH v4 7/9] power: supply: core: implement extension API

[Thomas Weißschuh]

Various drivers, mostly in platform/x86 extend the ACPI battery driver
with additional sysfs attributes to implement more UAPIs than are
exposed through ACPI by using various side-channels, like WMI,
nonstandard ACPI or EC communication.

While the created sysfs attributes look similar to the attributes
provided by the powersupply core, there are various deficiencies:

* They don't show up in uevent payload.
* They can't be queried with the standard in-kernel APIs.
* They don't work with triggers.
* The extending driver has to reimplement all of the parsing,
formatting and sysfs display logic.
* Writing a extension driver is completely different from writing a
normal power supply driver.

This extension API avoids all of these issues.
An extension is just a "struct power_supply_ext" with the same kind of
callbacks as in a normal "struct power_supply_desc".

The API is meant to be used via battery_hook_register(), the same way as
the current extensions.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h       |  14 +++
 drivers/power/supply/power_supply_core.c  | 155 ++++++++++++++++++++++++++++--
 drivers/power/supply/power_supply_sysfs.c |  22 ++++-
 include/linux/power_supply.h              |  32 ++++++
 4 files changed, 213 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 5dabbd895538003096b62d03fdd0201b82b090e6..4c3e602c416cec556173a8eb1a3114c13ded71b7 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -9,6 +9,8 @@
  *  Modified: 2004, Oct     Szabolcs Gyurko
  */
 
+#include <linux/lockdep.h>
+
 struct device;
 struct device_type;
 struct power_supply;
@@ -17,6 +19,18 @@ extern int power_supply_property_is_writeable(struct power_supply *psy,
 					      enum power_supply_property psp);
 extern bool power_supply_has_property(struct power_supply *psy,
 				      enum power_supply_property psp);
+extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
+					  enum power_supply_property psp);
+
+struct power_supply_ext_registration {
+	struct list_head list_head;
+	const struct power_supply_ext *ext;
+	void *data;
+};
+
+#define power_supply_for_each_extension(pos, psy) \
+	lockdep_assert_held(&(psy)->extensions_sem); \
+	list_for_each_entry(pos, &(psy)->extensions, list_head)
 
 #ifdef CONFIG_SYSFS
 
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 502b07468b93dfb7f5a6c2092588d931a7d015f2..bf3054ed034e091adefcdbf98873a108b4c90fde 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -81,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
 
 static void power_supply_changed_work(struct work_struct *work)
 {
+	int ret;
 	unsigned long flags;
 	struct power_supply *psy = container_of(work, struct power_supply,
 						changed_work);
@@ -88,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
 	dev_dbg(&psy->dev, "%s\n", __func__);
 
 	spin_lock_irqsave(&psy->changed_lock, flags);
+
+	if (unlikely(psy->update_groups)) {
+		psy->update_groups = false;
+		spin_unlock_irqrestore(&psy->changed_lock, flags);
+		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+		if (ret)
+			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
+		spin_lock_irqsave(&psy->changed_lock, flags);
+	}
+
 	/*
 	 * Check 'changed' here to avoid issues due to race between
 	 * power_supply_changed() and this routine. In worst case
@@ -1196,15 +1207,37 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
 	return found;
 }
 
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+				   enum power_supply_property psp)
+{
+	bool found = false;
+	int i;
+
+	for (i = 0; i < psy_ext->num_properties; i++) {
+		if (psy_ext->properties[i] == psp) {
+			found = true;
+			break;
+		}
+	}
+
+	return found;
+}
+
 bool power_supply_has_property(struct power_supply *psy,
 			       enum power_supply_property psp)
 {
+	struct power_supply_ext_registration *reg;
+
 	if (psy_desc_has_property(psy->desc, psp))
 		return true;
 
 	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
 		return true;
 
+	power_supply_for_each_extension(reg, psy)
+		if (power_supply_ext_has_property(reg->ext, psp))
+			return true;
+
 	return false;
 }
 
@@ -1212,12 +1245,21 @@ int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
 {
+	struct power_supply_ext_registration *reg;
+
 	if (atomic_read(&psy->use_cnt) <= 0) {
 		if (!psy->initialized)
 			return -EAGAIN;
 		return -ENODEV;
 	}
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, psp))
+			return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
+	}
+
 	if (psy_desc_has_property(psy->desc, psp))
 		return psy->desc->get_property(psy, psp, val);
 	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
@@ -1231,7 +1273,23 @@ int power_supply_set_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    const union power_supply_propval *val)
 {
-	if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
+	struct power_supply_ext_registration *reg;
+
+	if (atomic_read(&psy->use_cnt) <= 0)
+		return -ENODEV;
+
+	guard(rwsem_read)(&psy->extensions_sem);
+
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, psp)) {
+			if (reg->ext->set_property)
+				return reg->ext->set_property(psy, reg->ext, reg->data, psp, val);
+			else
+				return -ENODEV;
+		}
+	}
+
+	if (!psy->desc->set_property)
 		return -ENODEV;
 
 	return psy->desc->set_property(psy, psp, val);
@@ -1241,7 +1299,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
 int power_supply_property_is_writeable(struct power_supply *psy,
 					enum power_supply_property psp)
 {
-	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
+	struct power_supply_ext_registration *reg;
+
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, psp)) {
+			if (reg->ext->property_is_writeable)
+				return reg->ext->property_is_writeable(psy, reg->ext,
+								       reg->data, psp);
+			else
+				return -ENODEV;
+		}
+	}
+
+	if (!psy->desc->property_is_writeable)
+		return -ENODEV;
+
+	return psy->desc->property_is_writeable(psy, psp);
 }
 
 void power_supply_external_power_changed(struct power_supply *psy)
@@ -1260,6 +1333,67 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(power_supply_powers);
 
+static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&psy->changed_lock, flags);
+	psy->update_groups = true;
+	spin_unlock_irqrestore(&psy->changed_lock, flags);
+
+	power_supply_changed(psy);
+
+	power_supply_remove_hwmon_sysfs(psy);
+	return power_supply_add_hwmon_sysfs(psy);
+}
+
+int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
+				    void *data)
+{
+	struct power_supply_ext_registration *reg;
+	size_t i;
+
+	guard(rwsem_write)(&psy->extensions_sem);
+
+	power_supply_for_each_extension(reg, psy)
+		if (reg->ext == ext)
+			return -EEXIST;
+
+	for (i = 0; i < ext->num_properties; i++)
+		if (power_supply_has_property(psy, ext->properties[i]))
+			return -EEXIST;
+
+	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
+	if (!reg)
+		return -ENOMEM;
+
+	reg->ext = ext;
+	reg->data = data;
+	list_add(&reg->list_head, &psy->extensions);
+
+	return power_supply_update_sysfs_and_hwmon(psy);
+}
+EXPORT_SYMBOL_GPL(power_supply_register_extension);
+
+void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+	struct power_supply_ext_registration *reg;
+
+	guard(rwsem_write)(&psy->extensions_sem);
+
+	power_supply_for_each_extension(reg, psy) {
+		if (reg->ext == ext) {
+			list_del(&reg->list_head);
+			kfree(reg);
+			power_supply_update_sysfs_and_hwmon(psy);
+			return;
+		}
+	}
+
+	dev_warn(&psy->dev, "Trying to unregister invalid extension");
+}
+EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
+
 static void power_supply_dev_release(struct device *dev)
 {
 	struct power_supply *psy = to_power_supply(dev);
@@ -1414,6 +1548,9 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	init_rwsem(&psy->extensions_sem);
+	INIT_LIST_HEAD(&psy->extensions);
+
 	rc = device_add(dev);
 	if (rc)
 		goto device_add_failed;
@@ -1426,13 +1563,15 @@ __power_supply_register(struct device *parent,
 	if (rc)
 		goto register_thermal_failed;
 
-	rc = power_supply_create_triggers(psy);
-	if (rc)
-		goto create_triggers_failed;
+	scoped_guard(rwsem_read, &psy->extensions_sem) {
+		rc = power_supply_create_triggers(psy);
+		if (rc)
+			goto create_triggers_failed;
 
-	rc = power_supply_add_hwmon_sysfs(psy);
-	if (rc)
-		goto add_hwmon_sysfs_failed;
+		rc = power_supply_add_hwmon_sysfs(psy);
+		if (rc)
+			goto add_hwmon_sysfs_failed;
+	}
 
 	/*
 	 * Update use_cnt after any uevents (most notably from device_add()).
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 99bfe1f03eb8326d38c4e2831c9670313b42e425..2cf25bacd7a1bb66e5a72629bffaa6d16bfbf3be 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -268,6 +268,23 @@ static ssize_t power_supply_show_enum_with_available(
 	return count;
 }
 
+static ssize_t power_supply_show_charge_behaviour(struct device *dev,
+						  struct power_supply *psy,
+						  union power_supply_propval *value,
+						  char *buf)
+{
+	struct power_supply_ext_registration *reg;
+
+	power_supply_for_each_extension(reg, psy) {
+		if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
+			return power_supply_charge_behaviour_show(dev, reg->ext->charge_behaviours,
+								  value->intval, buf);
+	}
+
+	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
+						  value->intval, buf);
+}
+
 static ssize_t power_supply_format_property(struct device *dev,
 					    bool uevent,
 					    struct device_attribute *attr,
@@ -307,8 +324,7 @@ static ssize_t power_supply_format_property(struct device *dev,
 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
 		if (uevent) /* no possible values in uevents */
 			goto default_format;
-		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
-							 value.intval, buf);
+		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sysfs_emit(buf, "%s\n", value.strval);
@@ -385,6 +401,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	if (attrno == POWER_SUPPLY_PROP_TYPE)
 		return mode;
 
+	guard(rwsem_read)(&psy->extensions_sem);
+
 	if (power_supply_has_property(psy, attrno)) {
 		if (power_supply_property_is_writeable(psy, attrno) > 0)
 			mode |= S_IWUSR;
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index b98106e1a90f34bce5129317a099f363248342b9..016e44cb3eb5eb7ace01a032661f65a5d81a522f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,8 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 #include <linux/leds.h>
+#include <linux/rwsem.h>
+#include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
 
@@ -281,6 +283,27 @@ struct power_supply_desc {
 	int use_for_apm;
 };
 
+struct power_supply_ext {
+	u8 charge_behaviours;
+	const enum power_supply_property *properties;
+	size_t num_properties;
+
+	int (*get_property)(struct power_supply *psy,
+			    const struct power_supply_ext *ext,
+			    void *data,
+			    enum power_supply_property psp,
+			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    const struct power_supply_ext *ext,
+			    void *data,
+			    enum power_supply_property psp,
+			    const union power_supply_propval *val);
+	int (*property_is_writeable)(struct power_supply *psy,
+				     const struct power_supply_ext *ext,
+				     void *data,
+				     enum power_supply_property psp);
+};
+
 struct power_supply {
 	const struct power_supply_desc *desc;
 
@@ -300,10 +323,13 @@ struct power_supply {
 	struct delayed_work deferred_register_work;
 	spinlock_t changed_lock;
 	bool changed;
+	bool update_groups;
 	bool initialized;
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	struct rw_semaphore extensions_sem; /* protects "extensions" */
+	struct list_head extensions;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
@@ -878,6 +904,12 @@ devm_power_supply_register(struct device *parent,
 extern void power_supply_unregister(struct power_supply *psy);
 extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 
+extern int power_supply_register_extension(struct power_supply *psy,
+					   const struct power_supply_ext *ext,
+					   void *data);
+extern void power_supply_unregister_extension(struct power_supply *psy,
+					      const struct power_supply_ext *ext);
+
 #define to_power_supply(device) container_of(device, struct power_supply, dev)
 
 extern void *power_supply_get_drvdata(struct power_supply *psy);

-- 
2.47.0

[PATCH v4 8/9] power: supply: test-power: implement a power supply extension

[Thomas Weißschuh]

Allow easy testing of the new power supply extension functionality.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/test_power.c | 102 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 442ceb7795e1d84e34da2801d228d53fb67e08d9..af05f3c5c292fd5702109df7dfa177d85c9d18c9 100644
--- a/drivers/power/supply/test_power.c
+++ b/drivers/power/supply/test_power.c
@@ -37,6 +37,7 @@ static int battery_charge_counter	= -1000;
 static int battery_current		= -1600;
 static enum power_supply_charge_behaviour battery_charge_behaviour =
 	POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+static bool battery_hook;
 
 static bool module_initialized;
 
@@ -238,6 +239,80 @@ static const struct power_supply_config test_power_configs[] = {
 	},
 };
 
+static int power_supply_ext_manufacture_year = 1234;
+static const enum power_supply_property power_supply_ext_props[] = {
+	POWER_SUPPLY_PROP_MANUFACTURE_YEAR,
+};
+
+static int power_supply_ext_get_property(struct power_supply *psy,
+					 const struct power_supply_ext *ext,
+					 void *ext_data,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+		val->intval = power_supply_ext_manufacture_year;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int power_supply_ext_set_property(struct power_supply *psy,
+					 const struct power_supply_ext *ext,
+					 void *ext_data,
+					 enum power_supply_property psp,
+					 const union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_MANUFACTURE_YEAR:
+		power_supply_ext_manufacture_year = val->intval;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int power_supply_ext_property_is_writeable(struct power_supply *psy,
+						  const struct power_supply_ext *ext,
+						  void *ext_data,
+						  enum power_supply_property psp)
+{
+	return true;
+}
+
+static const struct power_supply_ext power_supply_ext = {
+	.properties = power_supply_ext_props,
+	.num_properties = ARRAY_SIZE(power_supply_ext_props),
+	.get_property = power_supply_ext_get_property,
+	.set_property = power_supply_ext_set_property,
+	.property_is_writeable = power_supply_ext_property_is_writeable,
+};
+
+static void test_battery_configure_battery_hook(bool enable)
+{
+	struct power_supply *psy;
+
+	if (battery_hook == enable)
+		return;
+
+	psy = test_power_supplies[TEST_BATTERY];
+
+	if (enable) {
+		if (power_supply_register_extension(psy, &power_supply_ext, NULL)) {
+			pr_err("registering battery extension failed\n");
+			return;
+		}
+	} else {
+		power_supply_unregister_extension(psy, &power_supply_ext);
+	}
+
+	battery_hook = enable;
+}
+
 static int __init test_power_init(void)
 {
 	int i;
@@ -258,6 +333,8 @@ static int __init test_power_init(void)
 		}
 	}
 
+	test_battery_configure_battery_hook(true);
+
 	module_initialized = true;
 	return 0;
 failed:
@@ -524,6 +601,22 @@ static int param_set_battery_current(const char *key,
 
 #define param_get_battery_current param_get_int
 
+static int param_set_battery_hook(const char *key,
+				  const struct kernel_param *kp)
+{
+	int tmp;
+
+	if (1 != sscanf(key, "%d", &tmp))
+		return -EINVAL;
+	if (tmp != 1 && tmp != 0)
+		return -EINVAL;
+
+	test_battery_configure_battery_hook(tmp);
+	return 0;
+}
+
+#define param_get_battery_hook param_get_int
+
 static const struct kernel_param_ops param_ops_ac_online = {
 	.set = param_set_ac_online,
 	.get = param_get_ac_online,
@@ -574,6 +667,11 @@ static const struct kernel_param_ops param_ops_battery_current = {
 	.get = param_get_battery_current,
 };
 
+static const struct kernel_param_ops param_ops_battery_hook = {
+	.set = param_set_battery_hook,
+	.get = param_get_battery_hook,
+};
+
 #define param_check_ac_online(name, p) __param_check(name, p, void);
 #define param_check_usb_online(name, p) __param_check(name, p, void);
 #define param_check_battery_status(name, p) __param_check(name, p, void);
@@ -584,6 +682,7 @@ static const struct kernel_param_ops param_ops_battery_current = {
 #define param_check_battery_voltage(name, p) __param_check(name, p, void);
 #define param_check_battery_charge_counter(name, p) __param_check(name, p, void);
 #define param_check_battery_current(name, p) __param_check(name, p, void);
+#define param_check_battery_hook(name, p) __param_check(name, p, void);
 
 
 module_param(ac_online, ac_online, 0644);
@@ -621,6 +720,9 @@ MODULE_PARM_DESC(battery_charge_counter,
 module_param(battery_current, battery_current, 0644);
 MODULE_PARM_DESC(battery_current, "battery current (milliampere)");
 
+module_param(battery_hook, battery_hook, 0644);
+MODULE_PARM_DESC(battery_hook, "battery hook");
+
 MODULE_DESCRIPTION("Power supply driver for testing");
 MODULE_AUTHOR("Anton Vorontsov <cbouatmailru@gmail.com>");
 MODULE_LICENSE("GPL");

-- 
2.47.0

[PATCH v4 9/9] power: supply: cros_charge-control: use power_supply extensions

[Thomas Weißschuh]

Power supply extensions provide an easier mechanism to implement
additional properties for existing power supplies.
Use that instead of reimplementing the sysfs attributes manually.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/cros_charge-control.c | 205 +++++++++++------------------
 1 file changed, 75 insertions(+), 130 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index 17c53591ce197d08d97c94d3d4359a282026dd7d..f0933ac3f042c19e7c1dfdc9aff1ad03443ceb16 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -18,13 +18,6 @@
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE)   | \
 					 BIT(POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE))
 
-enum CROS_CHCTL_ATTR {
-	CROS_CHCTL_ATTR_START_THRESHOLD,
-	CROS_CHCTL_ATTR_END_THRESHOLD,
-	CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR,
-	_CROS_CHCTL_ATTR_COUNT
-};
-
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
  * slightly, also the v1 API can not return any data.
@@ -41,13 +34,7 @@ struct cros_chctl_priv {
 	struct power_supply *hooked_battery;
 	u8 cmd_version;
 
-	/* The callbacks need to access this priv structure.
-	 * As neither the struct device nor power_supply are under the drivers
-	 * control, embed the attributes within priv to use with container_of().
-	 */
-	struct device_attribute device_attrs[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute *attributes[_CROS_CHCTL_ATTR_COUNT];
-	struct attribute_group group;
+	struct power_supply_ext psy_ext;
 
 	enum power_supply_charge_behaviour current_behaviour;
 	u8 current_start_threshold, current_end_threshold;
@@ -114,123 +101,85 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
 }
 
-static struct cros_chctl_priv *cros_chctl_attr_to_priv(struct attribute *attr,
-						       enum CROS_CHCTL_ATTR idx)
-{
-	struct device_attribute *dev_attr = container_of(attr, struct device_attribute, attr);
-
-	return container_of(dev_attr, struct cros_chctl_priv, device_attrs[idx]);
-}
+static const enum power_supply_property cros_chctl_psy_ext_props[] = {
+	POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR, /* has to be first */
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
 
-static ssize_t cros_chctl_store_threshold(struct device *dev, struct cros_chctl_priv *priv,
-					  int is_end_threshold, const char *buf, size_t count)
+static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       union power_supply_propval *val)
 {
-	int ret, val;
+	struct cros_chctl_priv *priv = data;
 
-	ret = kstrtoint(buf, 10, &val);
-	if (ret < 0)
-		return ret;
-	if (val < 0 || val > 100)
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+		val->intval = priv->current_start_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		val->intval = priv->current_end_threshold;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		val->intval = priv->current_behaviour;
+		return 0;
+	default:
 		return -EINVAL;
-
-	if (is_end_threshold) {
-		if (val <= priv->current_start_threshold)
-			return -EINVAL;
-		priv->current_end_threshold = val;
-	} else {
-		if (val >= priv->current_end_threshold)
-			return -EINVAL;
-		priv->current_start_threshold = val;
 	}
-
-	if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
-		ret = cros_chctl_configure_ec(priv);
-		if (ret < 0)
-			return ret;
-	}
-
-	return count;
 }
 
-static ssize_t charge_control_start_threshold_show(struct device *dev,
-						   struct device_attribute *attr,
-						   char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
 
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_start_threshold);
-}
-
-static ssize_t charge_control_start_threshold_store(struct device *dev,
-						    struct device_attribute *attr,
-						    const char *buf, size_t count)
+static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
+				       const struct power_supply_ext *ext,
+				       void *data,
+				       enum power_supply_property psp,
+				       const union power_supply_propval *val)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_START_THRESHOLD);
-
-	return cros_chctl_store_threshold(dev, priv, 0, buf, count);
-}
-
-static ssize_t charge_control_end_threshold_show(struct device *dev, struct device_attribute *attr,
-						 char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
-
-	return sysfs_emit(buf, "%u\n", (unsigned int)priv->current_end_threshold);
-}
-
-static ssize_t charge_control_end_threshold_store(struct device *dev, struct device_attribute *attr,
-						  const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_END_THRESHOLD);
-
-	return cros_chctl_store_threshold(dev, priv, 1, buf, count);
-}
-
-static ssize_t charge_behaviour_show(struct device *dev, struct device_attribute *attr, char *buf)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
-
-	return power_supply_charge_behaviour_show(dev, EC_CHARGE_CONTROL_BEHAVIOURS,
-						  priv->current_behaviour, buf);
-}
-
-static ssize_t charge_behaviour_store(struct device *dev, struct device_attribute *attr,
-				      const char *buf, size_t count)
-{
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(&attr->attr,
-							       CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR);
+	struct cros_chctl_priv *priv = data;
 	int ret;
 
-	ret = power_supply_charge_behaviour_parse(EC_CHARGE_CONTROL_BEHAVIOURS, buf);
-	if (ret < 0)
-		return ret;
-
-	priv->current_behaviour = ret;
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		if (val->intval < 0 || val->intval > 100)
+			return -EINVAL;
 
-	ret = cros_chctl_configure_ec(priv);
-	if (ret < 0)
-		return ret;
+		if (psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD) {
+			if (val->intval <= priv->current_start_threshold)
+				return -EINVAL;
+			priv->current_end_threshold = val->intval;
+		} else {
+			if (val->intval >= priv->current_end_threshold)
+				return -EINVAL;
+			priv->current_start_threshold = val->intval;
+		}
+
+		if (priv->current_behaviour == POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO) {
+			ret = cros_chctl_configure_ec(priv);
+			if (ret < 0)
+				return ret;
+		}
 
-	return count;
+		return 0;
+	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
+		priv->current_behaviour = val->intval;
+		ret = cros_chctl_configure_ec(priv);
+		if (ret < 0)
+			return ret;
+		return 0;
+	default:
+		return -EINVAL;
+	}
 }
 
-static umode_t cros_chtl_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+static int cros_chctl_psy_prop_is_writeable(struct power_supply *psy,
+					    const struct power_supply_ext *ext,
+					    void *data,
+					    enum power_supply_property psp)
 {
-	struct cros_chctl_priv *priv = cros_chctl_attr_to_priv(attr, n);
-
-	if (priv->cmd_version < 2) {
-		if (n == CROS_CHCTL_ATTR_START_THRESHOLD)
-			return 0;
-		if (n == CROS_CHCTL_ATTR_END_THRESHOLD)
-			return 0;
-	}
-
-	return attr->mode;
+	return true;
 }
 
 static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -241,7 +190,7 @@ static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_batt
 		return 0;
 
 	priv->hooked_battery = battery;
-	return device_add_group(&battery->dev, &priv->group);
+	return power_supply_register_extension(battery, &priv->psy_ext, priv);
 }
 
 static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
@@ -249,7 +198,7 @@ static int cros_chctl_remove_battery(struct power_supply *battery, struct acpi_b
 	struct cros_chctl_priv *priv = container_of(hook, struct cros_chctl_priv, battery_hook);
 
 	if (priv->hooked_battery == battery) {
-		device_remove_group(&battery->dev, &priv->group);
+		power_supply_unregister_extension(battery, &priv->psy_ext);
 		priv->hooked_battery = NULL;
 	}
 
@@ -275,7 +224,6 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
 	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
 	struct cros_chctl_priv *priv;
-	size_t i;
 	int ret;
 
 	ret = cros_chctl_fwk_charge_control_versions(cros_ec);
@@ -305,23 +253,20 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	dev_dbg(dev, "Command version: %u\n", (unsigned int)priv->cmd_version);
 
 	priv->cros_ec = cros_ec;
-	priv->device_attrs[CROS_CHCTL_ATTR_START_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_start_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_END_THRESHOLD] =
-		(struct device_attribute)__ATTR_RW(charge_control_end_threshold);
-	priv->device_attrs[CROS_CHCTL_ATTR_CHARGE_BEHAVIOUR] =
-		(struct device_attribute)__ATTR_RW(charge_behaviour);
-	for (i = 0; i < _CROS_CHCTL_ATTR_COUNT; i++) {
-		sysfs_attr_init(&priv->device_attrs[i].attr);
-		priv->attributes[i] = &priv->device_attrs[i].attr;
-	}
-	priv->group.is_visible = cros_chtl_attr_is_visible;
-	priv->group.attrs = priv->attributes;
 
 	priv->battery_hook.name = dev_name(dev);
 	priv->battery_hook.add_battery = cros_chctl_add_battery;
 	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
 
+	priv->psy_ext.properties = cros_chctl_psy_ext_props;
+	priv->psy_ext.num_properties = ARRAY_SIZE(cros_chctl_psy_ext_props);
+	if (priv->cmd_version == 1)
+		priv->psy_ext.num_properties = 1;
+	priv->psy_ext.charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS;
+	priv->psy_ext.get_property = cros_chctl_psy_ext_get_prop;
+	priv->psy_ext.set_property = cros_chctl_psy_ext_set_prop;
+	priv->psy_ext.property_is_writeable = cros_chctl_psy_prop_is_writeable;
+
 	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
 	priv->current_start_threshold = 0;
 	priv->current_end_threshold = 100;

-- 
2.47.0

Re: [PATCH v4 1/9] power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> Currently an uevent contains the same string representation of a
> property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this
> is specially formatted to indicate all possible values.
> This doesn't make sense for uevents and complicates parsing.
> Instead only include the currently active value in uevents.
>
> As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
> this change is not a problem.
> Soon the property will actually be used so fix the formatting before
> that happens.

AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
so we have to make sure that no userspace application uses this uevent information.

Other than that:

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available(
>   	return count;
>   }
>
> -static ssize_t power_supply_show_property(struct device *dev,
> -					  struct device_attribute *attr,
> -					  char *buf) {
> +static ssize_t power_supply_format_property(struct device *dev,
> +					    bool uevent,
> +					    struct device_attribute *attr,
> +					    char *buf)
> +{
>   	ssize_t ret;
>   	struct power_supply *psy = dev_get_drvdata(dev);
>   	const struct power_supply_attr *ps_attr = to_ps_attr(attr);
> @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>   				psy->desc->usb_types, value.intval, buf);
>   		break;
>   	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> +		if (uevent) /* no possible values in uevents */
> +			goto default_format;
>   		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
>   							 value.intval, buf);
>   		break;
> @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>   		ret = sysfs_emit(buf, "%s\n", value.strval);
>   		break;
>   	default:
> +default_format:
>   		if (ps_attr->text_values_len > 0 &&
>   				value.intval < ps_attr->text_values_len && value.intval >= 0) {
>   			ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
> @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev,
>   	return ret;
>   }
>
> +static ssize_t power_supply_show_property(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	return power_supply_format_property(dev, false, attr, buf);
> +}
> +
>   static ssize_t power_supply_store_property(struct device *dev,
>   					   struct device_attribute *attr,
>   					   const char *buf, size_t count) {
> @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>   	pwr_attr = &power_supply_attrs[prop];
>   	dev_attr = &pwr_attr->dev_attr;
>
> -	ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf);
> +	ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
>   	if (ret == -ENODEV || ret == -ENODATA) {
>   		/*
>   		 * When a battery is absent, we expect -ENODEV. Don't abort;
>

Re: [PATCH v4 2/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> The function only takes a desc as parameter, align the naming.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/power/supply/power_supply_core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 16085eff008442ecd04016cd12c2854a0230d9c6..2f61f6b90b99f40ee04a6d63ebc20036f0447102 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1180,8 +1180,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
>   }
>   EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
>
> -static bool psy_has_property(const struct power_supply_desc *psy_desc,
> -			     enum power_supply_property psp)
> +static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
> +				  enum power_supply_property psp)
>   {
>   	bool found = false;
>   	int i;
> @@ -1206,7 +1206,7 @@ int power_supply_get_property(struct power_supply *psy,
>   		return -ENODEV;
>   	}
>
> -	if (psy_has_property(psy->desc, psp))
> +	if (psy_desc_has_property(psy->desc, psp))
>   		return psy->desc->get_property(psy, psp, val);
>   	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
>   		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
> @@ -1300,7 +1300,7 @@ static int psy_register_thermal(struct power_supply *psy)
>   		return 0;
>
>   	/* Register battery zone device psy reports temperature */
> -	if (psy_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
> +	if (psy_desc_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
>   		/* Prefer our hwmon device and avoid duplicates */
>   		struct thermal_zone_params tzp = {
>   			.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
>

Re: [PATCH v4 3/9] power: supply: core: introduce power_supply_has_property()

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> Introduce a helper to check if a power supply implements a certain
> property. It will be used by the sysfs and hwmon code to remove similar
> open-coded checks.
> It also paves the way for the extension API to hook into.

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/power/supply/power_supply.h      |  2 ++
>   drivers/power/supply/power_supply_core.c | 12 ++++++++++++
>   2 files changed, 14 insertions(+)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 7434a6f2477504ee6c0a3c7420e1444387b41180..5dabbd895538003096b62d03fdd0201b82b090e6 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -15,6 +15,8 @@ struct power_supply;
>
>   extern int power_supply_property_is_writeable(struct power_supply *psy,
>   					      enum power_supply_property psp);
> +extern bool power_supply_has_property(struct power_supply *psy,
> +				      enum power_supply_property psp);
>
>   #ifdef CONFIG_SYSFS
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 2f61f6b90b99f40ee04a6d63ebc20036f0447102..502b07468b93dfb7f5a6c2092588d931a7d015f2 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1196,6 +1196,18 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
>   	return found;
>   }
>
> +bool power_supply_has_property(struct power_supply *psy,
> +			       enum power_supply_property psp)
> +{
> +	if (psy_desc_has_property(psy->desc, psp))
> +		return true;
> +
> +	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> +		return true;
> +
> +	return false;
> +}
> +
>   int power_supply_get_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    union power_supply_propval *val)
>

Re: [PATCH v4 4/9] power: supply: hwmon: prepare for power supply extensions

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> The upcoming extension API will add properties which are not part of the
> the power_supply_desc.
> Use power_supply_has_property() so the properties from extensions are
> also chec

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> ked.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/power/supply/power_supply_hwmon.c | 50 +++++++++++++++----------------
>   1 file changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index 01be04903d7d2465ae2acb9eeb0b55a87868bb87..95245e6a6baa3e85ae8551e71f4f7905639a3325 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -349,9 +349,28 @@ static const struct hwmon_chip_info power_supply_hwmon_chip_info = {
>   	.info = power_supply_hwmon_info,
>   };
>
> +static const enum power_supply_property power_supply_hwmon_props[] = {
> +	POWER_SUPPLY_PROP_CURRENT_AVG,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_POWER_AVG,
> +	POWER_SUPPLY_PROP_POWER_NOW,
> +	POWER_SUPPLY_PROP_TEMP,
> +	POWER_SUPPLY_PROP_TEMP_MAX,
> +	POWER_SUPPLY_PROP_TEMP_MIN,
> +	POWER_SUPPLY_PROP_TEMP_ALERT_MIN,
> +	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> +	POWER_SUPPLY_PROP_TEMP_AMBIENT,
> +	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN,
> +	POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX,
> +	POWER_SUPPLY_PROP_VOLTAGE_AVG,
> +	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +};
> +
>   int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>   {
> -	const struct power_supply_desc *desc = psy->desc;
>   	struct power_supply_hwmon *psyhw;
>   	struct device *dev = &psy->dev;
>   	struct device *hwmon;
> @@ -377,32 +396,11 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>   		goto error;
>   	}
>
> -	for (i = 0; i < desc->num_properties; i++) {
> -		const enum power_supply_property prop = desc->properties[i];
> -
> -		switch (prop) {
> -		case POWER_SUPPLY_PROP_CURRENT_AVG:
> -		case POWER_SUPPLY_PROP_CURRENT_MAX:
> -		case POWER_SUPPLY_PROP_CURRENT_NOW:
> -		case POWER_SUPPLY_PROP_POWER_AVG:
> -		case POWER_SUPPLY_PROP_POWER_NOW:
> -		case POWER_SUPPLY_PROP_TEMP:
> -		case POWER_SUPPLY_PROP_TEMP_MAX:
> -		case POWER_SUPPLY_PROP_TEMP_MIN:
> -		case POWER_SUPPLY_PROP_TEMP_ALERT_MIN:
> -		case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> -		case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> -		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MIN:
> -		case POWER_SUPPLY_PROP_TEMP_AMBIENT_ALERT_MAX:
> -		case POWER_SUPPLY_PROP_VOLTAGE_AVG:
> -		case POWER_SUPPLY_PROP_VOLTAGE_MIN:
> -		case POWER_SUPPLY_PROP_VOLTAGE_MAX:
> -		case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +	for (i = 0; i < ARRAY_SIZE(power_supply_hwmon_props); i++) {
> +		const enum power_supply_property prop = power_supply_hwmon_props[i];
> +
> +		if (power_supply_has_property(psy, prop))
>   			set_bit(prop, psyhw->props);
> -			break;
> -		default:
> -			break;
> -		}
>   	}
>
>   	name = psy->desc->name;
>

Re: [PATCH v4 1/9] power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR

[Thomas Weißschuh]

Nov 24, 2024 18:47:39 Armin Wolf <W_Armin@gmx.de>:

> Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:
>
>> Currently an uevent contains the same string representation of a
>> property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this
>> is specially formatted to indicate all possible values.
>> This doesn't make sense for uevents and complicates parsing.
>> Instead only include the currently active value in uevents.
>>
>> As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
>> this change is not a problem.
>> Soon the property will actually be used so fix the formatting before
>> that happens.
>
> AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
> so we have to make sure that no userspace application uses this uevent information.

It only does so after this series.
Currently it uses the ad-hoc sysfs attributes which don't show up in uevent.
(I'm the maintainer)

>
> Other than that:
>
> Reviewed-by: Armin Wolf <W_Armin@gmx.de>

Thanks!

>
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>> ---
>>   drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++----
>>   1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>> index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644
>> --- a/drivers/power/supply/power_supply_sysfs.c
>> +++ b/drivers/power/supply/power_supply_sysfs.c
>> @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available(
>>     return count;
>>   }
>>
>> -static ssize_t power_supply_show_property(struct device *dev,
>> -                     struct device_attribute *attr,
>> -                     char *buf) {
>> +static ssize_t power_supply_format_property(struct device *dev,
>> +                       bool uevent,
>> +                       struct device_attribute *attr,
>> +                       char *buf)
>> +{
>>     ssize_t ret;
>>     struct power_supply *psy = dev_get_drvdata(dev);
>>     const struct power_supply_attr *ps_attr = to_ps_attr(attr);
>> @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>                 psy->desc->usb_types, value.intval, buf);
>>         break;
>>     case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>> +       if (uevent) /* no possible values in uevents */
>> +           goto default_format;
>>         ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
>>                              value.intval, buf);
>>         break;
>> @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>>         ret = sysfs_emit(buf, "%s\n", value.strval);
>>         break;
>>     default:
>> +default_format:
>>         if (ps_attr->text_values_len > 0 &&
>>                 value.intval < ps_attr->text_values_len && value.intval >= 0) {
>>             ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>> @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev,
>>     return ret;
>>   }
>>
>> +static ssize_t power_supply_show_property(struct device *dev,
>> +                     struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +   return power_supply_format_property(dev, false, attr, buf);
>> +}
>> +
>>   static ssize_t power_supply_store_property(struct device *dev,
>>                        struct device_attribute *attr,
>>                        const char *buf, size_t count) {
>> @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>>     pwr_attr = &power_supply_attrs[prop];
>>     dev_attr = &pwr_attr->dev_attr;
>>
>> -   ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf);
>> +   ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
>>     if (ret == -ENODEV || ret == -ENODATA) {
>>         /*
>>          * When a battery is absent, we expect -ENODEV. Don't abort;
>>

Re: [PATCH v4 5/9] power: supply: sysfs: prepare for power supply extensions

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> The upcoming extension API will add properties which are not part of the
> the power_supply_desc.
> Use power_supply_has_property() so the properties from extensions are
> also checked.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>   drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f..bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -378,7 +378,6 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   	struct device *dev = kobj_to_dev(kobj);
>   	struct power_supply *psy = dev_get_drvdata(dev);
>   	umode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> -	int i;
>
>   	if (!power_supply_attrs[attrno].prop_name)
>   		return 0;
> @@ -386,19 +385,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   	if (attrno == POWER_SUPPLY_PROP_TYPE)
>   		return mode;
>
> -	for (i = 0; i < psy->desc->num_properties; i++) {
> -		int property = psy->desc->properties[i];
> -
> -		if (property == attrno) {
> -			if (power_supply_property_is_writeable(psy, property) > 0)
> -				mode |= S_IWUSR;
> -
> -			return mode;
> -		}
> -	}
> -
> -	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> +	if (power_supply_has_property(psy, attrno)) {
> +		if (power_supply_property_is_writeable(psy, attrno) > 0)

What happens with properties supplied via battery_info? Are drivers expecting the power supply core
to call property_is_writable() for those too?

Thanks,
Armin Wolf

> +			mode |= S_IWUSR;
>   		return mode;
> +	}
>
>   	return 0;
>   }
>

Re: [PATCH v4 6/9] power: supply: sysfs: rework uevent property loop

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> Instead of looping through all properties known to be supported by the
> psy, loop over all known properties and decide based on the return value
> of power_supply_get_property() whether the property existed.
>
> This makes the code shorter now and even more so when power supply
> extensions are added.
> It also simplifies the locking, as it can all happen inside
> power_supply_get_property().

Reviewed-by: Armin Wolf <W_Armin@gmx.de>

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/power/supply/power_supply_sysfs.c | 28 +++++-----------------------
>   1 file changed, 5 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167..99bfe1f03eb8326d38c4e2831c9670313b42e425 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -289,7 +289,7 @@ static ssize_t power_supply_format_property(struct device *dev,
>   				dev_dbg_ratelimited(dev,
>   					"driver has no data for `%s' property\n",
>   					attr->attr.name);
> -			else if (ret != -ENODEV && ret != -EAGAIN)
> +			else if (ret != -ENODEV && ret != -EAGAIN && ret != -EINVAL)
>   				dev_err_ratelimited(dev,
>   					"driver failed to report `%s' property: %zd\n",
>   					attr->attr.name, ret);
> @@ -441,7 +441,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>   	dev_attr = &pwr_attr->dev_attr;
>
>   	ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
> -	if (ret == -ENODEV || ret == -ENODATA) {
> +	if (ret == -ENODEV || ret == -ENODATA || ret == -EINVAL) {
>   		/*
>   		 * When a battery is absent, we expect -ENODEV. Don't abort;
>   		 * send the uevent with at least the PRESENT=0 property
> @@ -462,11 +462,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>
>   int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   {
> -	const struct power_supply *psy = dev_get_drvdata(dev);
> -	const enum power_supply_property *battery_props =
> -		power_supply_battery_info_properties;
> -	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
> -					 sizeof(unsigned long) + 1] = {0};
> +	struct power_supply *psy = dev_get_drvdata(dev);
>   	int ret = 0, j;
>   	char *prop_buf;
>
> @@ -494,22 +490,8 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   	if (ret)
>   		goto out;
>
> -	for (j = 0; j < psy->desc->num_properties; j++) {
> -		set_bit(psy->desc->properties[j], psy_drv_properties);
> -		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> -				      prop_buf);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	for (j = 0; j < power_supply_battery_info_properties_size; j++) {
> -		if (test_bit(battery_props[j], psy_drv_properties))
> -			continue;
> -		if (!power_supply_battery_info_has_prop(psy->battery_info,
> -				battery_props[j]))
> -			continue;
> -		ret = add_prop_uevent(dev, env, battery_props[j],
> -			      prop_buf);
> +	for (j = 0; j < POWER_SUPPLY_ATTR_CNT; j++) {
> +		ret = add_prop_uevent(dev, env, j, prop_buf);
>   		if (ret)
>   			goto out;
>   	}
>

Re: [PATCH v4 1/9] power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR

[Armin Wolf]

Am 24.11.24 um 18:55 schrieb Thomas Weißschuh:

> Nov 24, 2024 18:47:39 Armin Wolf <W_Armin@gmx.de>:
>
>> Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:
>>
>>> Currently an uevent contains the same string representation of a
>>> property as sysfs. However for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR this
>>> is specially formatted to indicate all possible values.
>>> This doesn't make sense for uevents and complicates parsing.
>>> Instead only include the currently active value in uevents.
>>>
>>> As currently no in-tree driver uses POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
>>> this change is not a problem.
>>> Soon the property will actually be used so fix the formatting before
>>> that happens.
>> AFAIK the cros-charge-behaviour driver does use POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR,
>> so we have to make sure that no userspace application uses this uevent information.
> It only does so after this series.
> Currently it uses the ad-hoc sysfs attributes which don't show up in uevent.
> (I'm the maintainer)

I see, in this case you can ignore my comment.

Thank,
Armin Wolf

>> Other than that:
>>
>> Reviewed-by: Armin Wolf <W_Armin@gmx.de>
> Thanks!
>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>    drivers/power/supply/power_supply_sysfs.c | 20 ++++++++++++++++----
>>>    1 file changed, 16 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
>>> index 571de43fcca9827cf0fe3023e453defbf1eaec7d..a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f 100644
>>> --- a/drivers/power/supply/power_supply_sysfs.c
>>> +++ b/drivers/power/supply/power_supply_sysfs.c
>>> @@ -268,9 +268,11 @@ static ssize_t power_supply_show_enum_with_available(
>>>      return count;
>>>    }
>>>
>>> -static ssize_t power_supply_show_property(struct device *dev,
>>> -                     struct device_attribute *attr,
>>> -                     char *buf) {
>>> +static ssize_t power_supply_format_property(struct device *dev,
>>> +                       bool uevent,
>>> +                       struct device_attribute *attr,
>>> +                       char *buf)
>>> +{
>>>      ssize_t ret;
>>>      struct power_supply *psy = dev_get_drvdata(dev);
>>>      const struct power_supply_attr *ps_attr = to_ps_attr(attr);
>>> @@ -303,6 +305,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>                  psy->desc->usb_types, value.intval, buf);
>>>          break;
>>>      case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>>> +       if (uevent) /* no possible values in uevents */
>>> +           goto default_format;
>>>          ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
>>>                               value.intval, buf);
>>>          break;
>>> @@ -310,6 +314,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>          ret = sysfs_emit(buf, "%s\n", value.strval);
>>>          break;
>>>      default:
>>> +default_format:
>>>          if (ps_attr->text_values_len > 0 &&
>>>                  value.intval < ps_attr->text_values_len && value.intval >= 0) {
>>>              ret = sysfs_emit(buf, "%s\n", ps_attr->text_values[value.intval]);
>>> @@ -321,6 +326,13 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>      return ret;
>>>    }
>>>
>>> +static ssize_t power_supply_show_property(struct device *dev,
>>> +                     struct device_attribute *attr,
>>> +                     char *buf)
>>> +{
>>> +   return power_supply_format_property(dev, false, attr, buf);
>>> +}
>>> +
>>>    static ssize_t power_supply_store_property(struct device *dev,
>>>                         struct device_attribute *attr,
>>>                         const char *buf, size_t count) {
>>> @@ -437,7 +449,7 @@ static int add_prop_uevent(const struct device *dev, struct kobj_uevent_env *env
>>>      pwr_attr = &power_supply_attrs[prop];
>>>      dev_attr = &pwr_attr->dev_attr;
>>>
>>> -   ret = power_supply_show_property((struct device *)dev, dev_attr, prop_buf);
>>> +   ret = power_supply_format_property((struct device *)dev, true, dev_attr, prop_buf);
>>>      if (ret == -ENODEV || ret == -ENODATA) {
>>>          /*
>>>           * When a battery is absent, we expect -ENODEV. Don't abort;
>>>

Re: [PATCH v4 7/9] power: supply: core: implement extension API

[Armin Wolf]

Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:

> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
>
> While the created sysfs attributes look similar to the attributes
> provided by the powersupply core, there are various deficiencies:
>
> * They don't show up in uevent payload.
> * They can't be queried with the standard in-kernel APIs.
> * They don't work with triggers.
> * The extending driver has to reimplement all of the parsing,
> formatting and sysfs display logic.
> * Writing a extension driver is completely different from writing a
> normal power supply driver.
>
> This extension API avoids all of these issues.
> An extension is just a "struct power_supply_ext" with the same kind of
> callbacks as in a normal "struct power_supply_desc".
>
> The API is meant to be used via battery_hook_register(), the same way as
> the current extensions.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/power/supply/power_supply.h       |  14 +++
>   drivers/power/supply/power_supply_core.c  | 155 ++++++++++++++++++++++++++++--
>   drivers/power/supply/power_supply_sysfs.c |  22 ++++-
>   include/linux/power_supply.h              |  32 ++++++
>   4 files changed, 213 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 5dabbd895538003096b62d03fdd0201b82b090e6..4c3e602c416cec556173a8eb1a3114c13ded71b7 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -9,6 +9,8 @@
>    *  Modified: 2004, Oct     Szabolcs Gyurko
>    */
>
> +#include <linux/lockdep.h>
> +
>   struct device;
>   struct device_type;
>   struct power_supply;
> @@ -17,6 +19,18 @@ extern int power_supply_property_is_writeable(struct power_supply *psy,
>   					      enum power_supply_property psp);
>   extern bool power_supply_has_property(struct power_supply *psy,
>   				      enum power_supply_property psp);
> +extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
> +					  enum power_supply_property psp);
> +
> +struct power_supply_ext_registration {
> +	struct list_head list_head;
> +	const struct power_supply_ext *ext;
> +	void *data;
> +};
> +
> +#define power_supply_for_each_extension(pos, psy) \
> +	lockdep_assert_held(&(psy)->extensions_sem); \
> +	list_for_each_entry(pos, &(psy)->extensions, list_head)
>
>   #ifdef CONFIG_SYSFS
>
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 502b07468b93dfb7f5a6c2092588d931a7d015f2..bf3054ed034e091adefcdbf98873a108b4c90fde 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -81,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
>
>   static void power_supply_changed_work(struct work_struct *work)
>   {
> +	int ret;
>   	unsigned long flags;
>   	struct power_supply *psy = container_of(work, struct power_supply,
>   						changed_work);
> @@ -88,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
>   	dev_dbg(&psy->dev, "%s\n", __func__);
>
>   	spin_lock_irqsave(&psy->changed_lock, flags);
> +
> +	if (unlikely(psy->update_groups)) {
> +		psy->update_groups = false;
> +		spin_unlock_irqrestore(&psy->changed_lock, flags);
> +		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
> +		if (ret)
> +			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
> +		spin_lock_irqsave(&psy->changed_lock, flags);
> +	}
> +
>   	/*
>   	 * Check 'changed' here to avoid issues due to race between
>   	 * power_supply_changed() and this routine. In worst case
> @@ -1196,15 +1207,37 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
>   	return found;
>   }
>
> +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> +				   enum power_supply_property psp)
> +{
> +	bool found = false;
> +	int i;
> +
> +	for (i = 0; i < psy_ext->num_properties; i++) {
> +		if (psy_ext->properties[i] == psp) {
> +			found = true;
> +			break;
> +		}
> +	}
> +
> +	return found;

Can we just return false here and directly return true when the property is found?

> +}
> +
>   bool power_supply_has_property(struct power_supply *psy,
>   			       enum power_supply_property psp)
>   {
> +	struct power_supply_ext_registration *reg;
> +
>   	if (psy_desc_has_property(psy->desc, psp))
>   		return true;
>
>   	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
>   		return true;
>
> +	power_supply_for_each_extension(reg, psy)
> +		if (power_supply_ext_has_property(reg->ext, psp))
> +			return true;
> +
>   	return false;
>   }
>
> @@ -1212,12 +1245,21 @@ int power_supply_get_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    union power_supply_propval *val)
>   {
> +	struct power_supply_ext_registration *reg;
> +
>   	if (atomic_read(&psy->use_cnt) <= 0) {
>   		if (!psy->initialized)
>   			return -EAGAIN;
>   		return -ENODEV;
>   	}
>
> +	guard(rwsem_read)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, psp))
> +			return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> +	}

Maybe we can use scoped_guard() here?

> +
>   	if (psy_desc_has_property(psy->desc, psp))
>   		return psy->desc->get_property(psy, psp, val);
>   	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> @@ -1231,7 +1273,23 @@ int power_supply_set_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    const union power_supply_propval *val)
>   {
> -	if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
> +	struct power_supply_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	guard(rwsem_read)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->set_property)
> +				return reg->ext->set_property(psy, reg->ext, reg->data, psp, val);
> +			else
> +				return -ENODEV;
> +		}
> +	}

Same as above.

> +
> +	if (!psy->desc->set_property)
>   		return -ENODEV;
>
>   	return psy->desc->set_property(psy, psp, val);
> @@ -1241,7 +1299,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
>   int power_supply_property_is_writeable(struct power_supply *psy,
>   					enum power_supply_property psp)
>   {
> -	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
> +	struct power_supply_ext_registration *reg;
> +
> +	power_supply_for_each_extension(reg, psy) {

Missing guard here.

> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->property_is_writeable)
> +				return reg->ext->property_is_writeable(psy, reg->ext,
> +								       reg->data, psp);
> +			else
> +				return -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
> +		return -ENODEV;
> +
> +	return psy->desc->property_is_writeable(psy, psp);
>   }
>
>   void power_supply_external_power_changed(struct power_supply *psy)
> @@ -1260,6 +1333,67 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(power_supply_powers);
>
> +static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&psy->changed_lock, flags);
> +	psy->update_groups = true;
> +	spin_unlock_irqrestore(&psy->changed_lock, flags);
> +
> +	power_supply_changed(psy);
> +
> +	power_supply_remove_hwmon_sysfs(psy);
> +	return power_supply_add_hwmon_sysfs(psy);

Do we need some locking here or is this ok?

> +}
> +
> +int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> +				    void *data)
> +{
> +	struct power_supply_ext_registration *reg;
> +	size_t i;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy)
> +		if (reg->ext == ext)
> +			return -EEXIST;
> +
> +	for (i = 0; i < ext->num_properties; i++)
> +		if (power_supply_has_property(psy, ext->properties[i]))
> +			return -EEXIST;
> +
> +	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	reg->ext = ext;
> +	reg->data = data;
> +	list_add(&reg->list_head, &psy->extensions);
> +
> +	return power_supply_update_sysfs_and_hwmon(psy);

We need to clean up *reg here should power_supply_update_sysfs_and_hwmon() fail.

> +}
> +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> +
> +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> +	struct power_supply_ext_registration *reg;
> +
> +	guard(rwsem_write)(&psy->extensions_sem);
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			kfree(reg);
> +			power_supply_update_sysfs_and_hwmon(psy);
> +			return;
> +		}
> +	}
> +
> +	dev_warn(&psy->dev, "Trying to unregister invalid extension");
> +}
> +EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
> +
>   static void power_supply_dev_release(struct device *dev)
>   {
>   	struct power_supply *psy = to_power_supply(dev);
> @@ -1414,6 +1548,9 @@ __power_supply_register(struct device *parent,
>   	}
>
>   	spin_lock_init(&psy->changed_lock);
> +	init_rwsem(&psy->extensions_sem);
> +	INIT_LIST_HEAD(&psy->extensions);
> +
>   	rc = device_add(dev);
>   	if (rc)
>   		goto device_add_failed;
> @@ -1426,13 +1563,15 @@ __power_supply_register(struct device *parent,
>   	if (rc)
>   		goto register_thermal_failed;
>
> -	rc = power_supply_create_triggers(psy);
> -	if (rc)
> -		goto create_triggers_failed;
> +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> +		rc = power_supply_create_triggers(psy);
> +		if (rc)
> +			goto create_triggers_failed;
>
> -	rc = power_supply_add_hwmon_sysfs(psy);
> -	if (rc)
> -		goto add_hwmon_sysfs_failed;
> +		rc = power_supply_add_hwmon_sysfs(psy);
> +		if (rc)
> +			goto add_hwmon_sysfs_failed;
> +	}
>
>   	/*
>   	 * Update use_cnt after any uevents (most notably from device_add()).
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 99bfe1f03eb8326d38c4e2831c9670313b42e425..2cf25bacd7a1bb66e5a72629bffaa6d16bfbf3be 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -268,6 +268,23 @@ static ssize_t power_supply_show_enum_with_available(
>   	return count;
>   }
>
> +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> +						  struct power_supply *psy,
> +						  union power_supply_propval *value,
> +						  char *buf)
> +{
> +	struct power_supply_ext_registration *reg;
> +

Missing guard here.

Thanks,
Armin Wolf

> +	power_supply_for_each_extension(reg, psy) {
> +		if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
> +			return power_supply_charge_behaviour_show(dev, reg->ext->charge_behaviours,
> +								  value->intval, buf);
> +	}
> +
> +	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> +						  value->intval, buf);
> +}
> +
>   static ssize_t power_supply_format_property(struct device *dev,
>   					    bool uevent,
>   					    struct device_attribute *attr,
> @@ -307,8 +324,7 @@ static ssize_t power_supply_format_property(struct device *dev,
>   	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
>   		if (uevent) /* no possible values in uevents */
>   			goto default_format;
> -		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> -							 value.intval, buf);
> +		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
>   		break;
>   	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>   		ret = sysfs_emit(buf, "%s\n", value.strval);
> @@ -385,6 +401,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   	if (attrno == POWER_SUPPLY_PROP_TYPE)
>   		return mode;
>
> +	guard(rwsem_read)(&psy->extensions_sem);
> +
>   	if (power_supply_has_property(psy, attrno)) {
>   		if (power_supply_property_is_writeable(psy, attrno) > 0)
>   			mode |= S_IWUSR;
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index b98106e1a90f34bce5129317a099f363248342b9..016e44cb3eb5eb7ace01a032661f65a5d81a522f 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -15,6 +15,8 @@
>   #include <linux/device.h>
>   #include <linux/workqueue.h>
>   #include <linux/leds.h>
> +#include <linux/rwsem.h>
> +#include <linux/list.h>
>   #include <linux/spinlock.h>
>   #include <linux/notifier.h>
>
> @@ -281,6 +283,27 @@ struct power_supply_desc {
>   	int use_for_apm;
>   };
>
> +struct power_supply_ext {
> +	u8 charge_behaviours;
> +	const enum power_supply_property *properties;
> +	size_t num_properties;
> +
> +	int (*get_property)(struct power_supply *psy,
> +			    const struct power_supply_ext *ext,
> +			    void *data,
> +			    enum power_supply_property psp,
> +			    union power_supply_propval *val);
> +	int (*set_property)(struct power_supply *psy,
> +			    const struct power_supply_ext *ext,
> +			    void *data,
> +			    enum power_supply_property psp,
> +			    const union power_supply_propval *val);
> +	int (*property_is_writeable)(struct power_supply *psy,
> +				     const struct power_supply_ext *ext,
> +				     void *data,
> +				     enum power_supply_property psp);
> +};
> +
>   struct power_supply {
>   	const struct power_supply_desc *desc;
>
> @@ -300,10 +323,13 @@ struct power_supply {
>   	struct delayed_work deferred_register_work;
>   	spinlock_t changed_lock;
>   	bool changed;
> +	bool update_groups;
>   	bool initialized;
>   	bool removing;
>   	atomic_t use_cnt;
>   	struct power_supply_battery_info *battery_info;
> +	struct rw_semaphore extensions_sem; /* protects "extensions" */
> +	struct list_head extensions;
>   #ifdef CONFIG_THERMAL
>   	struct thermal_zone_device *tzd;
>   	struct thermal_cooling_device *tcd;
> @@ -878,6 +904,12 @@ devm_power_supply_register(struct device *parent,
>   extern void power_supply_unregister(struct power_supply *psy);
>   extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +extern int power_supply_register_extension(struct power_supply *psy,
> +					   const struct power_supply_ext *ext,
> +					   void *data);
> +extern void power_supply_unregister_extension(struct power_supply *psy,
> +					      const struct power_supply_ext *ext);
> +
>   #define to_power_supply(device) container_of(device, struct power_supply, dev)
>
>   extern void *power_supply_get_drvdata(struct power_supply *psy);
>

Re: [PATCH v4 5/9] power: supply: sysfs: prepare for power supply extensions

[Thomas Weißschuh]

On 2024-11-24 18:57:23+0100, Armin Wolf wrote:
> Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:
> 
> > The upcoming extension API will add properties which are not part of the
> > the power_supply_desc.
> > Use power_supply_has_property() so the properties from extensions are
> > also checked.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >   drivers/power/supply/power_supply_sysfs.c | 17 ++++-------------
> >   1 file changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index a7351b9c8fe34a464a4e69b1a1a4a4179c1a4b4f..bfe48fe01a8d03828c2e539e1e6e5e9fc5c60167 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -378,7 +378,6 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> >   	struct device *dev = kobj_to_dev(kobj);
> >   	struct power_supply *psy = dev_get_drvdata(dev);
> >   	umode_t mode = S_IRUSR | S_IRGRP | S_IROTH;
> > -	int i;
> > 
> >   	if (!power_supply_attrs[attrno].prop_name)
> >   		return 0;
> > @@ -386,19 +385,11 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> >   	if (attrno == POWER_SUPPLY_PROP_TYPE)
> >   		return mode;
> > 
> > -	for (i = 0; i < psy->desc->num_properties; i++) {
> > -		int property = psy->desc->properties[i];
> > -
> > -		if (property == attrno) {
> > -			if (power_supply_property_is_writeable(psy, property) > 0)
> > -				mode |= S_IWUSR;
> > -
> > -			return mode;
> > -		}
> > -	}
> > -
> > -	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> > +	if (power_supply_has_property(psy, attrno)) {
> > +		if (power_supply_property_is_writeable(psy, attrno) > 0)
> 
> What happens with properties supplied via battery_info? Are drivers expecting the power supply core
> to call property_is_writable() for those too?

I don't think this should be an issue.
But we could also modify power_supply_property_is_writeable() and handle
battery info properties there, they are never writable.

Maybe we could even replace the whole battery_info logic itself with a
powersupply extension.

> Thanks,
> Armin Wolf
> 
> > +			mode |= S_IWUSR;
> >   		return mode;
> > +	}
> > 
> >   	return 0;
> >   }
> > 

Re: [PATCH v4 7/9] power: supply: core: implement extension API

[Thomas Weißschuh]

On 2024-11-24 19:15:45+0100, Armin Wolf wrote:
> Am 11.11.24 um 22:40 schrieb Thomas Weißschuh:
> 
> > Various drivers, mostly in platform/x86 extend the ACPI battery driver
> > with additional sysfs attributes to implement more UAPIs than are
> > exposed through ACPI by using various side-channels, like WMI,
> > nonstandard ACPI or EC communication.
> > 
> > While the created sysfs attributes look similar to the attributes
> > provided by the powersupply core, there are various deficiencies:
> > 
> > * They don't show up in uevent payload.
> > * They can't be queried with the standard in-kernel APIs.
> > * They don't work with triggers.
> > * The extending driver has to reimplement all of the parsing,
> > formatting and sysfs display logic.
> > * Writing a extension driver is completely different from writing a
> > normal power supply driver.
> > 
> > This extension API avoids all of these issues.
> > An extension is just a "struct power_supply_ext" with the same kind of
> > callbacks as in a normal "struct power_supply_desc".
> > 
> > The API is meant to be used via battery_hook_register(), the same way as
> > the current extensions.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   drivers/power/supply/power_supply.h       |  14 +++
> >   drivers/power/supply/power_supply_core.c  | 155 ++++++++++++++++++++++++++++--
> >   drivers/power/supply/power_supply_sysfs.c |  22 ++++-
> >   include/linux/power_supply.h              |  32 ++++++
> >   4 files changed, 213 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 5dabbd895538003096b62d03fdd0201b82b090e6..4c3e602c416cec556173a8eb1a3114c13ded71b7 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -9,6 +9,8 @@
> >    *  Modified: 2004, Oct     Szabolcs Gyurko
> >    */
> > 
> > +#include <linux/lockdep.h>
> > +
> >   struct device;
> >   struct device_type;
> >   struct power_supply;
> > @@ -17,6 +19,18 @@ extern int power_supply_property_is_writeable(struct power_supply *psy,
> >   					      enum power_supply_property psp);
> >   extern bool power_supply_has_property(struct power_supply *psy,
> >   				      enum power_supply_property psp);
> > +extern bool power_supply_ext_has_property(const struct power_supply_ext *ext,
> > +					  enum power_supply_property psp);
> > +
> > +struct power_supply_ext_registration {
> > +	struct list_head list_head;
> > +	const struct power_supply_ext *ext;
> > +	void *data;
> > +};
> > +
> > +#define power_supply_for_each_extension(pos, psy) \
> > +	lockdep_assert_held(&(psy)->extensions_sem); \
> > +	list_for_each_entry(pos, &(psy)->extensions, list_head)
> > 
> >   #ifdef CONFIG_SYSFS
> > 
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index 502b07468b93dfb7f5a6c2092588d931a7d015f2..bf3054ed034e091adefcdbf98873a108b4c90fde 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -81,6 +81,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
> > 
> >   static void power_supply_changed_work(struct work_struct *work)
> >   {
> > +	int ret;
> >   	unsigned long flags;
> >   	struct power_supply *psy = container_of(work, struct power_supply,
> >   						changed_work);
> > @@ -88,6 +89,16 @@ static void power_supply_changed_work(struct work_struct *work)
> >   	dev_dbg(&psy->dev, "%s\n", __func__);
> > 
> >   	spin_lock_irqsave(&psy->changed_lock, flags);
> > +
> > +	if (unlikely(psy->update_groups)) {
> > +		psy->update_groups = false;
> > +		spin_unlock_irqrestore(&psy->changed_lock, flags);
> > +		ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
> > +		if (ret)
> > +			dev_warn(&psy->dev, "failed to update sysfs groups: %pe\n", ERR_PTR(ret));
> > +		spin_lock_irqsave(&psy->changed_lock, flags);
> > +	}
> > +
> >   	/*
> >   	 * Check 'changed' here to avoid issues due to race between
> >   	 * power_supply_changed() and this routine. In worst case
> > @@ -1196,15 +1207,37 @@ static bool psy_desc_has_property(const struct power_supply_desc *psy_desc,
> >   	return found;
> >   }
> > 
> > +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> > +				   enum power_supply_property psp)
> > +{
> > +	bool found = false;
> > +	int i;
> > +
> > +	for (i = 0; i < psy_ext->num_properties; i++) {
> > +		if (psy_ext->properties[i] == psp) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return found;
> 
> Can we just return false here and directly return true when the property is found?

Yes, this was just the existing logic from psy_has_propery().
But I don't care either way.

> > +}
> > +
> >   bool power_supply_has_property(struct power_supply *psy,
> >   			       enum power_supply_property psp)
> >   {
> > +	struct power_supply_ext_registration *reg;
> > +
> >   	if (psy_desc_has_property(psy->desc, psp))
> >   		return true;
> > 
> >   	if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> >   		return true;
> > 
> > +	power_supply_for_each_extension(reg, psy)
> > +		if (power_supply_ext_has_property(reg->ext, psp))
> > +			return true;
> > +
> >   	return false;
> >   }
> > 
> > @@ -1212,12 +1245,21 @@ int power_supply_get_property(struct power_supply *psy,
> >   			    enum power_supply_property psp,
> >   			    union power_supply_propval *val)
> >   {
> > +	struct power_supply_ext_registration *reg;
> > +
> >   	if (atomic_read(&psy->use_cnt) <= 0) {
> >   		if (!psy->initialized)
> >   			return -EAGAIN;
> >   		return -ENODEV;
> >   	}
> > 
> > +	guard(rwsem_read)(&psy->extensions_sem);
> > +
> > +	power_supply_for_each_extension(reg, psy) {
> > +		if (power_supply_ext_has_property(reg->ext, psp))
> > +			return reg->ext->get_property(psy, reg->ext, reg->data, psp, val);
> > +	}
> 
> Maybe we can use scoped_guard() here?

Ack. 

> 
> > +
> >   	if (psy_desc_has_property(psy->desc, psp))
> >   		return psy->desc->get_property(psy, psp, val);
> >   	else if (power_supply_battery_info_has_prop(psy->battery_info, psp))
> > @@ -1231,7 +1273,23 @@ int power_supply_set_property(struct power_supply *psy,
> >   			    enum power_supply_property psp,
> >   			    const union power_supply_propval *val)
> >   {
> > -	if (atomic_read(&psy->use_cnt) <= 0 || !psy->desc->set_property)
> > +	struct power_supply_ext_registration *reg;
> > +
> > +	if (atomic_read(&psy->use_cnt) <= 0)
> > +		return -ENODEV;
> > +
> > +	guard(rwsem_read)(&psy->extensions_sem);
> > +
> > +	power_supply_for_each_extension(reg, psy) {
> > +		if (power_supply_ext_has_property(reg->ext, psp)) {
> > +			if (reg->ext->set_property)
> > +				return reg->ext->set_property(psy, reg->ext, reg->data, psp, val);
> > +			else
> > +				return -ENODEV;
> > +		}
> > +	}
> 
> Same as above.

Ack.
> > +
> > +	if (!psy->desc->set_property)
> >   		return -ENODEV;
> > 
> >   	return psy->desc->set_property(psy, psp, val);
> > @@ -1241,7 +1299,22 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
> >   int power_supply_property_is_writeable(struct power_supply *psy,
> >   					enum power_supply_property psp)
> >   {
> > -	return psy->desc->property_is_writeable && psy->desc->property_is_writeable(psy, psp);
> > +	struct power_supply_ext_registration *reg;
> > +
> > +	power_supply_for_each_extension(reg, psy) {
> 
> Missing guard here.

The sysfs caller already has the lock, so it can't be taken inside.
But the hwmon caller doesn't, so there needs to be locking there.

I'll add some explicit lockdep assertions to make it easier to see.

> > +		if (power_supply_ext_has_property(reg->ext, psp)) {
> > +			if (reg->ext->property_is_writeable)
> > +				return reg->ext->property_is_writeable(psy, reg->ext,
> > +								       reg->data, psp);
> > +			else
> > +				return -ENODEV;
> > +		}
> > +	}
> > +
> > +	if (!psy->desc->property_is_writeable)
> > +		return -ENODEV;
> > +
> > +	return psy->desc->property_is_writeable(psy, psp);
> >   }
> > 
> >   void power_supply_external_power_changed(struct power_supply *psy)
> > @@ -1260,6 +1333,67 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
> >   }
> >   EXPORT_SYMBOL_GPL(power_supply_powers);
> > 
> > +static int power_supply_update_sysfs_and_hwmon(struct power_supply *psy)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&psy->changed_lock, flags);
> > +	psy->update_groups = true;
> > +	spin_unlock_irqrestore(&psy->changed_lock, flags);
> > +
> > +	power_supply_changed(psy);
> > +
> > +	power_supply_remove_hwmon_sysfs(psy);
> > +	return power_supply_add_hwmon_sysfs(psy);
> 
> Do we need some locking here or is this ok?

This should be fine. All callers hold the lock.
As above, I'll fix the name and add explicit lockdep assertions.
 
> > +}
> > +
> > +int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext,
> > +				    void *data)
> > +{
> > +	struct power_supply_ext_registration *reg;
> > +	size_t i;
> > +
> > +	guard(rwsem_write)(&psy->extensions_sem);
> > +
> > +	power_supply_for_each_extension(reg, psy)
> > +		if (reg->ext == ext)
> > +			return -EEXIST;
> > +
> > +	for (i = 0; i < ext->num_properties; i++)
> > +		if (power_supply_has_property(psy, ext->properties[i]))
> > +			return -EEXIST;
> > +
> > +	reg = kmalloc(sizeof(*reg), GFP_KERNEL);
> > +	if (!reg)
> > +		return -ENOMEM;
> > +
> > +	reg->ext = ext;
> > +	reg->data = data;
> > +	list_add(&reg->list_head, &psy->extensions);
> > +
> > +	return power_supply_update_sysfs_and_hwmon(psy);
> 
> We need to clean up *reg here should power_supply_update_sysfs_and_hwmon() fail.

Ack.

> > +}
> > +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> > +
> > +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> > +{
> > +	struct power_supply_ext_registration *reg;
> > +
> > +	guard(rwsem_write)(&psy->extensions_sem);
> > +
> > +	power_supply_for_each_extension(reg, psy) {
> > +		if (reg->ext == ext) {
> > +			list_del(&reg->list_head);
> > +			kfree(reg);
> > +			power_supply_update_sysfs_and_hwmon(psy);
> > +			return;
> > +		}
> > +	}
> > +
> > +	dev_warn(&psy->dev, "Trying to unregister invalid extension");
> > +}
> > +EXPORT_SYMBOL_GPL(power_supply_unregister_extension);
> > +
> >   static void power_supply_dev_release(struct device *dev)
> >   {
> >   	struct power_supply *psy = to_power_supply(dev);
> > @@ -1414,6 +1548,9 @@ __power_supply_register(struct device *parent,
> >   	}
> > 
> >   	spin_lock_init(&psy->changed_lock);
> > +	init_rwsem(&psy->extensions_sem);
> > +	INIT_LIST_HEAD(&psy->extensions);
> > +
> >   	rc = device_add(dev);
> >   	if (rc)
> >   		goto device_add_failed;
> > @@ -1426,13 +1563,15 @@ __power_supply_register(struct device *parent,
> >   	if (rc)
> >   		goto register_thermal_failed;
> > 
> > -	rc = power_supply_create_triggers(psy);
> > -	if (rc)
> > -		goto create_triggers_failed;
> > +	scoped_guard(rwsem_read, &psy->extensions_sem) {
> > +		rc = power_supply_create_triggers(psy);
> > +		if (rc)
> > +			goto create_triggers_failed;
> > 
> > -	rc = power_supply_add_hwmon_sysfs(psy);
> > -	if (rc)
> > -		goto add_hwmon_sysfs_failed;
> > +		rc = power_supply_add_hwmon_sysfs(psy);
> > +		if (rc)
> > +			goto add_hwmon_sysfs_failed;
> > +	}
> > 
> >   	/*
> >   	 * Update use_cnt after any uevents (most notably from device_add()).
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index 99bfe1f03eb8326d38c4e2831c9670313b42e425..2cf25bacd7a1bb66e5a72629bffaa6d16bfbf3be 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -268,6 +268,23 @@ static ssize_t power_supply_show_enum_with_available(
> >   	return count;
> >   }
> > 
> > +static ssize_t power_supply_show_charge_behaviour(struct device *dev,
> > +						  struct power_supply *psy,
> > +						  union power_supply_propval *value,
> > +						  char *buf)
> > +{
> > +	struct power_supply_ext_registration *reg;
> > +
> 
> Missing guard here.

It looks like it. Lockdep should have complained about it due to the
implicit assertion in power_supply_for_each_extension().
Or maybe I didn't test this part... will investigate.

> Thanks,
> Armin Wolf
> 
> > +	power_supply_for_each_extension(reg, psy) {
> > +		if (power_supply_ext_has_property(reg->ext, POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR))
> > +			return power_supply_charge_behaviour_show(dev, reg->ext->charge_behaviours,
> > +								  value->intval, buf);
> > +	}
> > +
> > +	return power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> > +						  value->intval, buf);
> > +}
> > +
> >   static ssize_t power_supply_format_property(struct device *dev,
> >   					    bool uevent,
> >   					    struct device_attribute *attr,
> > @@ -307,8 +324,7 @@ static ssize_t power_supply_format_property(struct device *dev,
> >   	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> >   		if (uevent) /* no possible values in uevents */
> >   			goto default_format;
> > -		ret = power_supply_charge_behaviour_show(dev, psy->desc->charge_behaviours,
> > -							 value.intval, buf);
> > +		ret = power_supply_show_charge_behaviour(dev, psy, &value, buf);
> >   		break;
> >   	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
> >   		ret = sysfs_emit(buf, "%s\n", value.strval);
> > @@ -385,6 +401,8 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> >   	if (attrno == POWER_SUPPLY_PROP_TYPE)
> >   		return mode;
> > 
> > +	guard(rwsem_read)(&psy->extensions_sem);
> > +
> >   	if (power_supply_has_property(psy, attrno)) {
> >   		if (power_supply_property_is_writeable(psy, attrno) > 0)
> >   			mode |= S_IWUSR;
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index b98106e1a90f34bce5129317a099f363248342b9..016e44cb3eb5eb7ace01a032661f65a5d81a522f 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -15,6 +15,8 @@
> >   #include <linux/device.h>
> >   #include <linux/workqueue.h>
> >   #include <linux/leds.h>
> > +#include <linux/rwsem.h>
> > +#include <linux/list.h>
> >   #include <linux/spinlock.h>
> >   #include <linux/notifier.h>
> > 
> > @@ -281,6 +283,27 @@ struct power_supply_desc {
> >   	int use_for_apm;
> >   };
> > 
> > +struct power_supply_ext {
> > +	u8 charge_behaviours;
> > +	const enum power_supply_property *properties;
> > +	size_t num_properties;
> > +
> > +	int (*get_property)(struct power_supply *psy,
> > +			    const struct power_supply_ext *ext,
> > +			    void *data,
> > +			    enum power_supply_property psp,
> > +			    union power_supply_propval *val);
> > +	int (*set_property)(struct power_supply *psy,
> > +			    const struct power_supply_ext *ext,
> > +			    void *data,
> > +			    enum power_supply_property psp,
> > +			    const union power_supply_propval *val);
> > +	int (*property_is_writeable)(struct power_supply *psy,
> > +				     const struct power_supply_ext *ext,
> > +				     void *data,
> > +				     enum power_supply_property psp);
> > +};
> > +
> >   struct power_supply {
> >   	const struct power_supply_desc *desc;
> > 
> > @@ -300,10 +323,13 @@ struct power_supply {
> >   	struct delayed_work deferred_register_work;
> >   	spinlock_t changed_lock;
> >   	bool changed;
> > +	bool update_groups;
> >   	bool initialized;
> >   	bool removing;
> >   	atomic_t use_cnt;
> >   	struct power_supply_battery_info *battery_info;
> > +	struct rw_semaphore extensions_sem; /* protects "extensions" */
> > +	struct list_head extensions;
> >   #ifdef CONFIG_THERMAL
> >   	struct thermal_zone_device *tzd;
> >   	struct thermal_cooling_device *tcd;
> > @@ -878,6 +904,12 @@ devm_power_supply_register(struct device *parent,
> >   extern void power_supply_unregister(struct power_supply *psy);
> >   extern int power_supply_powers(struct power_supply *psy, struct device *dev);
> > 
> > +extern int power_supply_register_extension(struct power_supply *psy,
> > +					   const struct power_supply_ext *ext,
> > +					   void *data);
> > +extern void power_supply_unregister_extension(struct power_supply *psy,
> > +					      const struct power_supply_ext *ext);
> > +
> >   #define to_power_supply(device) container_of(device, struct power_supply, dev)
> > 
> >   extern void *power_supply_get_drvdata(struct power_supply *psy);
> > 

Re: (subset) [PATCH v4 0/9] power: supply: extension API

[Sebastian Reichel]

On Mon, 11 Nov 2024 22:40:02 +0100, Thomas Weißschuh wrote:
> Introduce a mechanism for drivers to extend the properties implemented
> by a power supply.
> 
> Motivation
> ----------
> 
> Various drivers, mostly in platform/x86 extend the ACPI battery driver
> with additional sysfs attributes to implement more UAPIs than are
> exposed through ACPI by using various side-channels, like WMI,
> nonstandard ACPI or EC communication.
> 
> [...]

Applied, thanks!

[1/9] power: supply: sysfs: print single value in uevent for POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR
      commit: 31d8440e07704d53041936222728636421957660
[2/9] power: supply: core: rename psy_has_property() to psy_desc_has_property()
      commit: 172f2151c2b436746173d794887115e026961d82
[3/9] power: supply: core: introduce power_supply_has_property()
      commit: aa40f37d636570458e1be76f51564357347eb77c
[4/9] power: supply: hwmon: prepare for power supply extensions
      commit: 39bb32f06c1f7eb34b4a9838e878f3d741b7d50c
[5/9] power: supply: sysfs: prepare for power supply extensions
      commit: 5c2141f2c7c671e8696e2ee1c7b332c77266dd08
[6/9] power: supply: sysfs: rework uevent property loop
      commit: f29a749d01dc136ee6e08afafebbccc389ef5b05

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