Message-ID: <vq7r2udbmqfp5v7o2occtnoziwrnpidht2wgkofvfyfic2o67m@3wtnxxvtdgye>
Date: Sat, 14 Sep 2024 12:50:57 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Armin Wolf <W_Armin@....de>, Hans de Goede <hdegoede@...hat.com>, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v3 6/9] power: supply: core: implement extension API

Hi,

On Wed, Sep 04, 2024 at 09:25:39PM GMT, Thomas Weißschuh wrote:
> Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> ---

missing long description :)

In general I like the concept. It looks like a good candidate to
clean up the mess with the custom registration of properties done
by the x86 platform code.

Apart from ACPI with custom battery extensions mess, there is
something similar in the embedded world: Many charger chips have
rudimentary fuel-gauge support. Some drivers register a charger/usb
and a battery device. That results in two battery devices when the
machine also has a proper fuel gauge. Having two battery devices
exposed is bad, since that means the machine has two batteries
(like some Thinkpads). Not exposing the battery from the charger
device is possibly loosing information (depending on fuel gauge
features).

I think it should be possible to register the charger's rudimentary
battery device as a power_supply_ext, if it detects a proper fuel
gauge being available (e.g. via the DT "power-supplies" property).
But then the charger's properties are the preferred ones. I think
they should be fallback properties instead. A machine only gets its
own fuel-gauge, when that provides better values than the data
available from the charger :)

So the question is which property getter/setter should be used
when the same property is exposed multiple times. Your examples
only add new properties and does not run into this. The intuitive
thing would be for properties from extensions to be preferred,
but do we actually have a use case for that?

Greetings,

-- Sebastian

>  drivers/power/supply/power_supply.h       |  12 ++++
>  drivers/power/supply/power_supply_core.c  | 114 +++++++++++++++++++++++++++++-
>  drivers/power/supply/power_supply_sysfs.c |  20 +++++-
>  include/linux/power_supply.h              |  30 ++++++++
>  4 files changed, 171 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index b01faeaf7827..fb45f0717bd1 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -15,6 +15,18 @@ struct power_supply;
>  
>  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) \
> +	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 dcb7e4853030..db5e7af57e67 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1199,15 +1199,40 @@ 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;
> +
> +	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(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;
>  }
>  
> @@ -1215,12 +1240,19 @@ 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;
>  	}
>  
> +	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))
> @@ -1234,7 +1266,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 power_supply_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	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);
> @@ -1244,8 +1290,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 power_supply_ext_registration *reg;
> +
> +	if (atomic_read(&psy->use_cnt) <= 0)
> +		return -ENODEV;
> +
> +	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);
> @@ -1268,6 +1327,54 @@ 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,
> +				    void *ext_data)
> +{
> +	struct power_supply_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 = kmalloc(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 power_supply_ext_registration *reg;
> +
> +	power_supply_for_each_extension(reg, psy) {
> +		if (reg->ext == ext) {
> +			list_del(&reg->list_head);
> +			kfree(reg);
> +			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);
> @@ -1427,6 +1534,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_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 915a4ba62258..e7c306afd846 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 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_show_property(struct device *dev,
>  					  struct device_attribute *attr,
>  					  char *buf) {
> @@ -306,8 +323,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);
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 72dc7e45c90c..51788faf1d66 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,28 @@ 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 *ext_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 *ext_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 *ext_data,
> +				     enum power_supply_property psp);
> +};
> +
>  struct power_supply {
>  	const struct power_supply_desc *desc;
>  
> @@ -303,6 +326,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;
> @@ -887,6 +911,12 @@ 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,
> +					   void *ext_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.46.0
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists