Re: [PATCH v5 4/4] power: supply: cros_charge-control: implement a power supply extension

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Thu, Dec 05, 2024 at 09:46:38PM +0100, Thomas Weißschuh wrote:
> 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@xxxxxxxxxxxxxx>
> ---

LGTM.

Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>

-- Sebastian

>  drivers/power/supply/cros_charge-control.c | 217 ++++++++++++-----------------
>  1 file changed, 88 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
> index 17c53591ce197d08d97c94d3d4359a282026dd7d..7f914a65f88f8ea57bf01c334d25d9b8ebaa2de7 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;
> +	const struct power_supply_ext *psy_ext;
>  
>  	enum power_supply_charge_behaviour current_behaviour;
>  	u8 current_start_threshold, current_end_threshold;
> @@ -114,124 +101,104 @@ 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 for cros_chctl_psy_ext_v1 */
> +	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)
> -{
> -	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)
> +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_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);
> +	return true;
> +}
>  
> -	if (priv->cmd_version < 2) {
> -		if (n == CROS_CHCTL_ATTR_START_THRESHOLD)
> -			return 0;
> -		if (n == CROS_CHCTL_ATTR_END_THRESHOLD)
> -			return 0;
> -	}
> +static const struct power_supply_ext cros_chctl_psy_ext = {
> +	.properties = cros_chctl_psy_ext_props,
> +	.num_properties = ARRAY_SIZE(cros_chctl_psy_ext_props),
> +	.charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS,
> +	.get_property = cros_chctl_psy_ext_get_prop,
> +	.set_property = cros_chctl_psy_ext_set_prop,
> +	.property_is_writeable = cros_chctl_psy_prop_is_writeable,
> +};
>  
> -	return attr->mode;
> -}
> +static const struct power_supply_ext cros_chctl_psy_ext_v1 = {
> +	.properties = cros_chctl_psy_ext_props,
> +	.num_properties = 1,
> +	.charge_behaviours = EC_CHARGE_CONTROL_BEHAVIOURS,
> +	.get_property = cros_chctl_psy_ext_get_prop,
> +	.set_property = cros_chctl_psy_ext_set_prop,
> +	.property_is_writeable = cros_chctl_psy_prop_is_writeable,
> +};
>  
>  static int cros_chctl_add_battery(struct power_supply *battery, struct acpi_battery_hook *hook)
>  {
> @@ -241,7 +208,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 +216,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 +242,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,18 +271,11 @@ 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;
> +
> +	if (priv->cmd_version == 1)
> +		priv->psy_ext = &cros_chctl_psy_ext_v1;
> +	else
> +		priv->psy_ext = &cros_chctl_psy_ext;
>  
>  	priv->battery_hook.name = dev_name(dev);
>  	priv->battery_hook.add_battery = cros_chctl_add_battery;
> 
> -- 
> 2.47.1
> 
> 

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux