[PATCH 0/4] Cleanup power_supply_sysfs.c

[PATCH 0/4] Cleanup power_supply_sysfs.c

[Mathew King]

A few various patches to cleanup the power_supply sysfs implementation.

Mathew King (4):
  power_supply: Cleanup power supply sysfs attribute list
  power_supply: Use designated initializer for property text arrays
  power_supply: Add a macro that maps enum properties to text values
  power_supply: Add power supply type property to uevent env

 drivers/power/supply/power_supply.h       |   7 +-
 drivers/power/supply/power_supply_core.c  |   9 +-
 drivers/power/supply/power_supply_sysfs.c | 507 +++++++++++++---------
 3 files changed, 305 insertions(+), 218 deletions(-)

-- 
2.26.2.303.gf8c07b1a785-goog

[PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list

[Mathew King]

Make the device attribute list used to create sysfs attributes more
robust by decoupling the list order from order of the enum defined in
power_supply.h. This is done by using a designated initializer in the
POWER_SUPPLY_ATTR macro. Also properties not added to the list will
still be created with name in the format "prop_*".

Signed-off-by: Mathew King <mathewk@chromium.org>
---
 drivers/power/supply/power_supply.h       |   7 +-
 drivers/power/supply/power_supply_core.c  |   9 +-
 drivers/power/supply/power_supply_sysfs.c | 290 +++++++++++++---------
 3 files changed, 179 insertions(+), 127 deletions(-)

diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
index c310d4f36c10..e3475d35ec2a 100644
--- a/drivers/power/supply/power_supply.h
+++ b/drivers/power/supply/power_supply.h
@@ -15,12 +15,15 @@ struct power_supply;
 
 #ifdef CONFIG_SYSFS
 
-extern void power_supply_init_attrs(struct device_type *dev_type);
+extern int power_supply_init_attrs(struct device_type *dev_type);
+extern void power_supply_destroy_attrs(void);
 extern int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env);
 
 #else
 
-static inline void power_supply_init_attrs(struct device_type *dev_type) {}
+static inline int power_supply_init_attrs(struct device_type *dev_type)
+{ return 0; }
+static void power_supply_destroy_attrs(void) {}
 #define power_supply_uevent NULL
 
 #endif /* CONFIG_SYSFS */
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 1a9a9fae73d3..7fb99302b532 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -1329,19 +1329,26 @@ EXPORT_SYMBOL_GPL(power_supply_get_drvdata);
 
 static int __init power_supply_class_init(void)
 {
+	int ret;
 	power_supply_class = class_create(THIS_MODULE, "power_supply");
 
 	if (IS_ERR(power_supply_class))
 		return PTR_ERR(power_supply_class);
 
 	power_supply_class->dev_uevent = power_supply_uevent;
-	power_supply_init_attrs(&power_supply_dev_type);
+
+	ret = power_supply_init_attrs(&power_supply_dev_type);
+	if (ret) {
+		class_destroy(power_supply_class);
+		return ret;
+	}
 
 	return 0;
 }
 
 static void __exit power_supply_class_exit(void)
 {
+	power_supply_destroy_attrs();
 	class_destroy(power_supply_class);
 }
 
diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index f37ad4eae60b..b579081599d7 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -18,27 +18,22 @@
 
 #include "power_supply.h"
 
-/*
- * This is because the name "current" breaks the device attr macro.
- * The "current" word resolves to "(get_current())" so instead of
- * "current" "(get_current())" appears in the sysfs.
- *
- * The source of this definition is the device.h which calls __ATTR
- * macro in sysfs.h which calls the __stringify macro.
- *
- * Only modification that the name is not tried to be resolved
- * (as a macro let's say).
- */
+#define UNDEF_PROP_NAME_FMT "PROP_%d"
+#define UNDEF_PROP_NAME_LEN (strlen(UNDEF_PROP_NAME_FMT) + 2)
+
+struct power_supply_attr {
+	const char *prop_name;
+	const char *upper_name;
+	const char *lower_name;
+	struct device_attribute dev_attr;
+};
 
