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

From: Sebastian Reichel
Date: Thu Dec 05 2024 - 20:16:12 EST

Next message: kernel test robot: "fs/bcachefs/reflink.c:37:5-8: Unneeded variable: "ret". Return "0" on line 44"
Previous message: kernel test robot: "drivers/md/bcache/super.c:108:14: sparse: sparse: restricted __le64 degrades to integer"
In reply to: Thomas Weißschuh: "[PATCH v5 4/4] power: supply: cros_charge-control: implement a power supply extension"
Next in thread: Thomas Weißschuh: "[PATCH v5 3/4] power: supply: test-power: implement a power supply extension"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: kernel test robot: "fs/bcachefs/reflink.c:37:5-8: Unneeded variable: "ret". Return "0" on line 44"
Previous message: kernel test robot: "drivers/md/bcache/super.c:108:14: sparse: sparse: restricted __le64 degrades to integer"
In reply to: Thomas Weißschuh: "[PATCH v5 4/4] power: supply: cros_charge-control: implement a power supply extension"
Next in thread: Thomas Weißschuh: "[PATCH v5 3/4] power: supply: test-power: implement a power supply extension"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]