[PATCH RFC v2 0/5] power: supply: extension API

[PATCH RFC v2 0/5] 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 not implemented anymore, can be readded)

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.

For example my upcoming cros_ec charge control driver[0] saves 80 lines
of code with this patchset.

Contents
--------

* Patch 1 and 2 are generic preparation patches, that probably make
  sense without this series.
* Patch 3 implements the extension API itself.
* Patch 4 implements a PoC locking scheme for the extension API.
* Patch 5 adds extension support to test_power.c
* Patch 6 converts the in-tree platform/x86/system76 driver to the
  extension API.

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

* Newly registered properties will not show up in hwmon.
  To do that properly would require some changes in the hwmon core.
  As far as I know, no current driver would extend the hwmon properties anyways.
* As this is only useful with the hooks of CONFIG_ACPI_BATTERY, should
  it also be gated behind this or another config?
* Only one extension can be used at a time.
  So far this should be enough, having more would complicate the
  implementation.
* 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 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 (5):
      power: supply: sysfs: use power_supply_property_is_writeable()
      power: supply: core: avoid iterating properties directly
      power: supply: core: implement extension API
      power: supply: test-power: implement a power supply extension
      platform/x86: system76: Use power_supply extension API

 drivers/platform/x86/system76_acpi.c      |  93 ++++++++++++----------
 drivers/power/supply/power_supply.h       |  12 +++
 drivers/power/supply/power_supply_core.c  | 128 ++++++++++++++++++++++++++++--
 drivers/power/supply/power_supply_hwmon.c |  48 +++++------
 drivers/power/supply/power_supply_sysfs.c |  50 +++++++++---
 drivers/power/supply/test_power.c         | 101 +++++++++++++++++++++++
 include/linux/power_supply.h              |  26 ++++++
 7 files changed, 371 insertions(+), 87 deletions(-)
---
base-commit: 061d1af7b0305227182bd9da60c7706c079348b7
change-id: 20240602-power-supply-extensions-07d949f509d9

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

[PATCH RFC v2 1/5] power: supply: sysfs: use power_supply_property_is_writeable()

[Thomas Weißschuh]

Instead of open-coding the helper use it directly.

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

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index b86e11bdc07e..3e63d165b2f7 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -379,8 +379,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 		int property = psy->desc->properties[i];
 
 		if (property == attrno) {
-			if (psy->desc->property_is_writeable &&
-			    psy->desc->property_is_writeable(psy, property) > 0)
+			if (power_supply_property_is_writeable(psy, property) > 0)
 				mode |= S_IWUSR;
 
 			return mode;

-- 
2.45.2

[PATCH RFC v2 2/5] power: supply: core: avoid iterating properties directly

[Thomas Weißschuh]

With the introduction of power supply extension, it will not be enough
to iterate the properties on the struct power_supply directly.
Instead introduce a helper power_supply_has_property() which ill handle
properties added by extensions.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h       |  3 ++
 drivers/power/supply/power_supply_core.c  | 10 +++----
 drivers/power/supply/power_supply_hwmon.c | 48 +++++++++++++++----------------
 drivers/power/supply/power_supply_sysfs.c | 14 +++------
 4 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 3cbafc58bdad..622be1f0a180 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,6 +13,9 @@ struct device;
 struct device_type;
 struct power_supply;
 
+extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+				      enum power_supply_property psp);
+
 #ifdef CONFIG_SYSFS
 
 extern void power_supply_init_attrs(void);
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index fefe938c9342..d57ecdd966e0 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,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)
+bool power_supply_has_property(const struct power_supply_desc *psy_desc,
+			       enum power_supply_property psp)
 {
 	bool found = false;
 	int i;
@@ -1209,7 +1209,7 @@ int power_supply_get_property(struct power_supply *psy,
 		return -ENODEV;
 	}
 
-	if (psy_has_property(psy->desc, psp))
+	if (power_supply_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);
@@ -1308,7 +1308,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 (power_supply_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)
@@ -1361,7 +1361,7 @@ __power_supply_register(struct device *parent,
 		pr_warn("%s: Expected proper parent device for '%s'\n",
 			__func__, desc->name);
 
-	if (psy_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+	if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
 	    (!desc->usb_types || !desc->num_usb_types))
 		return ERR_PTR(-EINVAL);
 
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index c97893d4c25e..2ecbe4a74c25 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -8,6 +8,8 @@
 #include <linux/power_supply.h>
 #include <linux/slab.h>
 
+#include "power_supply.h"
+
 struct power_supply_hwmon {
 	struct power_supply *psy;
 	unsigned long *props;
@@ -324,9 +326,26 @@ 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_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;
@@ -352,30 +371,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_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->desc, prop))
 			set_bit(prop, psyhw->props);
-			break;
-		default:
-			break;
-		}
 	}
 
 	name = psy->desc->name;
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 3e63d165b2f7..abd44ebfe6fe 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -367,7 +367,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;
@@ -375,15 +374,10 @@ 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_has_property(psy->desc, attrno)) {
+		if (power_supply_property_is_writeable(psy, attrno) > 0)
+			mode |= S_IWUSR;
+		return mode;
 	}
 
 	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))

