* [PATCH 0/4] Cleanup power_supply_sysfs.c
@ 2020-04-24 17:35 Mathew King
2020-04-24 17:35 ` [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list Mathew King
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Mathew King @ 2020-04-24 17:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Mathew King, Sebastian Reichel, linux-pm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list
2020-04-24 17:35 [PATCH 0/4] Cleanup power_supply_sysfs.c Mathew King
@ 2020-04-24 17:35 ` Mathew King
2020-05-02 0:32 ` Sebastian Reichel
2020-04-24 17:35 ` [PATCH 2/4] power_supply: Use designated initializer for property text arrays Mathew King
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Mathew King @ 2020-04-24 17:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Mathew King, Sebastian Reichel, linux-pm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] power_supply: Use designated initializer for property text arrays
2020-04-24 17:35 [PATCH 0/4] Cleanup power_supply_sysfs.c Mathew King
2020-04-24 17:35 ` [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list Mathew King
@ 2020-04-24 17:35 ` Mathew King
2020-05-02 0:34 ` Sebastian Reichel
2020-04-24 17:35 ` [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values Mathew King
2020-04-24 17:35 ` [PATCH 4/4] power_supply: Add power supply type property to uevent env Mathew King
3 siblings, 1 reply; 10+ messages in thread
From: Mathew King @ 2020-04-24 17:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Mathew King, Sebastian Reichel, linux-pm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values
2020-04-24 17:35 [PATCH 0/4] Cleanup power_supply_sysfs.c Mathew King
2020-04-24 17:35 ` [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list Mathew King
2020-04-24 17:35 ` [PATCH 2/4] power_supply: Use designated initializer for property text arrays Mathew King
@ 2020-04-24 17:35 ` Mathew King
2020-05-03 1:20 ` Sebastian Reichel
2020-04-24 17:35 ` [PATCH 4/4] power_supply: Add power supply type property to uevent env Mathew King
3 siblings, 1 reply; 10+ messages in thread
From: Mathew King @ 2020-04-24 17:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Mathew King, Sebastian Reichel, linux-pm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] power_supply: Add power supply type property to uevent env
2020-04-24 17:35 [PATCH 0/4] Cleanup power_supply_sysfs.c Mathew King
` (2 preceding siblings ...)
2020-04-24 17:35 ` [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values Mathew King
@ 2020-04-24 17:35 ` Mathew King
2020-05-03 1:27 ` Sebastian Reichel
3 siblings, 1 reply; 10+ messages in thread
From: Mathew King @ 2020-04-24 17:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Mathew King, Sebastian Reichel, linux-pm
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list
2020-04-24 17:35 ` [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list Mathew King
@ 2020-05-02 0:32 ` Sebastian Reichel
2020-05-04 20:31 ` Mat King
0 siblings, 1 reply; 10+ messages in thread
From: Sebastian Reichel @ 2020-05-02 0:32 UTC (permalink / raw)
To: Mathew King; +Cc: linux-kernel, linux-pm
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] power_supply: Use designated initializer for property text arrays
2020-04-24 17:35 ` [PATCH 2/4] power_supply: Use designated initializer for property text arrays Mathew King
@ 2020-05-02 0:34 ` Sebastian Reichel
0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2020-05-02 0:34 UTC (permalink / raw)
To: Mathew King; +Cc: linux-kernel, linux-pm
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values
2020-04-24 17:35 ` [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values Mathew King
@ 2020-05-03 1:20 ` Sebastian Reichel
0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2020-05-03 1:20 UTC (permalink / raw)
To: Mathew King; +Cc: linux-kernel, linux-pm
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] power_supply: Add power supply type property to uevent env
2020-04-24 17:35 ` [PATCH 4/4] power_supply: Add power supply type property to uevent env Mathew King
@ 2020-05-03 1:27 ` Sebastian Reichel
0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2020-05-03 1:27 UTC (permalink / raw)
To: Mathew King; +Cc: linux-kernel, linux-pm
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list
2020-05-02 0:32 ` Sebastian Reichel
@ 2020-05-04 20:31 ` Mat King
0 siblings, 0 replies; 10+ messages in thread
From: Mat King @ 2020-05-04 20:31 UTC (permalink / raw)
To: Sebastian Reichel; +Cc: Mathew King, Linux Kernel Mailing List, linux-pm
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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-04 20:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-24 17:35 [PATCH 0/4] Cleanup power_supply_sysfs.c Mathew King
2020-04-24 17:35 ` [PATCH 1/4] power_supply: Cleanup power supply sysfs attribute list Mathew King
2020-05-02 0:32 ` Sebastian Reichel
2020-05-04 20:31 ` Mat King
2020-04-24 17:35 ` [PATCH 2/4] power_supply: Use designated initializer for property text arrays Mathew King
2020-05-02 0:34 ` Sebastian Reichel
2020-04-24 17:35 ` [PATCH 3/4] power_supply: Add a macro that maps enum properties to text values Mathew King
2020-05-03 1:20 ` Sebastian Reichel
2020-04-24 17:35 ` [PATCH 4/4] power_supply: Add power supply type property to uevent env Mathew King
2020-05-03 1:27 ` Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).