-#define POWER_SUPPLY_ATTR(_name)					\
-{									\
-	.attr = { .name = #_name },					\
-	.show = power_supply_show_property,				\
-	.store = power_supply_store_property,				\
+#define POWER_SUPPLY_ATTR(_name)	\
+[POWER_SUPPLY_PROP_ ## _name] =		\
+{					\
+	.prop_name = #_name		\
 }
 
-static struct device_attribute power_supply_attrs[];
-
 static const char * const power_supply_type_text[] = {
 	"Unknown", "Battery", "UPS", "Mains", "USB",
 	"USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
@@ -77,6 +72,91 @@ static const char * const power_supply_scope_text[] = {
 	"Unknown", "System", "Device"
 };
 
+static struct power_supply_attr power_supply_attrs[] = {
+	/* Properties of type `int' */
+	POWER_SUPPLY_ATTR(STATUS),
+	POWER_SUPPLY_ATTR(CHARGE_TYPE),
+	POWER_SUPPLY_ATTR(HEALTH),
+	POWER_SUPPLY_ATTR(PRESENT),
+	POWER_SUPPLY_ATTR(ONLINE),
+	POWER_SUPPLY_ATTR(AUTHENTIC),
+	POWER_SUPPLY_ATTR(TECHNOLOGY),
+	POWER_SUPPLY_ATTR(CYCLE_COUNT),
+	POWER_SUPPLY_ATTR(VOLTAGE_MAX),
+	POWER_SUPPLY_ATTR(VOLTAGE_MIN),
+	POWER_SUPPLY_ATTR(VOLTAGE_MAX_DESIGN),
+	POWER_SUPPLY_ATTR(VOLTAGE_MIN_DESIGN),
+	POWER_SUPPLY_ATTR(VOLTAGE_NOW),
+	POWER_SUPPLY_ATTR(VOLTAGE_AVG),
+	POWER_SUPPLY_ATTR(VOLTAGE_OCV),
+	POWER_SUPPLY_ATTR(VOLTAGE_BOOT),
+	POWER_SUPPLY_ATTR(CURRENT_MAX),
+	POWER_SUPPLY_ATTR(CURRENT_NOW),
+	POWER_SUPPLY_ATTR(CURRENT_AVG),
+	POWER_SUPPLY_ATTR(CURRENT_BOOT),
+	POWER_SUPPLY_ATTR(POWER_NOW),
+	POWER_SUPPLY_ATTR(POWER_AVG),
+	POWER_SUPPLY_ATTR(CHARGE_FULL_DESIGN),
+	POWER_SUPPLY_ATTR(CHARGE_EMPTY_DESIGN),
+	POWER_SUPPLY_ATTR(CHARGE_FULL),
+	POWER_SUPPLY_ATTR(CHARGE_EMPTY),
+	POWER_SUPPLY_ATTR(CHARGE_NOW),
+	POWER_SUPPLY_ATTR(CHARGE_AVG),
+	POWER_SUPPLY_ATTR(CHARGE_COUNTER),
+	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT),
+	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT_MAX),
+	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE),
+	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE_MAX),
+	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT),
+	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
+	POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
+	POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
+	POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
+	POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
+	POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
+	POWER_SUPPLY_ATTR(ENERGY_FULL_DESIGN),
+	POWER_SUPPLY_ATTR(ENERGY_EMPTY_DESIGN),
+	POWER_SUPPLY_ATTR(ENERGY_FULL),
+	POWER_SUPPLY_ATTR(ENERGY_EMPTY),
+	POWER_SUPPLY_ATTR(ENERGY_NOW),
+	POWER_SUPPLY_ATTR(ENERGY_AVG),
+	POWER_SUPPLY_ATTR(CAPACITY),
+	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MIN),
+	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MAX),
+	POWER_SUPPLY_ATTR(CAPACITY_LEVEL),
+	POWER_SUPPLY_ATTR(TEMP),
+	POWER_SUPPLY_ATTR(TEMP_MAX),
+	POWER_SUPPLY_ATTR(TEMP_MIN),
+	POWER_SUPPLY_ATTR(TEMP_ALERT_MIN),
+	POWER_SUPPLY_ATTR(TEMP_ALERT_MAX),
+	POWER_SUPPLY_ATTR(TEMP_AMBIENT),
+	POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MIN),
+	POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MAX),
+	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_NOW),
+	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_AVG),
+	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
+	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
+	POWER_SUPPLY_ATTR(TYPE),
+	POWER_SUPPLY_ATTR(USB_TYPE),
+	POWER_SUPPLY_ATTR(SCOPE),
+	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
+	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
+	POWER_SUPPLY_ATTR(CALIBRATE),
+	/* Properties of type `const char *' */
+	POWER_SUPPLY_ATTR(MODEL_NAME),
+	POWER_SUPPLY_ATTR(MANUFACTURER),
+	POWER_SUPPLY_ATTR(SERIAL_NUMBER),
+};
+
+static struct attribute *
+__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
+
+static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
+{
+	return container_of(attr, struct power_supply_attr, dev_attr) -
+		power_supply_attrs;
+}
+
 static ssize_t power_supply_show_usb_type(struct device *dev,
 					  enum power_supply_usb_type *usb_types,
 					  ssize_t num_usb_types,
@@ -117,7 +197,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 					  char *buf) {
 	ssize_t ret;
 	struct power_supply *psy = dev_get_drvdata(dev);
-	enum power_supply_property psp = attr - power_supply_attrs;
+	enum power_supply_property psp = dev_attr_psp(attr);
 	union power_supply_propval value;
 
 	if (psp == POWER_SUPPLY_PROP_TYPE) {
@@ -186,7 +266,7 @@ static ssize_t power_supply_store_property(struct device *dev,
 					   const char *buf, size_t count) {
 	ssize_t ret;
 	struct power_supply *psy = dev_get_drvdata(dev);
-	enum power_supply_property psp = attr - power_supply_attrs;
+	enum power_supply_property psp = dev_attr_psp(attr);
 	union power_supply_propval value;
 
 	switch (psp) {
@@ -235,86 +315,6 @@ static ssize_t power_supply_store_property(struct device *dev,
 	return count;
 }
 
-/* Must be in the same order as POWER_SUPPLY_PROP_* */
-static struct device_attribute power_supply_attrs[] = {
-	/* Properties of type `int' */
-	POWER_SUPPLY_ATTR(status),
-	POWER_SUPPLY_ATTR(charge_type),
-	POWER_SUPPLY_ATTR(health),
-	POWER_SUPPLY_ATTR(present),
-	POWER_SUPPLY_ATTR(online),
-	POWER_SUPPLY_ATTR(authentic),
-	POWER_SUPPLY_ATTR(technology),
-	POWER_SUPPLY_ATTR(cycle_count),
-	POWER_SUPPLY_ATTR(voltage_max),
-	POWER_SUPPLY_ATTR(voltage_min),
-	POWER_SUPPLY_ATTR(voltage_max_design),
-	POWER_SUPPLY_ATTR(voltage_min_design),
-	POWER_SUPPLY_ATTR(voltage_now),
-	POWER_SUPPLY_ATTR(voltage_avg),
-	POWER_SUPPLY_ATTR(voltage_ocv),
-	POWER_SUPPLY_ATTR(voltage_boot),
-	POWER_SUPPLY_ATTR(current_max),
-	POWER_SUPPLY_ATTR(current_now),
-	POWER_SUPPLY_ATTR(current_avg),
-	POWER_SUPPLY_ATTR(current_boot),
-	POWER_SUPPLY_ATTR(power_now),
-	POWER_SUPPLY_ATTR(power_avg),
-	POWER_SUPPLY_ATTR(charge_full_design),
-	POWER_SUPPLY_ATTR(charge_empty_design),
-	POWER_SUPPLY_ATTR(charge_full),
-	POWER_SUPPLY_ATTR(charge_empty),
-	POWER_SUPPLY_ATTR(charge_now),
-	POWER_SUPPLY_ATTR(charge_avg),
-	POWER_SUPPLY_ATTR(charge_counter),
-	POWER_SUPPLY_ATTR(constant_charge_current),
-	POWER_SUPPLY_ATTR(constant_charge_current_max),
-	POWER_SUPPLY_ATTR(constant_charge_voltage),
-	POWER_SUPPLY_ATTR(constant_charge_voltage_max),
-	POWER_SUPPLY_ATTR(charge_control_limit),
-	POWER_SUPPLY_ATTR(charge_control_limit_max),
-	POWER_SUPPLY_ATTR(charge_control_start_threshold),
-	POWER_SUPPLY_ATTR(charge_control_end_threshold),
-	POWER_SUPPLY_ATTR(input_current_limit),
-	POWER_SUPPLY_ATTR(input_voltage_limit),
-	POWER_SUPPLY_ATTR(input_power_limit),
-	POWER_SUPPLY_ATTR(energy_full_design),
-	POWER_SUPPLY_ATTR(energy_empty_design),
-	POWER_SUPPLY_ATTR(energy_full),
-	POWER_SUPPLY_ATTR(energy_empty),
-	POWER_SUPPLY_ATTR(energy_now),
-	POWER_SUPPLY_ATTR(energy_avg),
-	POWER_SUPPLY_ATTR(capacity),
-	POWER_SUPPLY_ATTR(capacity_alert_min),
-	POWER_SUPPLY_ATTR(capacity_alert_max),
-	POWER_SUPPLY_ATTR(capacity_level),
-	POWER_SUPPLY_ATTR(temp),
-	POWER_SUPPLY_ATTR(temp_max),
-	POWER_SUPPLY_ATTR(temp_min),
-	POWER_SUPPLY_ATTR(temp_alert_min),
-	POWER_SUPPLY_ATTR(temp_alert_max),
-	POWER_SUPPLY_ATTR(temp_ambient),
-	POWER_SUPPLY_ATTR(temp_ambient_alert_min),
-	POWER_SUPPLY_ATTR(temp_ambient_alert_max),
-	POWER_SUPPLY_ATTR(time_to_empty_now),
-	POWER_SUPPLY_ATTR(time_to_empty_avg),
-	POWER_SUPPLY_ATTR(time_to_full_now),
-	POWER_SUPPLY_ATTR(time_to_full_avg),
-	POWER_SUPPLY_ATTR(type),
-	POWER_SUPPLY_ATTR(usb_type),
-	POWER_SUPPLY_ATTR(scope),
-	POWER_SUPPLY_ATTR(precharge_current),
-	POWER_SUPPLY_ATTR(charge_term_current),
-	POWER_SUPPLY_ATTR(calibrate),
-	/* Properties of type `const char *' */
-	POWER_SUPPLY_ATTR(model_name),
-	POWER_SUPPLY_ATTR(manufacturer),
-	POWER_SUPPLY_ATTR(serial_number),
-};
-
-static struct attribute *
-__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
-
 static umode_t power_supply_attr_is_visible(struct kobject *kobj,
 					   struct attribute *attr,
 					   int attrno)
@@ -352,17 +352,7 @@ static const struct attribute_group *power_supply_attr_groups[] = {
 	NULL,
 };
 
-void power_supply_init_attrs(struct device_type *dev_type)
-{
-	int i;
-
-	dev_type->groups = power_supply_attr_groups;
-
-	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++)
-		__power_supply_attrs[i] = &power_supply_attrs[i].attr;
-}
-
-static char *kstruprdup(const char *str, gfp_t gfp)
+static char *kstrlwrdup(const char *str, gfp_t gfp)
 {
 	char *ret, *ustr;
 
@@ -372,19 +362,75 @@ static char *kstruprdup(const char *str, gfp_t gfp)
 		return NULL;
 
 	while (*str)
-		*ustr++ = toupper(*str++);
+		*ustr++ = tolower(*str++);
 
 	*ustr = 0;
 
 	return ret;
 }
 
+int power_supply_init_attrs(struct device_type *dev_type)
+{
+	int i;
+
+	dev_type->groups = power_supply_attr_groups;
+
+	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
+		const char *upper, *lower;
+		struct device_attribute *attr;
+
+		upper = power_supply_attrs[i].prop_name;
+
+		if (!upper) {
+			char *name = kmalloc(UNDEF_PROP_NAME_LEN, GFP_KERNEL);
+
+			if (!name)
+				goto err_no_mem;
+
+			sprintf(name, UNDEF_PROP_NAME_FMT, i);
+			upper = name;
+		}
+
+		lower = kstrlwrdup(upper, GFP_KERNEL);
+
+		if (!lower)
+			goto err_no_mem;
+
+		power_supply_attrs[i].upper_name = upper;
+		power_supply_attrs[i].lower_name = lower;
+
+		attr = &power_supply_attrs[i].dev_attr;
+
+		attr->attr.name = lower;
+		attr->show = power_supply_show_property;
+		attr->store = power_supply_store_property;
+		__power_supply_attrs[i] = &attr->attr;
+	}
+
+	return 0;
+
+err_no_mem:
+	power_supply_destroy_attrs();
+	return -ENOMEM;
+}
+
+void power_supply_destroy_attrs(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
+		kfree(power_supply_attrs[i].lower_name);
+
+		if (!power_supply_attrs[i].prop_name)
+			kfree(power_supply_attrs[i].upper_name);
+	}
+}
+
 int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct power_supply *psy = dev_get_drvdata(dev);
 	int ret = 0, j;
 	char *prop_buf;
-	char *attrname;
 
 	if (!psy || !psy->desc) {
 		dev_dbg(dev, "No power supply yet\n");
@@ -400,12 +446,14 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 		return -ENOMEM;
 
 	for (j = 0; j < psy->desc->num_properties; j++) {
-		struct device_attribute *attr;
+		struct power_supply_attr *pwr_attr;
+		struct device_attribute *dev_attr;
 		char *line;
 
-		attr = &power_supply_attrs[psy->desc->properties[j]];
+		pwr_attr = &power_supply_attrs[psy->desc->properties[j]];
+		dev_attr = &pwr_attr->dev_attr;
 
-		ret = power_supply_show_property(dev, attr, prop_buf);
+		ret = power_supply_show_property(dev, dev_attr, prop_buf);
 		if (ret == -ENODEV || ret == -ENODATA) {
 			/* When a battery is absent, we expect -ENODEV. Don't abort;
 			   send the uevent with at least the the PRESENT=0 property */
@@ -420,14 +468,8 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 		if (line)
 			*line = 0;
 
-		attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
-		if (!attrname) {
-			ret = -ENOMEM;
-			goto out;
-		}
-
-		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s", attrname, prop_buf);
-		kfree(attrname);
+		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s",
+				     pwr_attr->upper_name, prop_buf);
 		if (ret)
 			goto out;
 	}
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list

[Sebastian Reichel]

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

Hi,

On Fri, Apr 24, 2020 at 11:35:30AM -0600, Mathew King wrote:
> Make the device attribute list used to create sysfs attributes more
> robust by decoupling the list order from order of the enum defined in
> power_supply.h. This is done by using a designated initializer in the
> POWER_SUPPLY_ATTR macro.

Thanks for doing this :)

> Also properties not added to the list will still be created with
> name in the format "prop_*".

This is bad. We don't want to expose PROP_%d to userspace and risk
this becoming ABI. Instead it should not be registered and a warning
should be printed. This means we can drop the prop_name field and directly
use upper, which will always be in TEXT segment then.

For the lower case name, I think it's better to provide it as second
argument to POWER_SUPPLY_ATTR. Not very elegant solution, but it
means, that the string is in the TEXT segment and that we can
have the complete dev_attr initialized from the macro.

-- Sebastian

> Signed-off-by: Mathew King <mathewk@chromium.org>
> ---
>  drivers/power/supply/power_supply.h       |   7 +-
>  drivers/power/supply/power_supply_core.c  |   9 +-
>  drivers/power/supply/power_supply_sysfs.c | 290 +++++++++++++---------
>  3 files changed, 179 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> index c310d4f36c10..e3475d35ec2a 100644
> --- a/drivers/power/supply/power_supply.h
> +++ b/drivers/power/supply/power_supply.h
> @@ -15,12 +15,15 @@ struct power_supply;
>  
>  #ifdef CONFIG_SYSFS
>  
> -extern void power_supply_init_attrs(struct device_type *dev_type);
> +extern int power_supply_init_attrs(struct device_type *dev_type);
> +extern void power_supply_destroy_attrs(void);
>  extern int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env);
>  
>  #else
>  
> -static inline void power_supply_init_attrs(struct device_type *dev_type) {}
> +static inline int power_supply_init_attrs(struct device_type *dev_type)
> +{ return 0; }
> +static void power_supply_destroy_attrs(void) {}
>  #define power_supply_uevent NULL
>  
>  #endif /* CONFIG_SYSFS */
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 1a9a9fae73d3..7fb99302b532 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -1329,19 +1329,26 @@ EXPORT_SYMBOL_GPL(power_supply_get_drvdata);
>  
>  static int __init power_supply_class_init(void)
>  {
> +	int ret;
>  	power_supply_class = class_create(THIS_MODULE, "power_supply");
>  
>  	if (IS_ERR(power_supply_class))
>  		return PTR_ERR(power_supply_class);
>  
>  	power_supply_class->dev_uevent = power_supply_uevent;
> -	power_supply_init_attrs(&power_supply_dev_type);
> +
> +	ret = power_supply_init_attrs(&power_supply_dev_type);
> +	if (ret) {
> +		class_destroy(power_supply_class);
> +		return ret;
> +	}
>  
>  	return 0;
>  }
>  
>  static void __exit power_supply_class_exit(void)
>  {
> +	power_supply_destroy_attrs();
>  	class_destroy(power_supply_class);
>  }
>  
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index f37ad4eae60b..b579081599d7 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -18,27 +18,22 @@
>  
>  #include "power_supply.h"
>  
> -/*
> - * This is because the name "current" breaks the device attr macro.
> - * The "current" word resolves to "(get_current())" so instead of
> - * "current" "(get_current())" appears in the sysfs.
> - *
> - * The source of this definition is the device.h which calls __ATTR
> - * macro in sysfs.h which calls the __stringify macro.
> - *
> - * Only modification that the name is not tried to be resolved
> - * (as a macro let's say).
> - */
> +#define UNDEF_PROP_NAME_FMT "PROP_%d"
> +#define UNDEF_PROP_NAME_LEN (strlen(UNDEF_PROP_NAME_FMT) + 2)
> +
> +struct power_supply_attr {
> +	const char *prop_name;
> +	const char *upper_name;
> +	const char *lower_name;
> +	struct device_attribute dev_attr;
> +};
>  
> -#define POWER_SUPPLY_ATTR(_name)					\
> -{									\
> -	.attr = { .name = #_name },					\
> -	.show = power_supply_show_property,				\
> -	.store = power_supply_store_property,				\
> +#define POWER_SUPPLY_ATTR(_name)	\
> +[POWER_SUPPLY_PROP_ ## _name] =		\
> +{					\
> +	.prop_name = #_name		\
>  }
>  
> -static struct device_attribute power_supply_attrs[];
> -
>  static const char * const power_supply_type_text[] = {
>  	"Unknown", "Battery", "UPS", "Mains", "USB",
>  	"USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> @@ -77,6 +72,91 @@ static const char * const power_supply_scope_text[] = {
>  	"Unknown", "System", "Device"
>  };
>  
> +static struct power_supply_attr power_supply_attrs[] = {
> +	/* Properties of type `int' */
> +	POWER_SUPPLY_ATTR(STATUS),
> +	POWER_SUPPLY_ATTR(CHARGE_TYPE),
> +	POWER_SUPPLY_ATTR(HEALTH),
> +	POWER_SUPPLY_ATTR(PRESENT),
> +	POWER_SUPPLY_ATTR(ONLINE),
> +	POWER_SUPPLY_ATTR(AUTHENTIC),
> +	POWER_SUPPLY_ATTR(TECHNOLOGY),
> +	POWER_SUPPLY_ATTR(CYCLE_COUNT),
> +	POWER_SUPPLY_ATTR(VOLTAGE_MAX),
> +	POWER_SUPPLY_ATTR(VOLTAGE_MIN),
> +	POWER_SUPPLY_ATTR(VOLTAGE_MAX_DESIGN),
> +	POWER_SUPPLY_ATTR(VOLTAGE_MIN_DESIGN),
> +	POWER_SUPPLY_ATTR(VOLTAGE_NOW),
> +	POWER_SUPPLY_ATTR(VOLTAGE_AVG),
> +	POWER_SUPPLY_ATTR(VOLTAGE_OCV),
> +	POWER_SUPPLY_ATTR(VOLTAGE_BOOT),
> +	POWER_SUPPLY_ATTR(CURRENT_MAX),
> +	POWER_SUPPLY_ATTR(CURRENT_NOW),
> +	POWER_SUPPLY_ATTR(CURRENT_AVG),
> +	POWER_SUPPLY_ATTR(CURRENT_BOOT),
> +	POWER_SUPPLY_ATTR(POWER_NOW),
> +	POWER_SUPPLY_ATTR(POWER_AVG),
> +	POWER_SUPPLY_ATTR(CHARGE_FULL_DESIGN),
> +	POWER_SUPPLY_ATTR(CHARGE_EMPTY_DESIGN),
> +	POWER_SUPPLY_ATTR(CHARGE_FULL),
> +	POWER_SUPPLY_ATTR(CHARGE_EMPTY),
> +	POWER_SUPPLY_ATTR(CHARGE_NOW),
> +	POWER_SUPPLY_ATTR(CHARGE_AVG),
> +	POWER_SUPPLY_ATTR(CHARGE_COUNTER),
> +	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT),
> +	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT_MAX),
> +	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE),
> +	POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE_MAX),
> +	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT),
> +	POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
> +	POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
> +	POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> +	POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
> +	POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
> +	POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> +	POWER_SUPPLY_ATTR(ENERGY_FULL_DESIGN),
> +	POWER_SUPPLY_ATTR(ENERGY_EMPTY_DESIGN),
> +	POWER_SUPPLY_ATTR(ENERGY_FULL),
> +	POWER_SUPPLY_ATTR(ENERGY_EMPTY),
> +	POWER_SUPPLY_ATTR(ENERGY_NOW),
> +	POWER_SUPPLY_ATTR(ENERGY_AVG),
> +	POWER_SUPPLY_ATTR(CAPACITY),
> +	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MIN),
> +	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MAX),
> +	POWER_SUPPLY_ATTR(CAPACITY_LEVEL),
> +	POWER_SUPPLY_ATTR(TEMP),
> +	POWER_SUPPLY_ATTR(TEMP_MAX),
> +	POWER_SUPPLY_ATTR(TEMP_MIN),
> +	POWER_SUPPLY_ATTR(TEMP_ALERT_MIN),
> +	POWER_SUPPLY_ATTR(TEMP_ALERT_MAX),
> +	POWER_SUPPLY_ATTR(TEMP_AMBIENT),
> +	POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MIN),
> +	POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MAX),
> +	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_NOW),
> +	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_AVG),
> +	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
> +	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
> +	POWER_SUPPLY_ATTR(TYPE),
> +	POWER_SUPPLY_ATTR(USB_TYPE),
> +	POWER_SUPPLY_ATTR(SCOPE),
> +	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
> +	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
> +	POWER_SUPPLY_ATTR(CALIBRATE),
> +	/* Properties of type `const char *' */
> +	POWER_SUPPLY_ATTR(MODEL_NAME),
> +	POWER_SUPPLY_ATTR(MANUFACTURER),
> +	POWER_SUPPLY_ATTR(SERIAL_NUMBER),
> +};
> +
> +static struct attribute *
> +__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
> +
> +static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
> +{
> +	return container_of(attr, struct power_supply_attr, dev_attr) -
> +		power_supply_attrs;
> +}
> +
>  static ssize_t power_supply_show_usb_type(struct device *dev,
>  					  enum power_supply_usb_type *usb_types,
>  					  ssize_t num_usb_types,
> @@ -117,7 +197,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>  					  char *buf) {
>  	ssize_t ret;
>  	struct power_supply *psy = dev_get_drvdata(dev);
> -	enum power_supply_property psp = attr - power_supply_attrs;
> +	enum power_supply_property psp = dev_attr_psp(attr);
>  	union power_supply_propval value;
>  
>  	if (psp == POWER_SUPPLY_PROP_TYPE) {
> @@ -186,7 +266,7 @@ static ssize_t power_supply_store_property(struct device *dev,
>  					   const char *buf, size_t count) {
>  	ssize_t ret;
>  	struct power_supply *psy = dev_get_drvdata(dev);
> -	enum power_supply_property psp = attr - power_supply_attrs;
> +	enum power_supply_property psp = dev_attr_psp(attr);
>  	union power_supply_propval value;
>  
>  	switch (psp) {
> @@ -235,86 +315,6 @@ static ssize_t power_supply_store_property(struct device *dev,
>  	return count;
>  }
>  
> -/* Must be in the same order as POWER_SUPPLY_PROP_* */
> -static struct device_attribute power_supply_attrs[] = {
> -	/* Properties of type `int' */
> -	POWER_SUPPLY_ATTR(status),
> -	POWER_SUPPLY_ATTR(charge_type),
> -	POWER_SUPPLY_ATTR(health),
> -	POWER_SUPPLY_ATTR(present),
> -	POWER_SUPPLY_ATTR(online),
> -	POWER_SUPPLY_ATTR(authentic),
> -	POWER_SUPPLY_ATTR(technology),
> -	POWER_SUPPLY_ATTR(cycle_count),
> -	POWER_SUPPLY_ATTR(voltage_max),
> -	POWER_SUPPLY_ATTR(voltage_min),
> -	POWER_SUPPLY_ATTR(voltage_max_design),
> -	POWER_SUPPLY_ATTR(voltage_min_design),
> -	POWER_SUPPLY_ATTR(voltage_now),
> -	POWER_SUPPLY_ATTR(voltage_avg),
> -	POWER_SUPPLY_ATTR(voltage_ocv),
> -	POWER_SUPPLY_ATTR(voltage_boot),
> -	POWER_SUPPLY_ATTR(current_max),
> -	POWER_SUPPLY_ATTR(current_now),
> -	POWER_SUPPLY_ATTR(current_avg),
> -	POWER_SUPPLY_ATTR(current_boot),
> -	POWER_SUPPLY_ATTR(power_now),
> -	POWER_SUPPLY_ATTR(power_avg),
> -	POWER_SUPPLY_ATTR(charge_full_design),
> -	POWER_SUPPLY_ATTR(charge_empty_design),
> -	POWER_SUPPLY_ATTR(charge_full),
> -	POWER_SUPPLY_ATTR(charge_empty),
> -	POWER_SUPPLY_ATTR(charge_now),
> -	POWER_SUPPLY_ATTR(charge_avg),
> -	POWER_SUPPLY_ATTR(charge_counter),
> -	POWER_SUPPLY_ATTR(constant_charge_current),
> -	POWER_SUPPLY_ATTR(constant_charge_current_max),
> -	POWER_SUPPLY_ATTR(constant_charge_voltage),
> -	POWER_SUPPLY_ATTR(constant_charge_voltage_max),
> -	POWER_SUPPLY_ATTR(charge_control_limit),
> -	POWER_SUPPLY_ATTR(charge_control_limit_max),
> -	POWER_SUPPLY_ATTR(charge_control_start_threshold),
> -	POWER_SUPPLY_ATTR(charge_control_end_threshold),
> -	POWER_SUPPLY_ATTR(input_current_limit),
> -	POWER_SUPPLY_ATTR(input_voltage_limit),
> -	POWER_SUPPLY_ATTR(input_power_limit),
> -	POWER_SUPPLY_ATTR(energy_full_design),
> -	POWER_SUPPLY_ATTR(energy_empty_design),
> -	POWER_SUPPLY_ATTR(energy_full),
> -	POWER_SUPPLY_ATTR(energy_empty),
> -	POWER_SUPPLY_ATTR(energy_now),
> -	POWER_SUPPLY_ATTR(energy_avg),
> -	POWER_SUPPLY_ATTR(capacity),
> -	POWER_SUPPLY_ATTR(capacity_alert_min),
> -	POWER_SUPPLY_ATTR(capacity_alert_max),
> -	POWER_SUPPLY_ATTR(capacity_level),
> -	POWER_SUPPLY_ATTR(temp),
> -	POWER_SUPPLY_ATTR(temp_max),
> -	POWER_SUPPLY_ATTR(temp_min),
> -	POWER_SUPPLY_ATTR(temp_alert_min),
> -	POWER_SUPPLY_ATTR(temp_alert_max),
> -	POWER_SUPPLY_ATTR(temp_ambient),
> -	POWER_SUPPLY_ATTR(temp_ambient_alert_min),
> -	POWER_SUPPLY_ATTR(temp_ambient_alert_max),
> -	POWER_SUPPLY_ATTR(time_to_empty_now),
> -	POWER_SUPPLY_ATTR(time_to_empty_avg),
> -	POWER_SUPPLY_ATTR(time_to_full_now),
> -	POWER_SUPPLY_ATTR(time_to_full_avg),
> -	POWER_SUPPLY_ATTR(type),
> -	POWER_SUPPLY_ATTR(usb_type),
> -	POWER_SUPPLY_ATTR(scope),
> -	POWER_SUPPLY_ATTR(precharge_current),
> -	POWER_SUPPLY_ATTR(charge_term_current),
> -	POWER_SUPPLY_ATTR(calibrate),
> -	/* Properties of type `const char *' */
> -	POWER_SUPPLY_ATTR(model_name),
> -	POWER_SUPPLY_ATTR(manufacturer),
> -	POWER_SUPPLY_ATTR(serial_number),
> -};
> -
> -static struct attribute *
> -__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
> -
>  static umode_t power_supply_attr_is_visible(struct kobject *kobj,
>  					   struct attribute *attr,
>  					   int attrno)
> @@ -352,17 +352,7 @@ static const struct attribute_group *power_supply_attr_groups[] = {
>  	NULL,
>  };
>  
> -void power_supply_init_attrs(struct device_type *dev_type)
> -{
> -	int i;
> -
> -	dev_type->groups = power_supply_attr_groups;
> -
> -	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++)
> -		__power_supply_attrs[i] = &power_supply_attrs[i].attr;
> -}
> -
> -static char *kstruprdup(const char *str, gfp_t gfp)
> +static char *kstrlwrdup(const char *str, gfp_t gfp)
>  {
>  	char *ret, *ustr;
>  
> @@ -372,19 +362,75 @@ static char *kstruprdup(const char *str, gfp_t gfp)
>  		return NULL;
>  
>  	while (*str)
> -		*ustr++ = toupper(*str++);
> +		*ustr++ = tolower(*str++);
>  
>  	*ustr = 0;
>  
>  	return ret;
>  }
>  
> +int power_supply_init_attrs(struct device_type *dev_type)
> +{
> +	int i;
> +
> +	dev_type->groups = power_supply_attr_groups;
> +
> +	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> +		const char *upper, *lower;
> +		struct device_attribute *attr;
> +
> +		upper = power_supply_attrs[i].prop_name;
> +
> +		if (!upper) {
> +			char *name = kmalloc(UNDEF_PROP_NAME_LEN, GFP_KERNEL);
> +
> +			if (!name)
> +				goto err_no_mem;
> +
> +			sprintf(name, UNDEF_PROP_NAME_FMT, i);
> +			upper = name;
> +		}
> +
> +		lower = kstrlwrdup(upper, GFP_KERNEL);
> +
> +		if (!lower)
> +			goto err_no_mem;
> +
> +		power_supply_attrs[i].upper_name = upper;
> +		power_supply_attrs[i].lower_name = lower;
> +
> +		attr = &power_supply_attrs[i].dev_attr;
> +
> +		attr->attr.name = lower;
> +		attr->show = power_supply_show_property;
> +		attr->store = power_supply_store_property;
> +		__power_supply_attrs[i] = &attr->attr;
> +	}
> +
> +	return 0;
> +
> +err_no_mem:
> +	power_supply_destroy_attrs();
> +	return -ENOMEM;
> +}
> +
> +void power_supply_destroy_attrs(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> +		kfree(power_supply_attrs[i].lower_name);
> +
> +		if (!power_supply_attrs[i].prop_name)
> +			kfree(power_supply_attrs[i].upper_name);
> +	}
> +}
> +
>  int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	int ret = 0, j;
>  	char *prop_buf;
> -	char *attrname;
>  
>  	if (!psy || !psy->desc) {
>  		dev_dbg(dev, "No power supply yet\n");
> @@ -400,12 +446,14 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
>  		return -ENOMEM;
>  
>  	for (j = 0; j < psy->desc->num_properties; j++) {
> -		struct device_attribute *attr;
> +		struct power_supply_attr *pwr_attr;
> +		struct device_attribute *dev_attr;
>  		char *line;
>  
> -		attr = &power_supply_attrs[psy->desc->properties[j]];
> +		pwr_attr = &power_supply_attrs[psy->desc->properties[j]];
> +		dev_attr = &pwr_attr->dev_attr;
>  
> -		ret = power_supply_show_property(dev, attr, prop_buf);
> +		ret = power_supply_show_property(dev, dev_attr, prop_buf);
>  		if (ret == -ENODEV || ret == -ENODATA) {
>  			/* When a battery is absent, we expect -ENODEV. Don't abort;
>  			   send the uevent with at least the the PRESENT=0 property */
> @@ -420,14 +468,8 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
>  		if (line)
>  			*line = 0;
>  
> -		attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
> -		if (!attrname) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> -
> -		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s", attrname, prop_buf);
> -		kfree(attrname);
> +		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s",
> +				     pwr_attr->upper_name, prop_buf);
>  		if (ret)
>  			goto out;
>  	}
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

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

Re: [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list

[Mat King]

On Fri, May 1, 2020 at 6:32 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Fri, Apr 24, 2020 at 11:35:30AM -0600, Mathew King wrote:
> > Make the device attribute list used to create sysfs attributes more
> > robust by decoupling the list order from order of the enum defined in
> > power_supply.h. This is done by using a designated initializer in the
> > POWER_SUPPLY_ATTR macro.
>
> Thanks for doing this :)
>
> > Also properties not added to the list will still be created with
> > name in the format "prop_*".
>
> This is bad. We don't want to expose PROP_%d to userspace and risk
> this becoming ABI. Instead it should not be registered and a warning
> should be printed. This means we can drop the prop_name field and directly
> use upper, which will always be in TEXT segment then.
>
> For the lower case name, I think it's better to provide it as second
> argument to POWER_SUPPLY_ATTR. Not very elegant solution, but it
> means, that the string is in the TEXT segment and that we can
> have the complete dev_attr initialized from the macro.

Thanks for the feedback. I just sent a new patch series with these
issues addressed.

>
> -- Sebastian
>
> > Signed-off-by: Mathew King <mathewk@chromium.org>
> > ---
> >  drivers/power/supply/power_supply.h       |   7 +-
> >  drivers/power/supply/power_supply_core.c  |   9 +-
> >  drivers/power/supply/power_supply_sysfs.c | 290 +++++++++++++---------
> >  3 files changed, 179 insertions(+), 127 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply.h b/drivers/power/supply/power_supply.h
> > index c310d4f36c10..e3475d35ec2a 100644
> > --- a/drivers/power/supply/power_supply.h
> > +++ b/drivers/power/supply/power_supply.h
> > @@ -15,12 +15,15 @@ struct power_supply;
> >
> >  #ifdef CONFIG_SYSFS
> >
> > -extern void power_supply_init_attrs(struct device_type *dev_type);
> > +extern int power_supply_init_attrs(struct device_type *dev_type);
> > +extern void power_supply_destroy_attrs(void);
> >  extern int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env);
> >
> >  #else
> >
> > -static inline void power_supply_init_attrs(struct device_type *dev_type) {}
> > +static inline int power_supply_init_attrs(struct device_type *dev_type)
> > +{ return 0; }
> > +static void power_supply_destroy_attrs(void) {}
> >  #define power_supply_uevent NULL
> >
> >  #endif /* CONFIG_SYSFS */
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index 1a9a9fae73d3..7fb99302b532 100644
> > --- a/drivers/power/supply/power_supply_core.c
> > +++ b/drivers/power/supply/power_supply_core.c
> > @@ -1329,19 +1329,26 @@ EXPORT_SYMBOL_GPL(power_supply_get_drvdata);
> >
> >  static int __init power_supply_class_init(void)
> >  {
> > +     int ret;
> >       power_supply_class = class_create(THIS_MODULE, "power_supply");
> >
> >       if (IS_ERR(power_supply_class))
> >               return PTR_ERR(power_supply_class);
> >
> >       power_supply_class->dev_uevent = power_supply_uevent;
> > -     power_supply_init_attrs(&power_supply_dev_type);
> > +
> > +     ret = power_supply_init_attrs(&power_supply_dev_type);
> > +     if (ret) {
> > +             class_destroy(power_supply_class);
> > +             return ret;
> > +     }
> >
> >       return 0;
> >  }
> >
> >  static void __exit power_supply_class_exit(void)
> >  {
> > +     power_supply_destroy_attrs();
> >       class_destroy(power_supply_class);
> >  }
> >
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index f37ad4eae60b..b579081599d7 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -18,27 +18,22 @@
> >
> >  #include "power_supply.h"
> >
> > -/*
> > - * This is because the name "current" breaks the device attr macro.
> > - * The "current" word resolves to "(get_current())" so instead of
> > - * "current" "(get_current())" appears in the sysfs.
> > - *
> > - * The source of this definition is the device.h which calls __ATTR
> > - * macro in sysfs.h which calls the __stringify macro.
> > - *
> > - * Only modification that the name is not tried to be resolved
> > - * (as a macro let's say).
> > - */
> > +#define UNDEF_PROP_NAME_FMT "PROP_%d"
> > +#define UNDEF_PROP_NAME_LEN (strlen(UNDEF_PROP_NAME_FMT) + 2)
> > +
> > +struct power_supply_attr {
> > +     const char *prop_name;
> > +     const char *upper_name;
> > +     const char *lower_name;
> > +     struct device_attribute dev_attr;
> > +};
> >
> > -#define POWER_SUPPLY_ATTR(_name)                                     \
> > -{                                                                    \
> > -     .attr = { .name = #_name },                                     \
> > -     .show = power_supply_show_property,                             \
> > -     .store = power_supply_store_property,                           \
> > +#define POWER_SUPPLY_ATTR(_name)     \
> > +[POWER_SUPPLY_PROP_ ## _name] =              \
> > +{                                    \
> > +     .prop_name = #_name             \
> >  }
> >
> > -static struct device_attribute power_supply_attrs[];
> > -
> >  static const char * const power_supply_type_text[] = {
> >       "Unknown", "Battery", "UPS", "Mains", "USB",
> >       "USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> > @@ -77,6 +72,91 @@ static const char * const power_supply_scope_text[] = {
> >       "Unknown", "System", "Device"
> >  };
> >
> > +static struct power_supply_attr power_supply_attrs[] = {
> > +     /* Properties of type `int' */
> > +     POWER_SUPPLY_ATTR(STATUS),
> > +     POWER_SUPPLY_ATTR(CHARGE_TYPE),
> > +     POWER_SUPPLY_ATTR(HEALTH),
> > +     POWER_SUPPLY_ATTR(PRESENT),
> > +     POWER_SUPPLY_ATTR(ONLINE),
> > +     POWER_SUPPLY_ATTR(AUTHENTIC),
> > +     POWER_SUPPLY_ATTR(TECHNOLOGY),
> > +     POWER_SUPPLY_ATTR(CYCLE_COUNT),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_MAX),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_MIN),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_MAX_DESIGN),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_MIN_DESIGN),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_NOW),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_AVG),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_OCV),
> > +     POWER_SUPPLY_ATTR(VOLTAGE_BOOT),
> > +     POWER_SUPPLY_ATTR(CURRENT_MAX),
> > +     POWER_SUPPLY_ATTR(CURRENT_NOW),
> > +     POWER_SUPPLY_ATTR(CURRENT_AVG),
> > +     POWER_SUPPLY_ATTR(CURRENT_BOOT),
> > +     POWER_SUPPLY_ATTR(POWER_NOW),
> > +     POWER_SUPPLY_ATTR(POWER_AVG),
> > +     POWER_SUPPLY_ATTR(CHARGE_FULL_DESIGN),
> > +     POWER_SUPPLY_ATTR(CHARGE_EMPTY_DESIGN),
> > +     POWER_SUPPLY_ATTR(CHARGE_FULL),
> > +     POWER_SUPPLY_ATTR(CHARGE_EMPTY),
> > +     POWER_SUPPLY_ATTR(CHARGE_NOW),
> > +     POWER_SUPPLY_ATTR(CHARGE_AVG),
> > +     POWER_SUPPLY_ATTR(CHARGE_COUNTER),
> > +     POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT),
> > +     POWER_SUPPLY_ATTR(CONSTANT_CHARGE_CURRENT_MAX),
> > +     POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE),
> > +     POWER_SUPPLY_ATTR(CONSTANT_CHARGE_VOLTAGE_MAX),
> > +     POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT),
> > +     POWER_SUPPLY_ATTR(CHARGE_CONTROL_LIMIT_MAX),
> > +     POWER_SUPPLY_ATTR(CHARGE_CONTROL_START_THRESHOLD),
> > +     POWER_SUPPLY_ATTR(CHARGE_CONTROL_END_THRESHOLD),
> > +     POWER_SUPPLY_ATTR(INPUT_CURRENT_LIMIT),
> > +     POWER_SUPPLY_ATTR(INPUT_VOLTAGE_LIMIT),
> > +     POWER_SUPPLY_ATTR(INPUT_POWER_LIMIT),
> > +     POWER_SUPPLY_ATTR(ENERGY_FULL_DESIGN),
> > +     POWER_SUPPLY_ATTR(ENERGY_EMPTY_DESIGN),
> > +     POWER_SUPPLY_ATTR(ENERGY_FULL),
> > +     POWER_SUPPLY_ATTR(ENERGY_EMPTY),
> > +     POWER_SUPPLY_ATTR(ENERGY_NOW),
> > +     POWER_SUPPLY_ATTR(ENERGY_AVG),
> > +     POWER_SUPPLY_ATTR(CAPACITY),
> > +     POWER_SUPPLY_ATTR(CAPACITY_ALERT_MIN),
> > +     POWER_SUPPLY_ATTR(CAPACITY_ALERT_MAX),
> > +     POWER_SUPPLY_ATTR(CAPACITY_LEVEL),
> > +     POWER_SUPPLY_ATTR(TEMP),
> > +     POWER_SUPPLY_ATTR(TEMP_MAX),
> > +     POWER_SUPPLY_ATTR(TEMP_MIN),
> > +     POWER_SUPPLY_ATTR(TEMP_ALERT_MIN),
> > +     POWER_SUPPLY_ATTR(TEMP_ALERT_MAX),
> > +     POWER_SUPPLY_ATTR(TEMP_AMBIENT),
> > +     POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MIN),
> > +     POWER_SUPPLY_ATTR(TEMP_AMBIENT_ALERT_MAX),
> > +     POWER_SUPPLY_ATTR(TIME_TO_EMPTY_NOW),
> > +     POWER_SUPPLY_ATTR(TIME_TO_EMPTY_AVG),
> > +     POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
> > +     POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
> > +     POWER_SUPPLY_ATTR(TYPE),
> > +     POWER_SUPPLY_ATTR(USB_TYPE),
> > +     POWER_SUPPLY_ATTR(SCOPE),
> > +     POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
> > +     POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
> > +     POWER_SUPPLY_ATTR(CALIBRATE),
> > +     /* Properties of type `const char *' */
> > +     POWER_SUPPLY_ATTR(MODEL_NAME),
> > +     POWER_SUPPLY_ATTR(MANUFACTURER),
> > +     POWER_SUPPLY_ATTR(SERIAL_NUMBER),
> > +};
> > +
> > +static struct attribute *
> > +__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
> > +
> > +static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
> > +{
> > +     return container_of(attr, struct power_supply_attr, dev_attr) -
> > +             power_supply_attrs;
> > +}
> > +
> >  static ssize_t power_supply_show_usb_type(struct device *dev,
> >                                         enum power_supply_usb_type *usb_types,
> >                                         ssize_t num_usb_types,
> > @@ -117,7 +197,7 @@ static ssize_t power_supply_show_property(struct device *dev,
> >                                         char *buf) {
> >       ssize_t ret;
> >       struct power_supply *psy = dev_get_drvdata(dev);
> > -     enum power_supply_property psp = attr - power_supply_attrs;
> > +     enum power_supply_property psp = dev_attr_psp(attr);
> >       union power_supply_propval value;
> >
> >       if (psp == POWER_SUPPLY_PROP_TYPE) {
> > @@ -186,7 +266,7 @@ static ssize_t power_supply_store_property(struct device *dev,
> >                                          const char *buf, size_t count) {
> >       ssize_t ret;
> >       struct power_supply *psy = dev_get_drvdata(dev);
> > -     enum power_supply_property psp = attr - power_supply_attrs;
> > +     enum power_supply_property psp = dev_attr_psp(attr);
> >       union power_supply_propval value;
> >
> >       switch (psp) {
> > @@ -235,86 +315,6 @@ static ssize_t power_supply_store_property(struct device *dev,
> >       return count;
> >  }
> >
> > -/* Must be in the same order as POWER_SUPPLY_PROP_* */
> > -static struct device_attribute power_supply_attrs[] = {
> > -     /* Properties of type `int' */
> > -     POWER_SUPPLY_ATTR(status),
> > -     POWER_SUPPLY_ATTR(charge_type),
> > -     POWER_SUPPLY_ATTR(health),
> > -     POWER_SUPPLY_ATTR(present),
> > -     POWER_SUPPLY_ATTR(online),
> > -     POWER_SUPPLY_ATTR(authentic),
> > -     POWER_SUPPLY_ATTR(technology),
> > -     POWER_SUPPLY_ATTR(cycle_count),
> > -     POWER_SUPPLY_ATTR(voltage_max),
> > -     POWER_SUPPLY_ATTR(voltage_min),
> > -     POWER_SUPPLY_ATTR(voltage_max_design),
> > -     POWER_SUPPLY_ATTR(voltage_min_design),
> > -     POWER_SUPPLY_ATTR(voltage_now),
> > -     POWER_SUPPLY_ATTR(voltage_avg),
> > -     POWER_SUPPLY_ATTR(voltage_ocv),
> > -     POWER_SUPPLY_ATTR(voltage_boot),
> > -     POWER_SUPPLY_ATTR(current_max),
> > -     POWER_SUPPLY_ATTR(current_now),
> > -     POWER_SUPPLY_ATTR(current_avg),
> > -     POWER_SUPPLY_ATTR(current_boot),
> > -     POWER_SUPPLY_ATTR(power_now),
> > -     POWER_SUPPLY_ATTR(power_avg),
> > -     POWER_SUPPLY_ATTR(charge_full_design),
> > -     POWER_SUPPLY_ATTR(charge_empty_design),
> > -     POWER_SUPPLY_ATTR(charge_full),
> > -     POWER_SUPPLY_ATTR(charge_empty),
> > -     POWER_SUPPLY_ATTR(charge_now),
> > -     POWER_SUPPLY_ATTR(charge_avg),
> > -     POWER_SUPPLY_ATTR(charge_counter),
> > -     POWER_SUPPLY_ATTR(constant_charge_current),
> > -     POWER_SUPPLY_ATTR(constant_charge_current_max),
> > -     POWER_SUPPLY_ATTR(constant_charge_voltage),
> > -     POWER_SUPPLY_ATTR(constant_charge_voltage_max),
> > -     POWER_SUPPLY_ATTR(charge_control_limit),
> > -     POWER_SUPPLY_ATTR(charge_control_limit_max),
> > -     POWER_SUPPLY_ATTR(charge_control_start_threshold),
> > -     POWER_SUPPLY_ATTR(charge_control_end_threshold),
> > -     POWER_SUPPLY_ATTR(input_current_limit),
> > -     POWER_SUPPLY_ATTR(input_voltage_limit),
> > -     POWER_SUPPLY_ATTR(input_power_limit),
> > -     POWER_SUPPLY_ATTR(energy_full_design),
> > -     POWER_SUPPLY_ATTR(energy_empty_design),
> > -     POWER_SUPPLY_ATTR(energy_full),
> > -     POWER_SUPPLY_ATTR(energy_empty),
> > -     POWER_SUPPLY_ATTR(energy_now),
> > -     POWER_SUPPLY_ATTR(energy_avg),
> > -     POWER_SUPPLY_ATTR(capacity),
> > -     POWER_SUPPLY_ATTR(capacity_alert_min),
> > -     POWER_SUPPLY_ATTR(capacity_alert_max),
> > -     POWER_SUPPLY_ATTR(capacity_level),
> > -     POWER_SUPPLY_ATTR(temp),
> > -     POWER_SUPPLY_ATTR(temp_max),
> > -     POWER_SUPPLY_ATTR(temp_min),
> > -     POWER_SUPPLY_ATTR(temp_alert_min),
> > -     POWER_SUPPLY_ATTR(temp_alert_max),
> > -     POWER_SUPPLY_ATTR(temp_ambient),
> > -     POWER_SUPPLY_ATTR(temp_ambient_alert_min),
> > -     POWER_SUPPLY_ATTR(temp_ambient_alert_max),
> > -     POWER_SUPPLY_ATTR(time_to_empty_now),
> > -     POWER_SUPPLY_ATTR(time_to_empty_avg),
> > -     POWER_SUPPLY_ATTR(time_to_full_now),
> > -     POWER_SUPPLY_ATTR(time_to_full_avg),
> > -     POWER_SUPPLY_ATTR(type),
> > -     POWER_SUPPLY_ATTR(usb_type),
> > -     POWER_SUPPLY_ATTR(scope),
> > -     POWER_SUPPLY_ATTR(precharge_current),
> > -     POWER_SUPPLY_ATTR(charge_term_current),
> > -     POWER_SUPPLY_ATTR(calibrate),
> > -     /* Properties of type `const char *' */
> > -     POWER_SUPPLY_ATTR(model_name),
> > -     POWER_SUPPLY_ATTR(manufacturer),
> > -     POWER_SUPPLY_ATTR(serial_number),
> > -};
> > -
> > -static struct attribute *
> > -__power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
> > -
> >  static umode_t power_supply_attr_is_visible(struct kobject *kobj,
> >                                          struct attribute *attr,
> >                                          int attrno)
> > @@ -352,17 +352,7 @@ static const struct attribute_group *power_supply_attr_groups[] = {
> >       NULL,
> >  };
> >
> > -void power_supply_init_attrs(struct device_type *dev_type)
> > -{
> > -     int i;
> > -
> > -     dev_type->groups = power_supply_attr_groups;
> > -
> > -     for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++)
> > -             __power_supply_attrs[i] = &power_supply_attrs[i].attr;
> > -}
> > -
> > -static char *kstruprdup(const char *str, gfp_t gfp)
> > +static char *kstrlwrdup(const char *str, gfp_t gfp)
> >  {
> >       char *ret, *ustr;
> >
> > @@ -372,19 +362,75 @@ static char *kstruprdup(const char *str, gfp_t gfp)
> >               return NULL;
> >
> >       while (*str)
> > -             *ustr++ = toupper(*str++);
> > +             *ustr++ = tolower(*str++);
> >
> >       *ustr = 0;
> >
> >       return ret;
> >  }
> >
> > +int power_supply_init_attrs(struct device_type *dev_type)
> > +{
> > +     int i;
> > +
> > +     dev_type->groups = power_supply_attr_groups;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> > +             const char *upper, *lower;
> > +             struct device_attribute *attr;
> > +
> > +             upper = power_supply_attrs[i].prop_name;
> > +
> > +             if (!upper) {
> > +                     char *name = kmalloc(UNDEF_PROP_NAME_LEN, GFP_KERNEL);
> > +
> > +                     if (!name)
> > +                             goto err_no_mem;
> > +
> > +                     sprintf(name, UNDEF_PROP_NAME_FMT, i);
> > +                     upper = name;
> > +             }
> > +
> > +             lower = kstrlwrdup(upper, GFP_KERNEL);
> > +
> > +             if (!lower)
> > +                     goto err_no_mem;
> > +
> > +             power_supply_attrs[i].upper_name = upper;
> > +             power_supply_attrs[i].lower_name = lower;
> > +
> > +             attr = &power_supply_attrs[i].dev_attr;
> > +
> > +             attr->attr.name = lower;
> > +             attr->show = power_supply_show_property;
> > +             attr->store = power_supply_store_property;
> > +             __power_supply_attrs[i] = &attr->attr;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_no_mem:
> > +     power_supply_destroy_attrs();
> > +     return -ENOMEM;
> > +}
> > +
> > +void power_supply_destroy_attrs(void)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(power_supply_attrs); i++) {
> > +             kfree(power_supply_attrs[i].lower_name);
> > +
> > +             if (!power_supply_attrs[i].prop_name)
> > +                     kfree(power_supply_attrs[i].upper_name);
> > +     }
> > +}
> > +
> >  int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  {
> >       struct power_supply *psy = dev_get_drvdata(dev);
> >       int ret = 0, j;
> >       char *prop_buf;
> > -     char *attrname;
> >
> >       if (!psy || !psy->desc) {
> >               dev_dbg(dev, "No power supply yet\n");
> > @@ -400,12 +446,14 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
> >               return -ENOMEM;
> >
> >       for (j = 0; j < psy->desc->num_properties; j++) {
> > -             struct device_attribute *attr;
> > +             struct power_supply_attr *pwr_attr;
> > +             struct device_attribute *dev_attr;
> >               char *line;
> >
> > -             attr = &power_supply_attrs[psy->desc->properties[j]];
> > +             pwr_attr = &power_supply_attrs[psy->desc->properties[j]];
> > +             dev_attr = &pwr_attr->dev_attr;
> >
> > -             ret = power_supply_show_property(dev, attr, prop_buf);
> > +             ret = power_supply_show_property(dev, dev_attr, prop_buf);
> >               if (ret == -ENODEV || ret == -ENODATA) {
> >                       /* When a battery is absent, we expect -ENODEV. Don't abort;
> >                          send the uevent with at least the the PRESENT=0 property */
> > @@ -420,14 +468,8 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
> >               if (line)
> >                       *line = 0;
> >
> > -             attrname = kstruprdup(attr->attr.name, GFP_KERNEL);
> > -             if (!attrname) {
> > -                     ret = -ENOMEM;
> > -                     goto out;
> > -             }
> > -
> > -             ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s", attrname, prop_buf);
> > -             kfree(attrname);
> > +             ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s",
> > +                                  pwr_attr->upper_name, prop_buf);
> >               if (ret)
> >                       goto out;
> >       }
> > --
> > 2.26.2.303.gf8c07b1a785-goog
> >

[PATCH 2/4] power_supply: Use designated initializer for property text arrays

[Mathew King]

Use designated initializers for the sysfs power supply text values. This
will help ensure that the text values are kept in sync with the enum
values from power_supply.h.

Signed-off-by: Mathew King <mathewk@chromium.org>
---
 drivers/power/supply/power_supply_sysfs.c | 76 ++++++++++++++++++-----
 1 file changed, 61 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index b579081599d7..328107589770 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -35,41 +35,87 @@ struct power_supply_attr {
 }
 
 static const char * const power_supply_type_text[] = {
-	"Unknown", "Battery", "UPS", "Mains", "USB",
-	"USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
-	"USB_PD", "USB_PD_DRP", "BrickID"
+	[POWER_SUPPLY_TYPE_UNKNOWN]		= "Unknown",
+	[POWER_SUPPLY_TYPE_BATTERY]		= "Battery",
+	[POWER_SUPPLY_TYPE_UPS]			= "UPS",
+	[POWER_SUPPLY_TYPE_MAINS]		= "Mains",
+	[POWER_SUPPLY_TYPE_USB]			= "USB",
+	[POWER_SUPPLY_TYPE_USB_DCP]		= "USB_DCP",
+	[POWER_SUPPLY_TYPE_USB_CDP]		= "USB_CDP",
+	[POWER_SUPPLY_TYPE_USB_ACA]		= "USB_ACA",
+	[POWER_SUPPLY_TYPE_USB_TYPE_C]		= "USB_C",
+	[POWER_SUPPLY_TYPE_USB_PD]		= "USB_PD",
+	[POWER_SUPPLY_TYPE_USB_PD_DRP]		= "USB_PD_DRP",
+	[POWER_SUPPLY_TYPE_APPLE_BRICK_ID]	= "BrickID",
 };
 
-static const char * const power_supply_usb_type_text[] = {
-	"Unknown", "SDP", "DCP", "CDP", "ACA", "C",
-	"PD", "PD_DRP", "PD_PPS", "BrickID"
+static const char * const POWER_SUPPLY_USB_TYPE_TEXT[] = {
+	[POWER_SUPPLY_USB_TYPE_UNKNOWN]		= "Unknown",
+	[POWER_SUPPLY_USB_TYPE_SDP]		= "SDP",
+	[POWER_SUPPLY_USB_TYPE_DCP]		= "DCP",
+	[POWER_SUPPLY_USB_TYPE_CDP]		= "CDP",
+	[POWER_SUPPLY_USB_TYPE_ACA]		= "ACA",
+	[POWER_SUPPLY_USB_TYPE_C]		= "C",
+	[POWER_SUPPLY_USB_TYPE_PD]		= "PD",
+	[POWER_SUPPLY_USB_TYPE_PD_DRP]		= "PD_DRP",
+	[POWER_SUPPLY_USB_TYPE_PD_PPS]		= "PD_PPS",
+	[POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID]	= "BrickID",
 };
 
 static const char * const power_supply_status_text[] = {
-	"Unknown", "Charging", "Discharging", "Not charging", "Full"
+	[POWER_SUPPLY_STATUS_UNKNOWN]		= "Unknown",
+	[POWER_SUPPLY_STATUS_CHARGING]		= "Charging",
+	[POWER_SUPPLY_STATUS_DISCHARGING]	= "Discharging",
+	[POWER_SUPPLY_STATUS_NOT_CHARGING]	= "Not charging",
+	[POWER_SUPPLY_STATUS_FULL]		= "Full",
 };
 
 static const char * const power_supply_charge_type_text[] = {
-	"Unknown", "N/A", "Trickle", "Fast", "Standard", "Adaptive", "Custom"
+	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
+	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
+	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
+	[POWER_SUPPLY_CHARGE_TYPE_FAST]		= "Fast",
+	[POWER_SUPPLY_CHARGE_TYPE_STANDARD]	= "Standard",
+	[POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE]	= "Adaptive",
+	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
 };
 
 static const char * const power_supply_health_text[] = {
-	"Unknown", "Good", "Overheat", "Dead", "Over voltage",
-	"Unspecified failure", "Cold", "Watchdog timer expire",
-	"Safety timer expire", "Over current"
+	[POWER_SUPPLY_HEALTH_UNKNOWN]		    = "Unknown",
+	[POWER_SUPPLY_HEALTH_GOOD]		    = "Good",
+	[POWER_SUPPLY_HEALTH_OVERHEAT]		    = "Overheat",
+	[POWER_SUPPLY_HEALTH_DEAD]		    = "Dead",
+	[POWER_SUPPLY_HEALTH_OVERVOLTAGE]	    = "Over voltage",
+	[POWER_SUPPLY_HEALTH_UNSPEC_FAILURE]	    = "Unspecified failure",
+	[POWER_SUPPLY_HEALTH_COLD]		    = "Cold",
+	[POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE] = "Watchdog timer expire",
+	[POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE]   = "Safety timer expire",
+	[POWER_SUPPLY_HEALTH_OVERCURRENT]	    = "Over current",
 };
 
 static const char * const power_supply_technology_text[] = {
-	"Unknown", "NiMH", "Li-ion", "Li-poly", "LiFe", "NiCd",
-	"LiMn"
+	[POWER_SUPPLY_TECHNOLOGY_UNKNOWN]	= "Unknown",
+	[POWER_SUPPLY_TECHNOLOGY_NiMH]		= "NiMH",
+	[POWER_SUPPLY_TECHNOLOGY_LION]		= "Li-ion",
+	[POWER_SUPPLY_TECHNOLOGY_LIPO]		= "Li-poly",
+	[POWER_SUPPLY_TECHNOLOGY_LiFe]		= "LiFe",
+	[POWER_SUPPLY_TECHNOLOGY_NiCd]		= "NiCd",
+	[POWER_SUPPLY_TECHNOLOGY_LiMn]		= "LiMn",
 };
 
 static const char * const power_supply_capacity_level_text[] = {
-	"Unknown", "Critical", "Low", "Normal", "High", "Full"
+	[POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN]	= "Unknown",
+	[POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL]	= "Critical",
+	[POWER_SUPPLY_CAPACITY_LEVEL_LOW]	= "Low",
+	[POWER_SUPPLY_CAPACITY_LEVEL_NORMAL]	= "Normal",
+	[POWER_SUPPLY_CAPACITY_LEVEL_HIGH]	= "High",
+	[POWER_SUPPLY_CAPACITY_LEVEL_FULL]	= "Full",
 };
 
 static const char * const power_supply_scope_text[] = {
-	"Unknown", "System", "Device"
+	[POWER_SUPPLY_SCOPE_UNKNOWN]	= "Unknown",
+	[POWER_SUPPLY_SCOPE_SYSTEM]	= "System",
+	[POWER_SUPPLY_SCOPE_DEVICE]	= "Device",
 };
 
 static struct power_supply_attr power_supply_attrs[] = {
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 2/4] power_supply: Use designated initializer for property text arrays

[Sebastian Reichel]

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

Hi,

On Fri, Apr 24, 2020 at 11:35:31AM -0600, Mathew King wrote:
> Use designated initializers for the sysfs power supply text values. This
> will help ensure that the text values are kept in sync with the enum
> values from power_supply.h.
> 
> Signed-off-by: Mathew King <mathewk@chromium.org>
> ---

Thanks, looks good to me, but does not apply without the first patch
(or due to other changes in power-supply's for-next branch).

-- Sebastian

>  drivers/power/supply/power_supply_sysfs.c | 76 ++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index b579081599d7..328107589770 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -35,41 +35,87 @@ struct power_supply_attr {
>  }
>  
>  static const char * const power_supply_type_text[] = {
> -	"Unknown", "Battery", "UPS", "Mains", "USB",
> -	"USB_DCP", "USB_CDP", "USB_ACA", "USB_C",
> -	"USB_PD", "USB_PD_DRP", "BrickID"
> +	[POWER_SUPPLY_TYPE_UNKNOWN]		= "Unknown",
> +	[POWER_SUPPLY_TYPE_BATTERY]		= "Battery",
> +	[POWER_SUPPLY_TYPE_UPS]			= "UPS",
> +	[POWER_SUPPLY_TYPE_MAINS]		= "Mains",
> +	[POWER_SUPPLY_TYPE_USB]			= "USB",
> +	[POWER_SUPPLY_TYPE_USB_DCP]		= "USB_DCP",
> +	[POWER_SUPPLY_TYPE_USB_CDP]		= "USB_CDP",
> +	[POWER_SUPPLY_TYPE_USB_ACA]		= "USB_ACA",
> +	[POWER_SUPPLY_TYPE_USB_TYPE_C]		= "USB_C",
> +	[POWER_SUPPLY_TYPE_USB_PD]		= "USB_PD",
> +	[POWER_SUPPLY_TYPE_USB_PD_DRP]		= "USB_PD_DRP",
> +	[POWER_SUPPLY_TYPE_APPLE_BRICK_ID]	= "BrickID",
>  };
>  
> -static const char * const power_supply_usb_type_text[] = {
> -	"Unknown", "SDP", "DCP", "CDP", "ACA", "C",
> -	"PD", "PD_DRP", "PD_PPS", "BrickID"
> +static const char * const POWER_SUPPLY_USB_TYPE_TEXT[] = {
> +	[POWER_SUPPLY_USB_TYPE_UNKNOWN]		= "Unknown",
> +	[POWER_SUPPLY_USB_TYPE_SDP]		= "SDP",
> +	[POWER_SUPPLY_USB_TYPE_DCP]		= "DCP",
> +	[POWER_SUPPLY_USB_TYPE_CDP]		= "CDP",
> +	[POWER_SUPPLY_USB_TYPE_ACA]		= "ACA",
> +	[POWER_SUPPLY_USB_TYPE_C]		= "C",
> +	[POWER_SUPPLY_USB_TYPE_PD]		= "PD",
> +	[POWER_SUPPLY_USB_TYPE_PD_DRP]		= "PD_DRP",
> +	[POWER_SUPPLY_USB_TYPE_PD_PPS]		= "PD_PPS",
> +	[POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID]	= "BrickID",
>  };
>  
>  static const char * const power_supply_status_text[] = {
> -	"Unknown", "Charging", "Discharging", "Not charging", "Full"
> +	[POWER_SUPPLY_STATUS_UNKNOWN]		= "Unknown",
> +	[POWER_SUPPLY_STATUS_CHARGING]		= "Charging",
> +	[POWER_SUPPLY_STATUS_DISCHARGING]	= "Discharging",
> +	[POWER_SUPPLY_STATUS_NOT_CHARGING]	= "Not charging",
> +	[POWER_SUPPLY_STATUS_FULL]		= "Full",
>  };
>  
>  static const char * const power_supply_charge_type_text[] = {
> -	"Unknown", "N/A", "Trickle", "Fast", "Standard", "Adaptive", "Custom"
> +	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
> +	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
> +	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
> +	[POWER_SUPPLY_CHARGE_TYPE_FAST]		= "Fast",
> +	[POWER_SUPPLY_CHARGE_TYPE_STANDARD]	= "Standard",
> +	[POWER_SUPPLY_CHARGE_TYPE_ADAPTIVE]	= "Adaptive",
> +	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
>  };
>  
>  static const char * const power_supply_health_text[] = {
> -	"Unknown", "Good", "Overheat", "Dead", "Over voltage",
> -	"Unspecified failure", "Cold", "Watchdog timer expire",
> -	"Safety timer expire", "Over current"
> +	[POWER_SUPPLY_HEALTH_UNKNOWN]		    = "Unknown",
> +	[POWER_SUPPLY_HEALTH_GOOD]		    = "Good",
> +	[POWER_SUPPLY_HEALTH_OVERHEAT]		    = "Overheat",
> +	[POWER_SUPPLY_HEALTH_DEAD]		    = "Dead",
> +	[POWER_SUPPLY_HEALTH_OVERVOLTAGE]	    = "Over voltage",
> +	[POWER_SUPPLY_HEALTH_UNSPEC_FAILURE]	    = "Unspecified failure",
> +	[POWER_SUPPLY_HEALTH_COLD]		    = "Cold",
> +	[POWER_SUPPLY_HEALTH_WATCHDOG_TIMER_EXPIRE] = "Watchdog timer expire",
> +	[POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE]   = "Safety timer expire",
> +	[POWER_SUPPLY_HEALTH_OVERCURRENT]	    = "Over current",
>  };
>  
>  static const char * const power_supply_technology_text[] = {
> -	"Unknown", "NiMH", "Li-ion", "Li-poly", "LiFe", "NiCd",
> -	"LiMn"
> +	[POWER_SUPPLY_TECHNOLOGY_UNKNOWN]	= "Unknown",
> +	[POWER_SUPPLY_TECHNOLOGY_NiMH]		= "NiMH",
> +	[POWER_SUPPLY_TECHNOLOGY_LION]		= "Li-ion",
> +	[POWER_SUPPLY_TECHNOLOGY_LIPO]		= "Li-poly",
> +	[POWER_SUPPLY_TECHNOLOGY_LiFe]		= "LiFe",
> +	[POWER_SUPPLY_TECHNOLOGY_NiCd]		= "NiCd",
> +	[POWER_SUPPLY_TECHNOLOGY_LiMn]		= "LiMn",
>  };
>  
>  static const char * const power_supply_capacity_level_text[] = {
> -	"Unknown", "Critical", "Low", "Normal", "High", "Full"
> +	[POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN]	= "Unknown",
> +	[POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL]	= "Critical",
> +	[POWER_SUPPLY_CAPACITY_LEVEL_LOW]	= "Low",
> +	[POWER_SUPPLY_CAPACITY_LEVEL_NORMAL]	= "Normal",
> +	[POWER_SUPPLY_CAPACITY_LEVEL_HIGH]	= "High",
> +	[POWER_SUPPLY_CAPACITY_LEVEL_FULL]	= "Full",
>  };
>  
>  static const char * const power_supply_scope_text[] = {
> -	"Unknown", "System", "Device"
> +	[POWER_SUPPLY_SCOPE_UNKNOWN]	= "Unknown",
> +	[POWER_SUPPLY_SCOPE_SYSTEM]	= "System",
> +	[POWER_SUPPLY_SCOPE_DEVICE]	= "Device",
>  };
>  
>  static struct power_supply_attr power_supply_attrs[] = {
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

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

[PATCH 3/4] power_supply: Add a macro that maps enum properties to text values

[Mathew King]

Reduce the number of touch points to add a new enum property to the
power_supply class by mapping the array of text values to the device
attribute descriptor. A new enum property can now added by creating an
array with the text values named POWER_SUPPLY_${PROPNAME}_TEXT and
adding POWER_SUPPLY_ENUM_ATTR(${PROPNAME}) to the power_supply_attrs
array.

Signed-off-by: Mathew King <mathewk@chromium.org>
---
 drivers/power/supply/power_supply_sysfs.c | 122 +++++++++-------------
 1 file changed, 49 insertions(+), 73 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index 328107589770..fbb05466b9a5 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -26,15 +26,25 @@ struct power_supply_attr {
 	const char *upper_name;
 	const char *lower_name;
 	struct device_attribute dev_attr;
+	const char * const *text_values;
+	int text_values_len;
 };
 
-#define POWER_SUPPLY_ATTR(_name)	\
-[POWER_SUPPLY_PROP_ ## _name] =		\
-{					\
-	.prop_name = #_name		\
+#define _POWER_SUPPLY_ATTR(_name, _text, _len)	\
+[POWER_SUPPLY_PROP_ ## _name] =			\
+{						\
+	.prop_name = #_name,			\
+	.text_values = _text,			\
+	.text_values_len = _len,		\
 }
 
-static const char * const power_supply_type_text[] = {
+#define POWER_SUPPLY_ATTR(_name) _POWER_SUPPLY_ATTR(_name, NULL, 0)
+#define _POWER_SUPPLY_ENUM_ATTR(_name, _text)	\
+	_POWER_SUPPLY_ATTR(_name, _text, ARRAY_SIZE(_text))
+#define POWER_SUPPLY_ENUM_ATTR(_name)	\
+	_POWER_SUPPLY_ENUM_ATTR(_name, POWER_SUPPLY_ ## _name ## _TEXT)
+
+static const char * const POWER_SUPPLY_TYPE_TEXT[] = {
 	[POWER_SUPPLY_TYPE_UNKNOWN]		= "Unknown",
 	[POWER_SUPPLY_TYPE_BATTERY]		= "Battery",
 	[POWER_SUPPLY_TYPE_UPS]			= "UPS",
@@ -62,7 +72,7 @@ static const char * const POWER_SUPPLY_USB_TYPE_TEXT[] = {
 	[POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID]	= "BrickID",
 };
 
-static const char * const power_supply_status_text[] = {
+static const char * const POWER_SUPPLY_STATUS_TEXT[] = {
 	[POWER_SUPPLY_STATUS_UNKNOWN]		= "Unknown",
 	[POWER_SUPPLY_STATUS_CHARGING]		= "Charging",
 	[POWER_SUPPLY_STATUS_DISCHARGING]	= "Discharging",
@@ -70,7 +80,7 @@ static const char * const power_supply_status_text[] = {
 	[POWER_SUPPLY_STATUS_FULL]		= "Full",
 };
 
-static const char * const power_supply_charge_type_text[] = {
+static const char * const POWER_SUPPLY_CHARGE_TYPE_TEXT[] = {
 	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
 	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
 	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
@@ -80,7 +90,7 @@ static const char * const power_supply_charge_type_text[] = {
 	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
 };
 
-static const char * const power_supply_health_text[] = {
+static const char * const POWER_SUPPLY_HEALTH_TEXT[] = {
 	[POWER_SUPPLY_HEALTH_UNKNOWN]		    = "Unknown",
 	[POWER_SUPPLY_HEALTH_GOOD]		    = "Good",
 	[POWER_SUPPLY_HEALTH_OVERHEAT]		    = "Overheat",
@@ -93,7 +103,7 @@ static const char * const power_supply_health_text[] = {
 	[POWER_SUPPLY_HEALTH_OVERCURRENT]	    = "Over current",
 };
 
-static const char * const power_supply_technology_text[] = {
+static const char * const POWER_SUPPLY_TECHNOLOGY_TEXT[] = {
 	[POWER_SUPPLY_TECHNOLOGY_UNKNOWN]	= "Unknown",
 	[POWER_SUPPLY_TECHNOLOGY_NiMH]		= "NiMH",
 	[POWER_SUPPLY_TECHNOLOGY_LION]		= "Li-ion",
@@ -103,7 +113,7 @@ static const char * const power_supply_technology_text[] = {
 	[POWER_SUPPLY_TECHNOLOGY_LiMn]		= "LiMn",
 };
 
-static const char * const power_supply_capacity_level_text[] = {
+static const char * const POWER_SUPPLY_CAPACITY_LEVEL_TEXT[] = {
 	[POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN]	= "Unknown",
 	[POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL]	= "Critical",
 	[POWER_SUPPLY_CAPACITY_LEVEL_LOW]	= "Low",
@@ -112,7 +122,7 @@ static const char * const power_supply_capacity_level_text[] = {
 	[POWER_SUPPLY_CAPACITY_LEVEL_FULL]	= "Full",
 };
 
-static const char * const power_supply_scope_text[] = {
+static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
 	[POWER_SUPPLY_SCOPE_UNKNOWN]	= "Unknown",
 	[POWER_SUPPLY_SCOPE_SYSTEM]	= "System",
 	[POWER_SUPPLY_SCOPE_DEVICE]	= "Device",
@@ -120,13 +130,13 @@ static const char * const power_supply_scope_text[] = {
 
 static struct power_supply_attr power_supply_attrs[] = {
 	/* Properties of type `int' */
-	POWER_SUPPLY_ATTR(STATUS),
-	POWER_SUPPLY_ATTR(CHARGE_TYPE),
-	POWER_SUPPLY_ATTR(HEALTH),
+	POWER_SUPPLY_ENUM_ATTR(STATUS),
+	POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE),
+	POWER_SUPPLY_ENUM_ATTR(HEALTH),
 	POWER_SUPPLY_ATTR(PRESENT),
 	POWER_SUPPLY_ATTR(ONLINE),
 	POWER_SUPPLY_ATTR(AUTHENTIC),
-	POWER_SUPPLY_ATTR(TECHNOLOGY),
+	POWER_SUPPLY_ENUM_ATTR(TECHNOLOGY),
 	POWER_SUPPLY_ATTR(CYCLE_COUNT),
 	POWER_SUPPLY_ATTR(VOLTAGE_MAX),
 	POWER_SUPPLY_ATTR(VOLTAGE_MIN),
@@ -169,7 +179,7 @@ static struct power_supply_attr power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(CAPACITY),
 	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MIN),
 	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MAX),
-	POWER_SUPPLY_ATTR(CAPACITY_LEVEL),
+	POWER_SUPPLY_ENUM_ATTR(CAPACITY_LEVEL),
 	POWER_SUPPLY_ATTR(TEMP),
 	POWER_SUPPLY_ATTR(TEMP_MAX),
 	POWER_SUPPLY_ATTR(TEMP_MIN),
@@ -182,9 +192,9 @@ static struct power_supply_attr power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_AVG),
 	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
 	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
-	POWER_SUPPLY_ATTR(TYPE),
+	POWER_SUPPLY_ENUM_ATTR(TYPE),
 	POWER_SUPPLY_ATTR(USB_TYPE),
-	POWER_SUPPLY_ATTR(SCOPE),
+	POWER_SUPPLY_ENUM_ATTR(SCOPE),
 	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
 	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
 	POWER_SUPPLY_ATTR(CALIBRATE),
@@ -197,10 +207,14 @@ static struct power_supply_attr power_supply_attrs[] = {
 static struct attribute *
 __power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
 
+static struct power_supply_attr *to_ps_attr(struct device_attribute *attr)
+{
+	return container_of(attr, struct power_supply_attr, dev_attr);
+}
+
 static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
 {
-	return container_of(attr, struct power_supply_attr, dev_attr) -
-		power_supply_attrs;
+	return  to_ps_attr(attr) - power_supply_attrs;
 }
 
 static ssize_t power_supply_show_usb_type(struct device *dev,
@@ -219,11 +233,11 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
 
 		if (value->intval == usb_type) {
 			count += sprintf(buf + count, "[%s] ",
-					 power_supply_usb_type_text[usb_type]);
+					 POWER_SUPPLY_USB_TYPE_TEXT[usb_type]);
 			match = true;
 		} else {
 			count += sprintf(buf + count, "%s ",
-					 power_supply_usb_type_text[usb_type]);
+					 POWER_SUPPLY_USB_TYPE_TEXT[usb_type]);
 		}
 	}
 
@@ -243,6 +257,7 @@ static ssize_t power_supply_show_property(struct device *dev,
 					  char *buf) {
 	ssize_t ret;
 	struct power_supply *psy = dev_get_drvdata(dev);
+	struct power_supply_attr *ps_attr = to_ps_attr(attr);
 	enum power_supply_property psp = dev_attr_psp(attr);
 	union power_supply_propval value;
 
@@ -263,39 +278,16 @@ static ssize_t power_supply_show_property(struct device *dev,
 		}
 	}
 
+	if (ps_attr->text_values_len > 0 &&
+	    value.intval < ps_attr->text_values_len && value.intval >= 0) {
+		return sprintf(buf, "%s\n", ps_attr->text_values[value.intval]);
+	}
+
 	switch (psp) {
-	case POWER_SUPPLY_PROP_STATUS:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_status_text[value.intval]);
-		break;
-	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_charge_type_text[value.intval]);
-		break;
-	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_health_text[value.intval]);
-		break;
-	case POWER_SUPPLY_PROP_TECHNOLOGY:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_technology_text[value.intval]);
-		break;
-	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_capacity_level_text[value.intval]);
-		break;
-	case POWER_SUPPLY_PROP_TYPE:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_type_text[value.intval]);
-		break;
 	case POWER_SUPPLY_PROP_USB_TYPE:
 		ret = power_supply_show_usb_type(dev, psy->desc->usb_types,
-						 psy->desc->num_usb_types,
-						 &value, buf);
-		break;
-	case POWER_SUPPLY_PROP_SCOPE:
-		ret = sprintf(buf, "%s\n",
-			      power_supply_scope_text[value.intval]);
+						psy->desc->num_usb_types,
+						&value, buf);
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
 		ret = sprintf(buf, "%s\n", value.strval);
@@ -312,30 +304,14 @@ static ssize_t power_supply_store_property(struct device *dev,
 					   const char *buf, size_t count) {
 	ssize_t ret;
 	struct power_supply *psy = dev_get_drvdata(dev);
+	struct power_supply_attr *ps_attr = to_ps_attr(attr);
 	enum power_supply_property psp = dev_attr_psp(attr);
 	union power_supply_propval value;
 
-	switch (psp) {
-	case POWER_SUPPLY_PROP_STATUS:
-		ret = sysfs_match_string(power_supply_status_text, buf);
-		break;
-	case POWER_SUPPLY_PROP_CHARGE_TYPE:
-		ret = sysfs_match_string(power_supply_charge_type_text, buf);
-		break;
-	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sysfs_match_string(power_supply_health_text, buf);
-		break;
-	case POWER_SUPPLY_PROP_TECHNOLOGY:
-		ret = sysfs_match_string(power_supply_technology_text, buf);
-		break;
-	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
-		ret = sysfs_match_string(power_supply_capacity_level_text, buf);
-		break;
-	case POWER_SUPPLY_PROP_SCOPE:
-		ret = sysfs_match_string(power_supply_scope_text, buf);
-		break;
-	default:
-		ret = -EINVAL;
+	ret = -EINVAL;
+	if (ps_attr->text_values_len > 0) {
+		ret = __sysfs_match_string(ps_attr->text_values,
+					   ps_attr->text_values_len, buf);
 	}
 
 	/*
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values

[Sebastian Reichel]

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

Hi,

On Fri, Apr 24, 2020 at 11:35:32AM -0600, Mathew King wrote:
> Reduce the number of touch points to add a new enum property to the
> power_supply class by mapping the array of text values to the device
> attribute descriptor. A new enum property can now added by creating an
> array with the text values named POWER_SUPPLY_${PROPNAME}_TEXT and
> adding POWER_SUPPLY_ENUM_ATTR(${PROPNAME}) to the power_supply_attrs
> array.
> 
> Signed-off-by: Mathew King <mathewk@chromium.org>
> ---

nice cleanup :)

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

-- Sebastian

>  drivers/power/supply/power_supply_sysfs.c | 122 +++++++++-------------
>  1 file changed, 49 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index 328107589770..fbb05466b9a5 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -26,15 +26,25 @@ struct power_supply_attr {
>  	const char *upper_name;
>  	const char *lower_name;
>  	struct device_attribute dev_attr;
> +	const char * const *text_values;
> +	int text_values_len;
>  };
>  
> -#define POWER_SUPPLY_ATTR(_name)	\
> -[POWER_SUPPLY_PROP_ ## _name] =		\
> -{					\
> -	.prop_name = #_name		\
> +#define _POWER_SUPPLY_ATTR(_name, _text, _len)	\
> +[POWER_SUPPLY_PROP_ ## _name] =			\
> +{						\
> +	.prop_name = #_name,			\
> +	.text_values = _text,			\
> +	.text_values_len = _len,		\
>  }
>  
> -static const char * const power_supply_type_text[] = {
> +#define POWER_SUPPLY_ATTR(_name) _POWER_SUPPLY_ATTR(_name, NULL, 0)
> +#define _POWER_SUPPLY_ENUM_ATTR(_name, _text)	\
> +	_POWER_SUPPLY_ATTR(_name, _text, ARRAY_SIZE(_text))
> +#define POWER_SUPPLY_ENUM_ATTR(_name)	\
> +	_POWER_SUPPLY_ENUM_ATTR(_name, POWER_SUPPLY_ ## _name ## _TEXT)
> +
> +static const char * const POWER_SUPPLY_TYPE_TEXT[] = {
>  	[POWER_SUPPLY_TYPE_UNKNOWN]		= "Unknown",
>  	[POWER_SUPPLY_TYPE_BATTERY]		= "Battery",
>  	[POWER_SUPPLY_TYPE_UPS]			= "UPS",
> @@ -62,7 +72,7 @@ static const char * const POWER_SUPPLY_USB_TYPE_TEXT[] = {
>  	[POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID]	= "BrickID",
>  };
>  
> -static const char * const power_supply_status_text[] = {
> +static const char * const POWER_SUPPLY_STATUS_TEXT[] = {
>  	[POWER_SUPPLY_STATUS_UNKNOWN]		= "Unknown",
>  	[POWER_SUPPLY_STATUS_CHARGING]		= "Charging",
>  	[POWER_SUPPLY_STATUS_DISCHARGING]	= "Discharging",
> @@ -70,7 +80,7 @@ static const char * const power_supply_status_text[] = {
>  	[POWER_SUPPLY_STATUS_FULL]		= "Full",
>  };
>  
> -static const char * const power_supply_charge_type_text[] = {
> +static const char * const POWER_SUPPLY_CHARGE_TYPE_TEXT[] = {
>  	[POWER_SUPPLY_CHARGE_TYPE_UNKNOWN]	= "Unknown",
>  	[POWER_SUPPLY_CHARGE_TYPE_NONE]		= "N/A",
>  	[POWER_SUPPLY_CHARGE_TYPE_TRICKLE]	= "Trickle",
> @@ -80,7 +90,7 @@ static const char * const power_supply_charge_type_text[] = {
>  	[POWER_SUPPLY_CHARGE_TYPE_CUSTOM]	= "Custom",
>  };
>  
> -static const char * const power_supply_health_text[] = {
> +static const char * const POWER_SUPPLY_HEALTH_TEXT[] = {
>  	[POWER_SUPPLY_HEALTH_UNKNOWN]		    = "Unknown",
>  	[POWER_SUPPLY_HEALTH_GOOD]		    = "Good",
>  	[POWER_SUPPLY_HEALTH_OVERHEAT]		    = "Overheat",
> @@ -93,7 +103,7 @@ static const char * const power_supply_health_text[] = {
>  	[POWER_SUPPLY_HEALTH_OVERCURRENT]	    = "Over current",
>  };
>  
> -static const char * const power_supply_technology_text[] = {
> +static const char * const POWER_SUPPLY_TECHNOLOGY_TEXT[] = {
>  	[POWER_SUPPLY_TECHNOLOGY_UNKNOWN]	= "Unknown",
>  	[POWER_SUPPLY_TECHNOLOGY_NiMH]		= "NiMH",
>  	[POWER_SUPPLY_TECHNOLOGY_LION]		= "Li-ion",
> @@ -103,7 +113,7 @@ static const char * const power_supply_technology_text[] = {
>  	[POWER_SUPPLY_TECHNOLOGY_LiMn]		= "LiMn",
>  };
>  
> -static const char * const power_supply_capacity_level_text[] = {
> +static const char * const POWER_SUPPLY_CAPACITY_LEVEL_TEXT[] = {
>  	[POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN]	= "Unknown",
>  	[POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL]	= "Critical",
>  	[POWER_SUPPLY_CAPACITY_LEVEL_LOW]	= "Low",
> @@ -112,7 +122,7 @@ static const char * const power_supply_capacity_level_text[] = {
>  	[POWER_SUPPLY_CAPACITY_LEVEL_FULL]	= "Full",
>  };
>  
> -static const char * const power_supply_scope_text[] = {
> +static const char * const POWER_SUPPLY_SCOPE_TEXT[] = {
>  	[POWER_SUPPLY_SCOPE_UNKNOWN]	= "Unknown",
>  	[POWER_SUPPLY_SCOPE_SYSTEM]	= "System",
>  	[POWER_SUPPLY_SCOPE_DEVICE]	= "Device",
> @@ -120,13 +130,13 @@ static const char * const power_supply_scope_text[] = {
>  
>  static struct power_supply_attr power_supply_attrs[] = {
>  	/* Properties of type `int' */
> -	POWER_SUPPLY_ATTR(STATUS),
> -	POWER_SUPPLY_ATTR(CHARGE_TYPE),
> -	POWER_SUPPLY_ATTR(HEALTH),
> +	POWER_SUPPLY_ENUM_ATTR(STATUS),
> +	POWER_SUPPLY_ENUM_ATTR(CHARGE_TYPE),
> +	POWER_SUPPLY_ENUM_ATTR(HEALTH),
>  	POWER_SUPPLY_ATTR(PRESENT),
>  	POWER_SUPPLY_ATTR(ONLINE),
>  	POWER_SUPPLY_ATTR(AUTHENTIC),
> -	POWER_SUPPLY_ATTR(TECHNOLOGY),
> +	POWER_SUPPLY_ENUM_ATTR(TECHNOLOGY),
>  	POWER_SUPPLY_ATTR(CYCLE_COUNT),
>  	POWER_SUPPLY_ATTR(VOLTAGE_MAX),
>  	POWER_SUPPLY_ATTR(VOLTAGE_MIN),
> @@ -169,7 +179,7 @@ static struct power_supply_attr power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(CAPACITY),
>  	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MIN),
>  	POWER_SUPPLY_ATTR(CAPACITY_ALERT_MAX),
> -	POWER_SUPPLY_ATTR(CAPACITY_LEVEL),
> +	POWER_SUPPLY_ENUM_ATTR(CAPACITY_LEVEL),
>  	POWER_SUPPLY_ATTR(TEMP),
>  	POWER_SUPPLY_ATTR(TEMP_MAX),
>  	POWER_SUPPLY_ATTR(TEMP_MIN),
> @@ -182,9 +192,9 @@ static struct power_supply_attr power_supply_attrs[] = {
>  	POWER_SUPPLY_ATTR(TIME_TO_EMPTY_AVG),
>  	POWER_SUPPLY_ATTR(TIME_TO_FULL_NOW),
>  	POWER_SUPPLY_ATTR(TIME_TO_FULL_AVG),
> -	POWER_SUPPLY_ATTR(TYPE),
> +	POWER_SUPPLY_ENUM_ATTR(TYPE),
>  	POWER_SUPPLY_ATTR(USB_TYPE),
> -	POWER_SUPPLY_ATTR(SCOPE),
> +	POWER_SUPPLY_ENUM_ATTR(SCOPE),
>  	POWER_SUPPLY_ATTR(PRECHARGE_CURRENT),
>  	POWER_SUPPLY_ATTR(CHARGE_TERM_CURRENT),
>  	POWER_SUPPLY_ATTR(CALIBRATE),
> @@ -197,10 +207,14 @@ static struct power_supply_attr power_supply_attrs[] = {
>  static struct attribute *
>  __power_supply_attrs[ARRAY_SIZE(power_supply_attrs) + 1];
>  
> +static struct power_supply_attr *to_ps_attr(struct device_attribute *attr)
> +{
> +	return container_of(attr, struct power_supply_attr, dev_attr);
> +}
> +
>  static enum power_supply_property dev_attr_psp(struct device_attribute *attr)
>  {
> -	return container_of(attr, struct power_supply_attr, dev_attr) -
> -		power_supply_attrs;
> +	return  to_ps_attr(attr) - power_supply_attrs;
>  }
>  
>  static ssize_t power_supply_show_usb_type(struct device *dev,
> @@ -219,11 +233,11 @@ static ssize_t power_supply_show_usb_type(struct device *dev,
>  
>  		if (value->intval == usb_type) {
>  			count += sprintf(buf + count, "[%s] ",
> -					 power_supply_usb_type_text[usb_type]);
> +					 POWER_SUPPLY_USB_TYPE_TEXT[usb_type]);
>  			match = true;
>  		} else {
>  			count += sprintf(buf + count, "%s ",
> -					 power_supply_usb_type_text[usb_type]);
> +					 POWER_SUPPLY_USB_TYPE_TEXT[usb_type]);
>  		}
>  	}
>  
> @@ -243,6 +257,7 @@ static ssize_t power_supply_show_property(struct device *dev,
>  					  char *buf) {
>  	ssize_t ret;
>  	struct power_supply *psy = dev_get_drvdata(dev);
> +	struct power_supply_attr *ps_attr = to_ps_attr(attr);
>  	enum power_supply_property psp = dev_attr_psp(attr);
>  	union power_supply_propval value;
>  
> @@ -263,39 +278,16 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		}
>  	}
>  
> +	if (ps_attr->text_values_len > 0 &&
> +	    value.intval < ps_attr->text_values_len && value.intval >= 0) {
> +		return sprintf(buf, "%s\n", ps_attr->text_values[value.intval]);
> +	}
> +
>  	switch (psp) {
> -	case POWER_SUPPLY_PROP_STATUS:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_status_text[value.intval]);
> -		break;
> -	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_charge_type_text[value.intval]);
> -		break;
> -	case POWER_SUPPLY_PROP_HEALTH:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_health_text[value.intval]);
> -		break;
> -	case POWER_SUPPLY_PROP_TECHNOLOGY:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_technology_text[value.intval]);
> -		break;
> -	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_capacity_level_text[value.intval]);
> -		break;
> -	case POWER_SUPPLY_PROP_TYPE:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_type_text[value.intval]);
> -		break;
>  	case POWER_SUPPLY_PROP_USB_TYPE:
>  		ret = power_supply_show_usb_type(dev, psy->desc->usb_types,
> -						 psy->desc->num_usb_types,
> -						 &value, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_SCOPE:
> -		ret = sprintf(buf, "%s\n",
> -			      power_supply_scope_text[value.intval]);
> +						psy->desc->num_usb_types,
> +						&value, buf);
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME ... POWER_SUPPLY_PROP_SERIAL_NUMBER:
>  		ret = sprintf(buf, "%s\n", value.strval);
> @@ -312,30 +304,14 @@ static ssize_t power_supply_store_property(struct device *dev,
>  					   const char *buf, size_t count) {
>  	ssize_t ret;
>  	struct power_supply *psy = dev_get_drvdata(dev);
> +	struct power_supply_attr *ps_attr = to_ps_attr(attr);
>  	enum power_supply_property psp = dev_attr_psp(attr);
>  	union power_supply_propval value;
>  
> -	switch (psp) {
> -	case POWER_SUPPLY_PROP_STATUS:
> -		ret = sysfs_match_string(power_supply_status_text, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> -		ret = sysfs_match_string(power_supply_charge_type_text, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_HEALTH:
> -		ret = sysfs_match_string(power_supply_health_text, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_TECHNOLOGY:
> -		ret = sysfs_match_string(power_supply_technology_text, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
> -		ret = sysfs_match_string(power_supply_capacity_level_text, buf);
> -		break;
> -	case POWER_SUPPLY_PROP_SCOPE:
> -		ret = sysfs_match_string(power_supply_scope_text, buf);
> -		break;
> -	default:
> -		ret = -EINVAL;
> +	ret = -EINVAL;
> +	if (ps_attr->text_values_len > 0) {
> +		ret = __sysfs_match_string(ps_attr->text_values,
> +					   ps_attr->text_values_len, buf);
>  	}
>  
>  	/*
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

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

[PATCH 4/4] power_supply: Add power supply type property to uevent env

[Mathew King]

Add POWER_SUPPLY_TYPE to the uevent env for power supply. Type is a
property of all power supplies and there is a sysfs entry for it but it
is not included in the properties array of the power supply so
explicitly add it to the udev env.

Signed-off-by: Mathew King <mathewk@chromium.org>
---
 drivers/power/supply/power_supply_sysfs.c | 61 ++++++++++++++---------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
index fbb05466b9a5..c7087cd7ffe3 100644
--- a/drivers/power/supply/power_supply_sysfs.c
+++ b/drivers/power/supply/power_supply_sysfs.c
@@ -448,6 +448,37 @@ void power_supply_destroy_attrs(void)
 	}
 }
 
+static int add_prop_uevent(struct device *dev, struct kobj_uevent_env *env,
+			   enum power_supply_property prop, char *prop_buf)
+{
+	int ret = 0;
+	struct power_supply_attr *pwr_attr;
+	struct device_attribute *dev_attr;
+	char *line;
+
+	pwr_attr = &power_supply_attrs[prop];
+	dev_attr = &pwr_attr->dev_attr;
+
+	ret = power_supply_show_property(dev, dev_attr, prop_buf);
+	if (ret == -ENODEV || ret == -ENODATA) {
+		/*
+		 * When a battery is absent, we expect -ENODEV. Don't abort;
+		 * send the uevent with at least the the PRESENT=0 property
+		 */
+		return 0;
+	}
+
+	if (ret < 0)
+		return ret;
+
+	line = strchr(prop_buf, '\n');
+	if (line)
+		*line = 0;
+
+	return add_uevent_var(env, "POWER_SUPPLY_%s=%s",
+			      pwr_attr->upper_name, prop_buf);
+}
+
 int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct power_supply *psy = dev_get_drvdata(dev);
@@ -467,31 +498,13 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
 	if (!prop_buf)
 		return -ENOMEM;
 
-	for (j = 0; j < psy->desc->num_properties; j++) {
-		struct power_supply_attr *pwr_attr;
-		struct device_attribute *dev_attr;
-		char *line;
-
-		pwr_attr = &power_supply_attrs[psy->desc->properties[j]];
-		dev_attr = &pwr_attr->dev_attr;
-
-		ret = power_supply_show_property(dev, dev_attr, prop_buf);
-		if (ret == -ENODEV || ret == -ENODATA) {
-			/* When a battery is absent, we expect -ENODEV. Don't abort;
-			   send the uevent with at least the the PRESENT=0 property */
-			ret = 0;
-			continue;
-		}
-
-		if (ret < 0)
-			goto out;
-
-		line = strchr(prop_buf, '\n');
-		if (line)
-			*line = 0;
+	ret = add_prop_uevent(dev, env, POWER_SUPPLY_PROP_TYPE, prop_buf);
+	if (ret)
+		goto out;
 
-		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s",
-				     pwr_attr->upper_name, prop_buf);
+	for (j = 0; j < psy->desc->num_properties; j++) {
+		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
+				      prop_buf);
 		if (ret)
 			goto out;
 	}
-- 
2.26.2.303.gf8c07b1a785-goog

Re: [PATCH 4/4] power_supply: Add power supply type property to uevent env

[Sebastian Reichel]

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

Hi,

On Fri, Apr 24, 2020 at 11:35:33AM -0600, Mathew King wrote:
> Add POWER_SUPPLY_TYPE to the uevent env for power supply. Type is a
> property of all power supplies and there is a sysfs entry for it but it
> is not included in the properties array of the power supply so
> explicitly add it to the udev env.
> 
> Signed-off-by: Mathew King <mathewk@chromium.org>
> ---

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

-- Sebastian

>  drivers/power/supply/power_supply_sysfs.c | 61 ++++++++++++++---------
>  1 file changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> index fbb05466b9a5..c7087cd7ffe3 100644
> --- a/drivers/power/supply/power_supply_sysfs.c
> +++ b/drivers/power/supply/power_supply_sysfs.c
> @@ -448,6 +448,37 @@ void power_supply_destroy_attrs(void)
>  	}
>  }
>  
> +static int add_prop_uevent(struct device *dev, struct kobj_uevent_env *env,
> +			   enum power_supply_property prop, char *prop_buf)
> +{
> +	int ret = 0;
> +	struct power_supply_attr *pwr_attr;
> +	struct device_attribute *dev_attr;
> +	char *line;
> +
> +	pwr_attr = &power_supply_attrs[prop];
> +	dev_attr = &pwr_attr->dev_attr;
> +
> +	ret = power_supply_show_property(dev, dev_attr, prop_buf);
> +	if (ret == -ENODEV || ret == -ENODATA) {
> +		/*
> +		 * When a battery is absent, we expect -ENODEV. Don't abort;
> +		 * send the uevent with at least the the PRESENT=0 property
> +		 */
> +		return 0;
> +	}
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	line = strchr(prop_buf, '\n');
> +	if (line)
> +		*line = 0;
> +
> +	return add_uevent_var(env, "POWER_SUPPLY_%s=%s",
> +			      pwr_attr->upper_name, prop_buf);
> +}
> +
>  int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
>  	struct power_supply *psy = dev_get_drvdata(dev);
> @@ -467,31 +498,13 @@ int power_supply_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	if (!prop_buf)
>  		return -ENOMEM;
>  
> -	for (j = 0; j < psy->desc->num_properties; j++) {
> -		struct power_supply_attr *pwr_attr;
> -		struct device_attribute *dev_attr;
> -		char *line;
> -
> -		pwr_attr = &power_supply_attrs[psy->desc->properties[j]];
> -		dev_attr = &pwr_attr->dev_attr;
> -
> -		ret = power_supply_show_property(dev, dev_attr, prop_buf);
> -		if (ret == -ENODEV || ret == -ENODATA) {
> -			/* When a battery is absent, we expect -ENODEV. Don't abort;
> -			   send the uevent with at least the the PRESENT=0 property */
> -			ret = 0;
> -			continue;
> -		}
> -
> -		if (ret < 0)
> -			goto out;
> -
> -		line = strchr(prop_buf, '\n');
> -		if (line)
> -			*line = 0;
> +	ret = add_prop_uevent(dev, env, POWER_SUPPLY_PROP_TYPE, prop_buf);
> +	if (ret)
> +		goto out;
>  
> -		ret = add_uevent_var(env, "POWER_SUPPLY_%s=%s",
> -				     pwr_attr->upper_name, prop_buf);
> +	for (j = 0; j < psy->desc->num_properties; j++) {
> +		ret = add_prop_uevent(dev, env, psy->desc->properties[j],
> +				      prop_buf);
>  		if (ret)
>  			goto out;
>  	}
> -- 
> 2.26.2.303.gf8c07b1a785-goog
> 

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