-- 
2.45.2

Re: [PATCH RFC v2 2/5] power: supply: core: avoid iterating properties directly

[Sebastian Reichel]

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

Hi,

On Sat, Jun 08, 2024 at 09:19:38PM GMT, Thomas Weißschuh wrote:
> With the introduction of power supply extension, it will not be enough
> to iterate the properties on the struct power_supply directly.
> Instead introduce a helper power_supply_has_property() which will handle
> properties added by extensions.

power_supply_has_property() needs to include properties from
battery_info, which are exposed in addition to the driver's
own properties.

Greetings,

-- Sebastian

> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/power/supply/power_supply.h       |  3 ++
>  drivers/power/supply/power_supply_core.c  | 10 +++----
>  drivers/power/supply/power_supply_hwmon.c | 48 +++++++++++++++----------------
>  drivers/power/supply/power_supply_sysfs.c | 14 +++------
>  4 files changed, 36 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 3cbafc58bdad..622be1f0a180 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -13,6 +13,9 @@ struct device;
>  struct device_type;
>  struct power_supply;
>  
> +extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> +				      enum power_supply_property psp);
> +
>  #ifdef CONFIG_SYSFS
>  
>  extern void power_supply_init_attrs(void);
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index fefe938c9342..d57ecdd966e0 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1183,8 +1183,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)
> +bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> +			       enum power_supply_property psp)
>  {
>  	bool found = false;
>  	int i;
> @@ -1209,7 +1209,7 @@ int power_supply_get_property(struct power_supply *psy,
>  		return -ENODEV;
>  	}
>  
> -	if (psy_has_property(psy->desc, psp))
> +	if (power_supply_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);
> @@ -1308,7 +1308,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 (power_supply_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)
> @@ -1361,7 +1361,7 @@ __power_supply_register(struct device *parent,
>  		pr_warn("%s: Expected proper parent device for '%s'\n",
>  			__func__, desc->name);
>  
> -	if (psy_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
> +	if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
>  	    (!desc->usb_types || !desc->num_usb_types))
>  		return ERR_PTR(-EINVAL);
>  
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index c97893d4c25e..2ecbe4a74c25 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -8,6 +8,8 @@
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
>  
> +#include "power_supply.h"
> +
>  struct power_supply_hwmon {
>  	struct power_supply *psy;
>  	unsigned long *props;
> @@ -324,9 +326,26 @@ 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_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;
> @@ -352,30 +371,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_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->desc, prop))
>  			set_bit(prop, psyhw->props);
> -			break;
> -		default:
> -			break;
> -		}
>  	}
>  
>  	name = psy->desc->name;
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 3e63d165b2f7..abd44ebfe6fe 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -367,7 +367,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;
> @@ -375,15 +374,10 @@ 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_has_property(psy->desc, attrno)) {
> +		if (power_supply_property_is_writeable(psy, attrno) > 0)
> +			mode |= S_IWUSR;
> +		return mode;
>  	}
>  
>  	if (power_supply_battery_info_has_prop(psy->battery_info, attrno))
> 
> -- 
> 2.45.2
> 
> 

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

[PATCH RFC v2 3/5] power: supply: core: implement extension API

[Thomas Weißschuh]

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/power/supply/power_supply.h       |  13 ++-
 drivers/power/supply/power_supply_core.c  | 128 ++++++++++++++++++++++++++++--
 drivers/power/supply/power_supply_hwmon.c |   2 +-
 drivers/power/supply/power_supply_sysfs.c |  37 ++++++++-
 include/linux/power_supply.h              |  26 ++++++
 5 files changed, 192 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index 622be1f0a180..686b66161900 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -13,8 +13,17 @@ struct device;
 struct device_type;
 struct power_supply;
 
-extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
-				      enum power_supply_property psp);
+struct psy_ext_registration {
+	struct list_head list_head;
+	const struct power_supply_ext *ext;
+};
+
+#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
+
+bool power_supply_has_property(const struct power_supply *psy,
+			       enum power_supply_property psp);
+bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
+				   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 d57ecdd966e0..fcdfedf3df7c 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
 }
 EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
 
-bool power_supply_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;
@@ -1199,17 +1199,55 @@ bool power_supply_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;
+
+	if (!psy_ext)
+		return false;
+
+	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(const struct power_supply *psy,
+			       enum power_supply_property psp)
+{
+	struct psy_ext_registration *reg;
+
+	psy_for_each_extension(psy, reg)
+		if (power_supply_ext_has_property(reg->ext, psp))
+			return true;
+
+	return psy_desc_has_property(psy->desc, psp);
+}
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
 {
+	struct psy_ext_registration *reg;
+
 	if (atomic_read(&psy->use_cnt) <= 0) {
 		if (!psy->initialized)
 			return -EAGAIN;
 		return -ENODEV;
 	}
 
-	if (power_supply_has_property(psy->desc, psp))
+	psy_for_each_extension(psy, reg) {
+		if (power_supply_ext_has_property(reg->ext, psp))
+			return reg->ext->get_property(psy, reg->ext, 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))
 		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
@@ -1222,7 +1260,21 @@ 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 psy_ext_registration *reg;
+
+	if (atomic_read(&psy->use_cnt) <= 0)
+		return -ENODEV;
+
+	psy_for_each_extension(psy, reg) {
+		if (power_supply_ext_has_property(reg->ext, psp)) {
+			if (reg->ext->set_property)
+				return reg->ext->set_property(psy, reg->ext, psp, val);
+			else
+				return -ENODEV;
+		}
+	}
+
+	if (!psy->desc->set_property)
 		return -ENODEV;
 
 	return psy->desc->set_property(psy, psp, val);
@@ -1232,8 +1284,21 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
 int power_supply_property_is_writeable(struct power_supply *psy,
 					enum power_supply_property psp)
 {
-	if (atomic_read(&psy->use_cnt) <= 0 ||
-			!psy->desc->property_is_writeable)
+	struct psy_ext_registration *reg;
+
+	if (atomic_read(&psy->use_cnt) <= 0)
+		return -ENODEV;
+
+	psy_for_each_extension(psy, reg) {
+		if (power_supply_ext_has_property(reg->ext, psp)) {
+			if (reg->ext->property_is_writeable)
+				return reg->ext->property_is_writeable(psy, reg->ext, psp);
+			else
+				return -ENODEV;
+		}
+	}
+
+	if (!psy->desc->property_is_writeable)
 		return -ENODEV;
 
 	return psy->desc->property_is_writeable(psy, psp);
@@ -1256,6 +1321,52 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(power_supply_powers);
 
+static int power_supply_update_groups(struct power_supply *psy)
+{
+	int ret;
+
+	ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
+	power_supply_changed(psy);
+	return ret;
+}
+
+int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+	struct psy_ext_registration *reg;
+	size_t i;
+
+	for (i = 0; i < ext->num_properties; i++) {
+		if (power_supply_has_property(psy, ext->properties[i]))
+			return -EEXIST;
+	}
+
+	reg = devm_kmalloc(&psy->dev, sizeof(*reg), GFP_KERNEL);
+	if (!reg)
+		return -ENOMEM;
+
+	reg->ext = ext;
+	list_add(&reg->list_head, &psy->extensions);
+
+	return power_supply_update_groups(psy);
+}
+EXPORT_SYMBOL_GPL(power_supply_register_extension);
+
+void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
+{
+	struct psy_ext_registration *reg;
+
+	psy_for_each_extension(psy, reg) {
+		if (reg->ext == ext) {
+			list_del(&reg->list_head);
+			power_supply_update_groups(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);
@@ -1308,7 +1419,7 @@ static int psy_register_thermal(struct power_supply *psy)
 		return 0;
 
 	/* Register battery zone device psy reports temperature */
-	if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
+	if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
 		/* Prefer our hwmon device and avoid duplicates */
 		struct thermal_zone_params tzp = {
 			.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
@@ -1361,7 +1472,7 @@ __power_supply_register(struct device *parent,
 		pr_warn("%s: Expected proper parent device for '%s'\n",
 			__func__, desc->name);
 
-	if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
+	if (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
 	    (!desc->usb_types || !desc->num_usb_types))
 		return ERR_PTR(-EINVAL);
 
@@ -1415,6 +1526,7 @@ __power_supply_register(struct device *parent,
 	}
 
 	spin_lock_init(&psy->changed_lock);
+	INIT_LIST_HEAD(&psy->extensions);
 	rc = device_add(dev);
 	if (rc)
 		goto device_add_failed;
diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
index 2ecbe4a74c25..8cb852a734b1 100644
--- a/drivers/power/supply/power_supply_hwmon.c
+++ b/drivers/power/supply/power_supply_hwmon.c
@@ -374,7 +374,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
 	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->desc, prop))
+		if (power_supply_has_property(psy, prop))
 			set_bit(prop, psyhw->props);
 	}
 
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index abd44ebfe6fe..8f29ddea08d0 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
 	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 psy_ext_registration *reg;
+
+	psy_for_each_extension(psy, reg) {
+		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_show_property(struct device *dev,
 					  struct device_attribute *attr,
 					  char *buf) {
@@ -304,8 +321,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 						&value, buf);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
-		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);
@@ -374,7 +390,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 	if (attrno == POWER_SUPPLY_PROP_TYPE)
 		return mode;
 
-	if (power_supply_has_property(psy->desc, attrno)) {
+	if (power_supply_has_property(psy, attrno)) {
 		if (power_supply_property_is_writeable(psy, attrno) > 0)
 			mode |= S_IWUSR;
 		return mode;
@@ -459,6 +475,7 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 		power_supply_battery_info_properties;
 	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
 					 sizeof(unsigned long) + 1] = {0};
+	struct psy_ext_registration *ext;
 	int ret = 0, j;
 	char *prop_buf;
 
@@ -486,7 +503,21 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	if (ret)
 		goto out;
 
+	psy_for_each_extension(psy, ext) {
+		for (j = 0; j < ext->ext->num_properties; j++) {
+			if (test_bit(ext->ext->properties[j], psy_drv_properties))
+				continue;
+			set_bit(ext->ext->properties[j], psy_drv_properties);
+			ret = add_prop_uevent(dev, env, ext->ext->properties[j],
+					      prop_buf);
+			if (ret)
+				goto out;
+		}
+	}
+
 	for (j = 0; j < psy->desc->num_properties; j++) {
+		if (test_bit(psy->desc->properties[j], psy_drv_properties))
+			continue;
 		set_bit(psy->desc->properties[j], psy_drv_properties);
 		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
 				      prop_buf);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 8e5705a56b85..128e7a67f268 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>
 #include <linux/workqueue.h>
 #include <linux/leds.h>
+#include <linux/list.h>
 #include <linux/spinlock.h>
 #include <linux/notifier.h>
 
@@ -280,6 +281,25 @@ 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,
+			    enum power_supply_property psp,
+			    union power_supply_propval *val);
+	int (*set_property)(struct power_supply *psy,
+			    const struct power_supply_ext *ext,
+			    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,
+				     enum power_supply_property psp);
+};
+
 struct power_supply {
 	const struct power_supply_desc *desc;
 
@@ -303,6 +323,7 @@ struct power_supply {
 	bool removing;
 	atomic_t use_cnt;
 	struct power_supply_battery_info *battery_info;
+	struct list_head extensions;
 #ifdef CONFIG_THERMAL
 	struct thermal_zone_device *tzd;
 	struct thermal_cooling_device *tcd;
@@ -892,6 +913,11 @@ devm_power_supply_register_no_ws(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);
+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.45.2

Re: [PATCH RFC v2 3/5] power: supply: core: implement extension API

[Armin Wolf]

Am 08.06.24 um 21:19 schrieb Thomas Weißschuh:

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/power/supply/power_supply.h       |  13 ++-
>   drivers/power/supply/power_supply_core.c  | 128 ++++++++++++++++++++++++++++--
>   drivers/power/supply/power_supply_hwmon.c |   2 +-
>   drivers/power/supply/power_supply_sysfs.c |  37 ++++++++-
>   include/linux/power_supply.h              |  26 ++++++
>   5 files changed, 192 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index 622be1f0a180..686b66161900 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -13,8 +13,17 @@ struct device;
>   struct device_type;
>   struct power_supply;
>
> -extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> -				      enum power_supply_property psp);
> +struct psy_ext_registration {
> +	struct list_head list_head;
> +	const struct power_supply_ext *ext;
> +};
> +
> +#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)

Hi,

sorry for taking so long to respond, the patch looks good to me except one single thing:

when removing a power supply extension, the driver has to be sure that no one is still using
the removed extension. So you might want to add some sort of locking when using a power supply
extension.

Thanks,
Armin Wolf

> +
> +bool power_supply_has_property(const struct power_supply *psy,
> +			       enum power_supply_property psp);
> +bool power_supply_ext_has_property(const struct power_supply_ext *psy_ext,
> +				   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 d57ecdd966e0..fcdfedf3df7c 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1183,8 +1183,8 @@ bool power_supply_battery_bti_in_range(struct power_supply_battery_info *info,
>   }
>   EXPORT_SYMBOL_GPL(power_supply_battery_bti_in_range);
>
> -bool power_supply_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;
> @@ -1199,17 +1199,55 @@ bool power_supply_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;
> +
> +	if (!psy_ext)
> +		return false;
> +
> +	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(const struct power_supply *psy,
> +			       enum power_supply_property psp)
> +{
> +	struct psy_ext_registration *reg;
> +
> +	psy_for_each_extension(psy, reg)
> +		if (power_supply_ext_has_property(reg->ext, psp))
> +			return true;
> +
> +	return psy_desc_has_property(psy->desc, psp);
> +}
> +
>   int power_supply_get_property(struct power_supply *psy,
>   			    enum power_supply_property psp,
>   			    union power_supply_propval *val)
>   {
> +	struct psy_ext_registration *reg;
> +
>   	if (atomic_read(&psy->use_cnt) <= 0) {
>   		if (!psy->initialized)
>   			return -EAGAIN;
>   		return -ENODEV;
>   	}
>
> -	if (power_supply_has_property(psy->desc, psp))
> +	psy_for_each_extension(psy, reg) {
> +		if (power_supply_ext_has_property(reg->ext, psp))
> +			return reg->ext->get_property(psy, reg->ext, 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))
>   		return power_supply_battery_info_get_prop(psy->battery_info, psp, val);
> @@ -1222,7 +1260,21 @@ 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 psy_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	psy_for_each_extension(psy, reg) {
> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->set_property)
> +				return reg->ext->set_property(psy, reg->ext, psp, val);
> +			else
> +				return -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->set_property)
>   		return -ENODEV;
>
>   	return psy->desc->set_property(psy, psp, val);
> @@ -1232,8 +1284,21 @@ EXPORT_SYMBOL_GPL(power_supply_set_property);
>   int power_supply_property_is_writeable(struct power_supply *psy,
>   					enum power_supply_property psp)
>   {
> -	if (atomic_read(&psy->use_cnt) <= 0 ||
> -			!psy->desc->property_is_writeable)
> +	struct psy_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	psy_for_each_extension(psy, reg) {
> +		if (power_supply_ext_has_property(reg->ext, psp)) {
> +			if (reg->ext->property_is_writeable)
> +				return reg->ext->property_is_writeable(psy, reg->ext, psp);
> +			else
> +				return -ENODEV;
> +		}
> +	}
> +
> +	if (!psy->desc->property_is_writeable)
>   		return -ENODEV;
>
>   	return psy->desc->property_is_writeable(psy, psp);
> @@ -1256,6 +1321,52 @@ int power_supply_powers(struct power_supply *psy, struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(power_supply_powers);
>
> +static int power_supply_update_groups(struct power_supply *psy)
> +{
> +	int ret;
> +
> +	ret = sysfs_update_groups(&psy->dev.kobj, power_supply_dev_type.groups);
> +	power_supply_changed(psy);
> +	return ret;
> +}
> +
> +int power_supply_register_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> +	struct psy_ext_registration *reg;
> +	size_t i;
> +
> +	for (i = 0; i < ext->num_properties; i++) {
> +		if (power_supply_has_property(psy, ext->properties[i]))
> +			return -EEXIST;
> +	}
> +
> +	reg = devm_kmalloc(&psy->dev, sizeof(*reg), GFP_KERNEL);
> +	if (!reg)
> +		return -ENOMEM;
> +
> +	reg->ext = ext;
> +	list_add(&reg->list_head, &psy->extensions);
> +
> +	return power_supply_update_groups(psy);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_register_extension);
> +
> +void power_supply_unregister_extension(struct power_supply *psy, const struct power_supply_ext *ext)
> +{
> +	struct psy_ext_registration *reg;
> +
> +	psy_for_each_extension(psy, reg) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			power_supply_update_groups(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);
> @@ -1308,7 +1419,7 @@ static int psy_register_thermal(struct power_supply *psy)
>   		return 0;
>
>   	/* Register battery zone device psy reports temperature */
> -	if (power_supply_has_property(psy->desc, POWER_SUPPLY_PROP_TEMP)) {
> +	if (power_supply_has_property(psy, POWER_SUPPLY_PROP_TEMP)) {
>   		/* Prefer our hwmon device and avoid duplicates */
>   		struct thermal_zone_params tzp = {
>   			.no_hwmon = IS_ENABLED(CONFIG_POWER_SUPPLY_HWMON)
> @@ -1361,7 +1472,7 @@ __power_supply_register(struct device *parent,
>   		pr_warn("%s: Expected proper parent device for '%s'\n",
>   			__func__, desc->name);
>
> -	if (power_supply_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
> +	if (psy_desc_has_property(desc, POWER_SUPPLY_PROP_USB_TYPE) &&
>   	    (!desc->usb_types || !desc->num_usb_types))
>   		return ERR_PTR(-EINVAL);
>
> @@ -1415,6 +1526,7 @@ __power_supply_register(struct device *parent,
>   	}
>
>   	spin_lock_init(&psy->changed_lock);
> +	INIT_LIST_HEAD(&psy->extensions);
>   	rc = device_add(dev);
>   	if (rc)
>   		goto device_add_failed;
> diff --git a/drivers/power/supply/power_supply_hwmon.c b/drivers/power/supply/power_supply_hwmon.c
> index 2ecbe4a74c25..8cb852a734b1 100644
> --- a/drivers/power/supply/power_supply_hwmon.c
> +++ b/drivers/power/supply/power_supply_hwmon.c
> @@ -374,7 +374,7 @@ int power_supply_add_hwmon_sysfs(struct power_supply *psy)
>   	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->desc, prop))
> +		if (power_supply_has_property(psy, prop))
>   			set_bit(prop, psyhw->props);
>   	}
>
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index abd44ebfe6fe..8f29ddea08d0 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -271,6 +271,23 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>   	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 psy_ext_registration *reg;
> +
> +	psy_for_each_extension(psy, reg) {
> +		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_show_property(struct device *dev,
>   					  struct device_attribute *attr,
>   					  char *buf) {
> @@ -304,8 +321,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>   						&value, buf);
>   		break;
>   	case POWER_SUPPLY_PROP_CHARGE_BEHAVIOUR:
> -		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);
> @@ -374,7 +390,7 @@ static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>   	if (attrno == POWER_SUPPLY_PROP_TYPE)
>   		return mode;
>
> -	if (power_supply_has_property(psy->desc, attrno)) {
> +	if (power_supply_has_property(psy, attrno)) {
>   		if (power_supply_property_is_writeable(psy, attrno) > 0)
>   			mode |= S_IWUSR;
>   		return mode;
> @@ -459,6 +475,7 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   		power_supply_battery_info_properties;
>   	unsigned long psy_drv_properties[POWER_SUPPLY_ATTR_CNT /
>   					 sizeof(unsigned long) + 1] = {0};
> +	struct psy_ext_registration *ext;
>   	int ret = 0, j;
>   	char *prop_buf;
>
> @@ -486,7 +503,21 @@ int power_supply_uevent(const struct device *dev, struct kobj_uevent_env *env)
>   	if (ret)
>   		goto out;
>
> +	psy_for_each_extension(psy, ext) {
> +		for (j = 0; j < ext->ext->num_properties; j++) {
> +			if (test_bit(ext->ext->properties[j], psy_drv_properties))
> +				continue;
> +			set_bit(ext->ext->properties[j], psy_drv_properties);
> +			ret = add_prop_uevent(dev, env, ext->ext->properties[j],
> +					      prop_buf);
> +			if (ret)
> +				goto out;
> +		}
> +	}
> +
>   	for (j = 0; j < psy->desc->num_properties; j++) {
> +		if (test_bit(psy->desc->properties[j], psy_drv_properties))
> +			continue;
>   		set_bit(psy->desc->properties[j], psy_drv_properties);
>   		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
>   				      prop_buf);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 8e5705a56b85..128e7a67f268 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -15,6 +15,7 @@
>   #include <linux/device.h>
>   #include <linux/workqueue.h>
>   #include <linux/leds.h>
> +#include <linux/list.h>
>   #include <linux/spinlock.h>
>   #include <linux/notifier.h>
>
> @@ -280,6 +281,25 @@ 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,
> +			    enum power_supply_property psp,
> +			    union power_supply_propval *val);
> +	int (*set_property)(struct power_supply *psy,
> +			    const struct power_supply_ext *ext,
> +			    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,
> +				     enum power_supply_property psp);
> +};
> +
>   struct power_supply {
>   	const struct power_supply_desc *desc;
>
> @@ -303,6 +323,7 @@ struct power_supply {
>   	bool removing;
>   	atomic_t use_cnt;
>   	struct power_supply_battery_info *battery_info;
> +	struct list_head extensions;
>   #ifdef CONFIG_THERMAL
>   	struct thermal_zone_device *tzd;
>   	struct thermal_cooling_device *tcd;
> @@ -892,6 +913,11 @@ devm_power_supply_register_no_ws(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);
> +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 RFC v2 3/5] power: supply: core: implement extension API

[Thomas Weißschuh]

Hi!

On 2024-06-14 01:11:29+0000, Armin Wolf wrote:
> Am 08.06.24 um 21:19 schrieb Thomas Weißschuh:
> 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   drivers/power/supply/power_supply.h       |  13 ++-
> >   drivers/power/supply/power_supply_core.c  | 128 ++++++++++++++++++++++++++++--
> >   drivers/power/supply/power_supply_hwmon.c |   2 +-
> >   drivers/power/supply/power_supply_sysfs.c |  37 ++++++++-
> >   include/linux/power_supply.h              |  26 ++++++
> >   5 files changed, 192 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index 622be1f0a180..686b66161900 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -13,8 +13,17 @@ struct device;
> >   struct device_type;
> >   struct power_supply;
> > 
> > -extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
> > -				      enum power_supply_property psp);
> > +struct psy_ext_registration {
> > +	struct list_head list_head;
> > +	const struct power_supply_ext *ext;
> > +};
> > +
> > +#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
> 
> sorry for taking so long to respond, the patch looks good to me except one single thing:
> 
> when removing a power supply extension, the driver has to be sure that no one is still using
> the removed extension. So you might want to add some sort of locking when using a power supply
> extension.

That was part of the v1 of the series [0].
I dropped it in v2 to focus on the external API and semantics first.
IMO now that we have the loop macro anyways that can also take care of
the locking: psy_for_each_extension_locked()

[0] https://lore.kernel.org/all/20240606-power-supply-extensions-v1-4-b45669290bdc@weissschuh.net/

Thomas

<snip>

Re: [PATCH RFC v2 3/5] power: supply: core: implement extension API

[Armin Wolf]

Am 14.06.24 um 09:02 schrieb Thomas Weißschuh:

> Hi!
>
> On 2024-06-14 01:11:29+0000, Armin Wolf wrote:
>> Am 08.06.24 um 21:19 schrieb Thomas Weißschuh:
>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>    drivers/power/supply/power_supply.h       |  13 ++-
>>>    drivers/power/supply/power_supply_core.c  | 128 ++++++++++++++++++++++++++++--
>>>    drivers/power/supply/power_supply_hwmon.c |   2 +-
>>>    drivers/power/supply/power_supply_sysfs.c |  37 ++++++++-
>>>    include/linux/power_supply.h              |  26 ++++++
>>>    5 files changed, 192 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
>>> index 622be1f0a180..686b66161900 100644
>>> --- a/drivers/power/supply/power_supply.h
>>> +++ b/drivers/power/supply/power_supply.h
>>> @@ -13,8 +13,17 @@ struct device;
>>>    struct device_type;
>>>    struct power_supply;
>>>
>>> -extern bool power_supply_has_property(const struct power_supply_desc *psy_desc,
>>> -				      enum power_supply_property psp);
>>> +struct psy_ext_registration {
>>> +	struct list_head list_head;
>>> +	const struct power_supply_ext *ext;
>>> +};
>>> +
>>> +#define psy_for_each_extension(psy, pos) list_for_each_entry(pos, &(psy)->extensions, list_head)
>> sorry for taking so long to respond, the patch looks good to me except one single thing:
>>
>> when removing a power supply extension, the driver has to be sure that no one is still using
>> the removed extension. So you might want to add some sort of locking when using a power supply
>> extension.
> That was part of the v1 of the series [0].
> I dropped it in v2 to focus on the external API and semantics first.
> IMO now that we have the loop macro anyways that can also take care of
> the locking: psy_for_each_extension_locked()
>
> [0] https://lore.kernel.org/all/20240606-power-supply-extensions-v1-4-b45669290bdc@weissschuh.net/
>
> Thomas

Sounds good to me.

Thanks,
Armin Wolf

> <snip>

[PATCH RFC v2 4/5] 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 | 101 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/power/supply/test_power.c b/drivers/power/supply/test_power.c
index 442ceb7795e1..12c1573e547a 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,79 @@ 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,
+					 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:
+		pr_info("%s: some properties deliberately report errors.\n",
+			__func__);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int power_supply_ext_set_property(struct power_supply *psy,
+					 const struct power_supply_ext *ext,
+					 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,
+						  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)) {
+			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 +332,8 @@ static int __init test_power_init(void)
 		}
 	}
 
+	test_battery_configure_battery_hook(true);
+
 	module_initialized = true;
 	return 0;
 failed:
@@ -524,6 +600,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 +666,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 +681,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 +719,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.45.2

[PATCH RFC v2 5/5] platform/x86: system76: Use power_supply extension API

[Thomas Weißschuh]

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/platform/x86/system76_acpi.c | 93 ++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 42 deletions(-)

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 3da753b3d00d..d9bc5cae2dda 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -162,7 +162,7 @@ enum {
 	THRESHOLD_END,
 };
 
-static ssize_t battery_get_threshold(int which, char *buf)
+static int battery_get_threshold(int which, int *val)
 {
 	struct acpi_object_list input;
 	union acpi_object param;
@@ -186,29 +186,21 @@ static ssize_t battery_get_threshold(int which, char *buf)
 	if (ret == BATTERY_THRESHOLD_INVALID)
 		return -EINVAL;
 
-	return sysfs_emit(buf, "%d\n", (int)ret);
+	*val = ret;
+	return 0;
 }
 
-static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
+static int battery_set_threshold(int which, unsigned int value)
 {
 	struct acpi_object_list input;
 	union acpi_object params[2];
 	acpi_handle handle;
 	acpi_status status;
-	unsigned int value;
-	int ret;
 
 	handle = ec_get_handle();
 	if (!handle)
 		return -ENODEV;
 
-	ret = kstrtouint(buf, 10, &value);
-	if (ret)
-		return ret;
-
-	if (value > 100)
-		return -EINVAL;
-
 	input.count = 2;
 	input.pointer = params;
 	// Start/stop selection
@@ -222,52 +214,69 @@ static ssize_t battery_set_threshold(int which, const char *buf, size_t count)
 	if (ACPI_FAILURE(status))
 		return -EIO;
 
-	return count;
-}
-
-static ssize_t charge_control_start_threshold_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	return battery_get_threshold(THRESHOLD_START, buf);
-}
-
-static ssize_t charge_control_start_threshold_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
-{
-	return battery_set_threshold(THRESHOLD_START, buf, count);
+	return 0;
 }
 
-static DEVICE_ATTR_RW(charge_control_start_threshold);
+static const enum power_supply_property system76_battery_properties[] = {
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD,
+	POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD,
+};
 
-static ssize_t charge_control_end_threshold_show(struct device *dev,
-	struct device_attribute *attr, char *buf)
+static int system76_property_is_writeable(struct power_supply *psy,
+					  const struct power_supply_ext *ext,
+					  enum power_supply_property psp)
 {
-	return battery_get_threshold(THRESHOLD_END, buf);
+	return true;
 }
 
-static ssize_t charge_control_end_threshold_store(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t count)
+static int system76_get_property(struct power_supply *psy,
+				 const struct power_supply_ext *ext,
+				 enum power_supply_property psp,
+				 union power_supply_propval *val)
 {
-	return battery_set_threshold(THRESHOLD_END, buf, count);
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+		return battery_get_threshold(THRESHOLD_START, &val->intval);
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		return battery_get_threshold(THRESHOLD_END, &val->intval);
+	default:
+		return -EINVAL;
+	};
+}
+
+static int system76_set_property(struct power_supply *psy, const struct power_supply_ext *ext,
+				 enum power_supply_property psp,
+				 const union power_supply_propval *val)
+{
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD:
+		if (val->intval < 0 || val->intval > 100)
+			return -EINVAL;
+		return battery_set_threshold(THRESHOLD_START, val->intval);
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD:
+		if (val->intval < 0 || val->intval > 100)
+			return -EINVAL;
+		return battery_set_threshold(THRESHOLD_END, val->intval);
+	default:
+		return -EINVAL;
+	};
 }
 
-static DEVICE_ATTR_RW(charge_control_end_threshold);
-
-static struct attribute *system76_battery_attrs[] = {
-	&dev_attr_charge_control_start_threshold.attr,
-	&dev_attr_charge_control_end_threshold.attr,
-	NULL,
+static const struct power_supply_ext system76_power_supply_ext = {
+	.properties            = system76_battery_properties,
+	.num_properties        = ARRAY_SIZE(system76_battery_properties),
+	.property_is_writeable = system76_property_is_writeable,
+	.get_property          = system76_get_property,
+	.set_property          = system76_set_property,
 };
 
-ATTRIBUTE_GROUPS(system76_battery);
-
 static int system76_battery_add(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
 	// System76 EC only supports 1 battery
 	if (strcmp(battery->desc->name, "BAT0") != 0)
 		return -ENODEV;
 
-	if (device_add_groups(&battery->dev, system76_battery_groups))
+	if (power_supply_register_extension(battery, &system76_power_supply_ext))
 		return -ENODEV;
 
 	return 0;
@@ -275,7 +284,7 @@ static int system76_battery_add(struct power_supply *battery, struct acpi_batter
 
 static int system76_battery_remove(struct power_supply *battery, struct acpi_battery_hook *hook)
 {
-	device_remove_groups(&battery->dev, system76_battery_groups);
+	power_supply_unregister_extension(battery, &system76_power_supply_ext);
 	return 0;
 }
 

-- 
2.45.2

Re: (subset) [PATCH RFC v2 0/5] power: supply: extension API

[Sebastian Reichel]

On Sat, 08 Jun 2024 21:19:36 +0200, 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/5] power: supply: sysfs: use power_supply_property_is_writeable()
      commit: be6299c6e55e971ffc060495708740a59aa0e45b

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