[PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API

[PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API

[Corentin Labbe]

Hello

I dont like warning in my boot log such as:
hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
So I did the conversion to the new API.

The patchset was tested with:
- sensors
- cat all values directly in sysfs (for ones not displayed by sensors like oem_info, model_number, etc..)

But due to missing functionnality on my hardware some code path was not
tested. (Like all cap_xxx)

Regards

Change since v1:
- use really the new API

Change since v2:
- fix 32b build by using div_u64()

Corentin Labbe (2):
  hwmon: acpi_power_meter: fix style issue
  hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

 drivers/hwmon/acpi_power_meter.c | 524 +++++++++++++------------------
 1 file changed, 227 insertions(+), 297 deletions(-)

-- 
2.35.1

[PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue

[Corentin Labbe]

Fix style issues found by checkpatch.

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/hwmon/acpi_power_meter.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index c405a5869581..d2545a1be9fc 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -481,7 +481,7 @@ static struct sensor_template meter_attrs[] = {
 	RO_SENSOR_TEMPLATE("power1_average_interval_max", show_val, 1),
 	RO_SENSOR_TEMPLATE("power1_is_battery", show_val, 5),
 	RW_SENSOR_TEMPLATE(POWER_AVG_INTERVAL_NAME, show_avg_interval,
-		set_avg_interval, 0),
+			   set_avg_interval, 0),
 	{},
 };
 
@@ -530,6 +530,7 @@ static void remove_domain_devices(struct acpi_power_meter_resource *resource)
 
 	for (i = 0; i < resource->num_domain_devices; i++) {
 		struct acpi_device *obj = resource->domain_devices[i];
+
 		if (!obj)
 			continue;
 
@@ -580,7 +581,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
 	}
 
 	resource->holders_dir = kobject_create_and_add("measures",
-					&resource->acpi_dev->dev.kobj);
+						       &resource->acpi_dev->dev.kobj);
 	if (!resource->holders_dir) {
 		res = -ENOMEM;
 		goto exit_free;
@@ -590,7 +591,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
 
 	for (i = 0; i < pss->package.count; i++) {
 		struct acpi_device *obj;
-		union acpi_object *element = &(pss->package.elements[i]);
+		union acpi_object *element = &pss->package.elements[i];
 
 		/* Refuse non-references */
 		if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
@@ -603,7 +604,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
 			continue;
 
 		res = sysfs_create_link(resource->holders_dir, &obj->dev.kobj,
-				      kobject_name(&obj->dev.kobj));
+					kobject_name(&obj->dev.kobj));
 		if (res) {
 			acpi_dev_put(obj);
 			resource->domain_devices[i] = NULL;
@@ -788,7 +789,7 @@ static int read_capabilities(struct acpi_power_meter_resource *resource)
 	str = &resource->model_number;
 
 	for (i = 11; i < 14; i++) {
-		union acpi_object *element = &(pss->package.elements[i]);
+		union acpi_object *element = &pss->package.elements[i];
 
 		if (element->type != ACPI_TYPE_STRING) {
 			res = -EINVAL;
@@ -868,8 +869,7 @@ static int acpi_power_meter_add(struct acpi_device *device)
 	if (!device)
 		return -EINVAL;
 
-	resource = kzalloc(sizeof(struct acpi_power_meter_resource),
-			   GFP_KERNEL);
+	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
 	if (!resource)
 		return -ENOMEM;
 
@@ -884,7 +884,8 @@ static int acpi_power_meter_add(struct acpi_device *device)
 	if (res)
 		goto exit_free;
 
-	resource->trip[0] = resource->trip[1] = -1;
+	resource->trip[0] = -1;
+	resource->trip[1] = -1;
 
 	res = setup_attrs(resource);
 	if (res)
-- 
2.35.1

Re: [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue

[Guenter Roeck]

On Mon, May 09, 2022 at 06:30:09AM +0000, Corentin Labbe wrote:
> Fix style issues found by checkpatch.
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/acpi_power_meter.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index c405a5869581..d2545a1be9fc 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -481,7 +481,7 @@ static struct sensor_template meter_attrs[] = {
>  	RO_SENSOR_TEMPLATE("power1_average_interval_max", show_val, 1),
>  	RO_SENSOR_TEMPLATE("power1_is_battery", show_val, 5),
>  	RW_SENSOR_TEMPLATE(POWER_AVG_INTERVAL_NAME, show_avg_interval,
> -		set_avg_interval, 0),
> +			   set_avg_interval, 0),
>  	{},
>  };
>  
> @@ -530,6 +530,7 @@ static void remove_domain_devices(struct acpi_power_meter_resource *resource)
>  
>  	for (i = 0; i < resource->num_domain_devices; i++) {
>  		struct acpi_device *obj = resource->domain_devices[i];
> +
>  		if (!obj)
>  			continue;
>  
> @@ -580,7 +581,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
>  	}
>  
>  	resource->holders_dir = kobject_create_and_add("measures",
> -					&resource->acpi_dev->dev.kobj);
> +						       &resource->acpi_dev->dev.kobj);
>  	if (!resource->holders_dir) {
>  		res = -ENOMEM;
>  		goto exit_free;
> @@ -590,7 +591,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
>  
>  	for (i = 0; i < pss->package.count; i++) {
>  		struct acpi_device *obj;
> -		union acpi_object *element = &(pss->package.elements[i]);
> +		union acpi_object *element = &pss->package.elements[i];
>  
>  		/* Refuse non-references */
>  		if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
> @@ -603,7 +604,7 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
>  			continue;
>  
>  		res = sysfs_create_link(resource->holders_dir, &obj->dev.kobj,
> -				      kobject_name(&obj->dev.kobj));
> +					kobject_name(&obj->dev.kobj));
>  		if (res) {
>  			acpi_dev_put(obj);
>  			resource->domain_devices[i] = NULL;
> @@ -788,7 +789,7 @@ static int read_capabilities(struct acpi_power_meter_resource *resource)
>  	str = &resource->model_number;
>  
>  	for (i = 11; i < 14; i++) {
> -		union acpi_object *element = &(pss->package.elements[i]);
> +		union acpi_object *element = &pss->package.elements[i];
>  
>  		if (element->type != ACPI_TYPE_STRING) {
>  			res = -EINVAL;
> @@ -868,8 +869,7 @@ static int acpi_power_meter_add(struct acpi_device *device)
>  	if (!device)
>  		return -EINVAL;
>  
> -	resource = kzalloc(sizeof(struct acpi_power_meter_resource),
> -			   GFP_KERNEL);
> +	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>  	if (!resource)
>  		return -ENOMEM;
>  
> @@ -884,7 +884,8 @@ static int acpi_power_meter_add(struct acpi_device *device)
>  	if (res)
>  		goto exit_free;
>  
> -	resource->trip[0] = resource->trip[1] = -1;
> +	resource->trip[0] = -1;
> +	resource->trip[1] = -1;
>  
>  	res = setup_attrs(resource);
>  	if (res)

[PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Corentin Labbe]

Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
So let's convert the driver to use hwmon_device_register_with_info().

Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
 drivers/hwmon/acpi_power_meter.c | 509 +++++++++++++------------------
 1 file changed, 219 insertions(+), 290 deletions(-)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index d2545a1be9fc..03a144c884aa 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -23,7 +23,8 @@
 #define ACPI_POWER_METER_DEVICE_NAME	"Power Meter"
 #define ACPI_POWER_METER_CLASS		"pwr_meter_resource"
 
-#define NUM_SENSORS			17
+#define TRIP_MIN 0
+#define TRIP_MAX 1
 
 #define POWER_METER_CAN_MEASURE	(1 << 0)
 #define POWER_METER_CAN_TRIP	(1 << 1)
@@ -38,11 +39,6 @@
 #define METER_NOTIFY_CAPPING	0x83
 #define METER_NOTIFY_INTERVAL	0x84
 
-#define POWER_AVERAGE_NAME	"power1_average"
-#define POWER_CAP_NAME		"power1_cap"
-#define POWER_AVG_INTERVAL_NAME	"power1_average_interval"
-#define POWER_ALARM_NAME	"power1_alarm"
-
 static int cap_in_hardware;
 static bool force_cap_on;
 
@@ -85,8 +81,6 @@ struct acpi_power_meter_resource {
 	u64		avg_interval;
 	int			sensors_valid;
 	unsigned long		sensors_last_updated;
-	struct sensor_device_attribute	sensors[NUM_SENSORS];
-	int			num_sensors;
 	s64			trip[2];
 	int			num_domain_devices;
 	struct acpi_device	**domain_devices;
@@ -122,47 +116,32 @@ static int update_avg_interval(struct acpi_power_meter_resource *resource)
 	return 0;
 }
 
-static ssize_t show_avg_interval(struct device *dev,
-				 struct device_attribute *devattr,
-				 char *buf)
+static int acpi_power_average_interval_read(struct acpi_power_meter_resource *resource)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
-
 	mutex_lock(&resource->lock);
 	update_avg_interval(resource);
 	mutex_unlock(&resource->lock);
 
-	return sprintf(buf, "%llu\n", resource->avg_interval);
+	return resource->avg_interval;
 }
 
-static ssize_t set_avg_interval(struct device *dev,
-				struct device_attribute *devattr,
-				const char *buf, size_t count)
+static int set_average_interval(struct acpi_power_meter_resource *resource, long val)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
-	int res;
-	unsigned long temp;
 	unsigned long long data;
 	acpi_status status;
 
-	res = kstrtoul(buf, 10, &temp);
-	if (res)
-		return res;
-
-	if (temp > resource->caps.max_avg_interval ||
-	    temp < resource->caps.min_avg_interval)
+	if (val > resource->caps.max_avg_interval ||
+	    val < resource->caps.min_avg_interval)
 		return -EINVAL;
-	arg0.integer.value = temp;
+	arg0.integer.value = val;
 
 	mutex_lock(&resource->lock);
 	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_PAI",
 				       &args, &data);
 	if (ACPI_SUCCESS(status))
-		resource->avg_interval = temp;
+		resource->avg_interval = val;
 	mutex_unlock(&resource->lock);
 
 	if (ACPI_FAILURE(status)) {
@@ -175,7 +154,7 @@ static ssize_t set_avg_interval(struct device *dev,
 	if (data)
 		return -EINVAL;
 
-	return count;
+	return 0;
 }
 
 /* Cap functions */
@@ -196,46 +175,33 @@ static int update_cap(struct acpi_power_meter_resource *resource)
 	return 0;
 }
 
-static ssize_t show_cap(struct device *dev,
-			struct device_attribute *devattr,
-			char *buf)
+static int acpi_power_cap_read(struct acpi_power_meter_resource *resource,
+			       long *val)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
-
 	mutex_lock(&resource->lock);
 	update_cap(resource);
 	mutex_unlock(&resource->lock);
 
-	return sprintf(buf, "%llu\n", resource->cap * 1000);
+	return resource->cap * 1000;
 }
 
-static ssize_t set_cap(struct device *dev, struct device_attribute *devattr,
-		       const char *buf, size_t count)
+static int set_cap(struct acpi_power_meter_resource *resource, long val)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list args = { 1, &arg0 };
-	int res;
-	unsigned long temp;
 	unsigned long long data;
 	acpi_status status;
 
-	res = kstrtoul(buf, 10, &temp);
-	if (res)
-		return res;
-
-	temp = DIV_ROUND_CLOSEST(temp, 1000);
-	if (temp > resource->caps.max_cap || temp < resource->caps.min_cap)
+	val = DIV_ROUND_CLOSEST(val, 1000);
+	if (val > resource->caps.max_cap || val < resource->caps.min_cap)
 		return -EINVAL;
-	arg0.integer.value = temp;
+	arg0.integer.value = val;
 
 	mutex_lock(&resource->lock);
 	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_SHL",
 				       &args, &data);
 	if (ACPI_SUCCESS(status))
-		resource->cap = temp;
+		resource->cap = val;
 	mutex_unlock(&resource->lock);
 
 	if (ACPI_FAILURE(status)) {
@@ -248,7 +214,7 @@ static ssize_t set_cap(struct device *dev, struct device_attribute *devattr,
 	if (data)
 		return -EINVAL;
 
-	return count;
+	return 0;
 }
 
 /* Power meter trip points */
@@ -263,12 +229,12 @@ static int set_acpi_trip(struct acpi_power_meter_resource *resource)
 	acpi_status status;
 
 	/* Both trip levels must be set */
-	if (resource->trip[0] < 0 || resource->trip[1] < 0)
+	if (resource->trip[TRIP_MIN] < 0 || resource->trip[TRIP_MAX] < 0)
 		return 0;
 
 	/* This driver stores min, max; ACPI wants max, min. */
-	arg_objs[0].integer.value = resource->trip[1];
-	arg_objs[1].integer.value = resource->trip[0];
+	arg_objs[0].integer.value = resource->trip[TRIP_MAX];
+	arg_objs[1].integer.value = resource->trip[TRIP_MIN];
 
 	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_PTP",
 				       &args, &data);
@@ -285,30 +251,18 @@ static int set_acpi_trip(struct acpi_power_meter_resource *resource)
 	return 0;
 }
 
-static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
-			const char *buf, size_t count)
+static int set_trip(struct acpi_power_meter_resource *resource, long val, int triptype)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
 	int res;
-	unsigned long temp;
 
-	res = kstrtoul(buf, 10, &temp);
-	if (res)
-		return res;
-
-	temp = DIV_ROUND_CLOSEST(temp, 1000);
+	val = DIV_ROUND_CLOSEST(val, 1000);
 
 	mutex_lock(&resource->lock);
-	resource->trip[attr->index - 7] = temp;
+	resource->trip[triptype] = val;
 	res = set_acpi_trip(resource);
 	mutex_unlock(&resource->lock);
 
-	if (res)
-		return res;
-
-	return count;
+	return res;
 }
 
 /* Power meter */
@@ -337,33 +291,26 @@ static int update_meter(struct acpi_power_meter_resource *resource)
 	return 0;
 }
 
-static ssize_t show_power(struct device *dev,
-			  struct device_attribute *devattr,
-			  char *buf)
+static int acpi_power_power_read(struct acpi_power_meter_resource *resource,
+				 long *val)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
-
 	mutex_lock(&resource->lock);
 	update_meter(resource);
 	mutex_unlock(&resource->lock);
 
-	return sprintf(buf, "%llu\n", resource->power * 1000);
+	*val = resource->power * 1000;
+	return 0;
 }
 
 /* Miscellaneous */
-static ssize_t show_str(struct device *dev,
-			struct device_attribute *devattr,
-			char *buf)
+static ssize_t show_str(struct device *dev, int index, char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
+	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
 	acpi_string val;
 	int ret;
 
 	mutex_lock(&resource->lock);
-	switch (attr->index) {
+	switch (index) {
 	case 0:
 		val = resource->model_number;
 		break;
@@ -375,7 +322,7 @@ static ssize_t show_str(struct device *dev,
 		break;
 	default:
 		WARN(1, "Implementation error: unexpected attribute index %d\n",
-		     attr->index);
+		     index);
 		val = "";
 		break;
 	}
@@ -384,141 +331,138 @@ static ssize_t show_str(struct device *dev,
 	return ret;
 }
 
-static ssize_t show_val(struct device *dev,
-			struct device_attribute *devattr,
-			char *buf)
+static ssize_t power1_is_battery_show(struct device *dev,
+				      struct device_attribute *attr,
+				      char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
-	u64 val = 0;
+	int val;
 
-	switch (attr->index) {
-	case 0:
-		val = resource->caps.min_avg_interval;
+	if (resource->caps.flags & POWER_METER_IS_BATTERY)
+		val = 1;
+	else
+		val = 0;
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t power1_model_number_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	return show_str(dev, 0, buf);
+}
+
+static ssize_t power1_serial_number_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	return show_str(dev, 1, buf);
+}
+
+static ssize_t power1_oem_info_show(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	return show_str(dev, 2, buf);
+}
+
+static int acpi_power_read(struct device *dev, enum hwmon_sensor_types type,
+			   u32 attr, int channel, long *val)
+{
+	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_power_average:
+		return acpi_power_power_read(resource, val);
+	case hwmon_power_average_interval_min:
+		*val = resource->caps.min_avg_interval;
 		break;
-	case 1:
-		val = resource->caps.max_avg_interval;
+	case hwmon_power_average_interval_max:
+		*val = resource->caps.max_avg_interval;
 		break;
-	case 2:
-		val = resource->caps.min_cap * 1000;
+	case hwmon_power_cap_min:
+		*val = resource->caps.min_cap * 1000;
 		break;
-	case 3:
-		val = resource->caps.max_cap * 1000;
+	case hwmon_power_cap_max:
+		*val = resource->caps.max_cap * 1000;
 		break;
-	case 4:
+	case hwmon_power_cap:
+		*val = acpi_power_cap_read(resource, val);
+		break;
+	case hwmon_power_cap_hyst:
 		if (resource->caps.hysteresis == UNKNOWN_HYSTERESIS)
-			return sprintf(buf, "unknown\n");
+			return -EINVAL;
 
-		val = resource->caps.hysteresis * 1000;
+		*val = resource->caps.hysteresis * 1000;
 		break;
-	case 5:
-		if (resource->caps.flags & POWER_METER_IS_BATTERY)
-			val = 1;
-		else
-			val = 0;
-		break;
-	case 6:
+	case hwmon_power_alarm:
 		if (resource->power > resource->cap)
-			val = 1;
+			*val = 1;
 		else
-			val = 0;
+			*val = 0;
 		break;
-	case 7:
-	case 8:
-		if (resource->trip[attr->index - 7] < 0)
-			return sprintf(buf, "unknown\n");
-
-		val = resource->trip[attr->index - 7] * 1000;
+	case hwmon_power_average_min:
+		if (resource->trip[TRIP_MIN] < 0)
+			return -EINVAL;
+		*val = resource->trip[TRIP_MIN] * 1000;
+		break;
+	case hwmon_power_average_max:
+		if (resource->trip[TRIP_MAX] < 0)
+			return -EINVAL;
+		*val = resource->trip[TRIP_MAX] * 1000;
+		break;
+	case hwmon_power_average_interval:
+		*val = acpi_power_average_interval_read(resource);
+		break;
+	case hwmon_power_accuracy:
+		*val = div_u64(resource->caps.accuracy, 1000);
 		break;
 	default:
 		WARN(1, "Implementation error: unexpected attribute index %d\n",
-		     attr->index);
-		break;
+		     attr);
+		return -EOPNOTSUPP;
 	}
 
-	return sprintf(buf, "%llu\n", val);
-}
-
-static ssize_t show_accuracy(struct device *dev,
-			     struct device_attribute *devattr,
-			     char *buf)
-{
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
-	unsigned int acc = resource->caps.accuracy;
-
-	return sprintf(buf, "%u.%u%%\n", acc / 1000, acc % 1000);
+	return 0;
 }
 
-static ssize_t show_name(struct device *dev,
-			 struct device_attribute *devattr,
-			 char *buf)
+static int acpi_power_write(struct device *dev, enum hwmon_sensor_types type,
+			    u32 attr, int channel, long val)
 {
-	return sprintf(buf, "%s\n", ACPI_POWER_METER_NAME);
-}
-
-#define RO_SENSOR_TEMPLATE(_label, _show, _index)	\
-	{						\
-		.label = _label,			\
-		.show  = _show,				\
-		.index = _index,			\
-	}
-
-#define RW_SENSOR_TEMPLATE(_label, _show, _set, _index)	\
-	{						\
-		.label = _label,			\
-		.show  = _show,				\
-		.set   = _set,				\
-		.index = _index,			\
+	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_power_average_min:
+		return set_trip(resource, TRIP_MIN, val);
+	case hwmon_power_average_max:
+		return set_trip(resource, TRIP_MAX, val);
+	case hwmon_power_cap:
+		if (resource->caps.configurable_cap)
+			return set_cap(resource, val);
+		else
+			return -EINVAL;
+	case hwmon_power_average_interval:
+		return set_average_interval(resource, val);
+	default:
+		return -EOPNOTSUPP;
 	}
+}
 
-/* Sensor descriptions.  If you add a sensor, update NUM_SENSORS above! */
-static struct sensor_template meter_attrs[] = {
-	RO_SENSOR_TEMPLATE(POWER_AVERAGE_NAME, show_power, 0),
-	RO_SENSOR_TEMPLATE("power1_accuracy", show_accuracy, 0),
-	RO_SENSOR_TEMPLATE("power1_average_interval_min", show_val, 0),
-	RO_SENSOR_TEMPLATE("power1_average_interval_max", show_val, 1),
-	RO_SENSOR_TEMPLATE("power1_is_battery", show_val, 5),
-	RW_SENSOR_TEMPLATE(POWER_AVG_INTERVAL_NAME, show_avg_interval,
-			   set_avg_interval, 0),
-	{},
-};
-
-static struct sensor_template misc_cap_attrs[] = {
-	RO_SENSOR_TEMPLATE("power1_cap_min", show_val, 2),
-	RO_SENSOR_TEMPLATE("power1_cap_max", show_val, 3),
-	RO_SENSOR_TEMPLATE("power1_cap_hyst", show_val, 4),
-	RO_SENSOR_TEMPLATE(POWER_ALARM_NAME, show_val, 6),
-	{},
-};
-
-static struct sensor_template ro_cap_attrs[] = {
-	RO_SENSOR_TEMPLATE(POWER_CAP_NAME, show_cap, 0),
-	{},
-};
-
-static struct sensor_template rw_cap_attrs[] = {
-	RW_SENSOR_TEMPLATE(POWER_CAP_NAME, show_cap, set_cap, 0),
-	{},
-};
-
-static struct sensor_template trip_attrs[] = {
-	RW_SENSOR_TEMPLATE("power1_average_min", show_val, set_trip, 7),
-	RW_SENSOR_TEMPLATE("power1_average_max", show_val, set_trip, 8),
-	{},
-};
-
-static struct sensor_template misc_attrs[] = {
-	RO_SENSOR_TEMPLATE("name", show_name, 0),
-	RO_SENSOR_TEMPLATE("power1_model_number", show_str, 0),
-	RO_SENSOR_TEMPLATE("power1_oem_info", show_str, 2),
-	RO_SENSOR_TEMPLATE("power1_serial_number", show_str, 1),
-	{},
+static DEVICE_ATTR_RO(power1_is_battery);
+static DEVICE_ATTR_RO(power1_model_number);
+static DEVICE_ATTR_RO(power1_oem_info);
+static DEVICE_ATTR_RO(power1_serial_number);
+
+static struct attribute *acpi_power_attrs[] = {
+	&dev_attr_power1_is_battery.attr,
+	&dev_attr_power1_model_number.attr,
+	&dev_attr_power1_oem_info.attr,
+	&dev_attr_power1_serial_number.attr,
+	NULL
 };
 
-#undef RO_SENSOR_TEMPLATE
-#undef RW_SENSOR_TEMPLATE
+ATTRIBUTE_GROUPS(acpi_power);
 
 /* Read power domain data */
 static void remove_domain_devices(struct acpi_power_meter_resource *resource)
@@ -621,55 +565,52 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
 	return res;
 }
 
-/* Registration and deregistration */
-static int register_attrs(struct acpi_power_meter_resource *resource,
-			  struct sensor_template *attrs)
-{
-	struct device *dev = &resource->acpi_dev->dev;
-	struct sensor_device_attribute *sensors =
-		&resource->sensors[resource->num_sensors];
-	int res = 0;
-
-	while (attrs->label) {
-		sensors->dev_attr.attr.name = attrs->label;
-		sensors->dev_attr.attr.mode = 0444;
-		sensors->dev_attr.show = attrs->show;
-		sensors->index = attrs->index;
-
-		if (attrs->set) {
-			sensors->dev_attr.attr.mode |= 0200;
-			sensors->dev_attr.store = attrs->set;
-		}
-
-		sysfs_attr_init(&sensors->dev_attr.attr);
-		res = device_create_file(dev, &sensors->dev_attr);
-		if (res) {
-			sensors->dev_attr.attr.name = NULL;
-			goto error;
-		}
-		sensors++;
-		resource->num_sensors++;
-		attrs++;
-	}
-
-error:
-	return res;
-}
-
-static void remove_attrs(struct acpi_power_meter_resource *resource)
+static umode_t acpi_power_is_visible(const void *data,
+				     enum hwmon_sensor_types type,
+				     u32 attr, int channel)
 {
-	int i;
+	const struct acpi_power_meter_resource *resource = data;
 
-	for (i = 0; i < resource->num_sensors; i++) {
-		if (!resource->sensors[i].dev_attr.attr.name)
-			continue;
-		device_remove_file(&resource->acpi_dev->dev,
-				   &resource->sensors[i].dev_attr);
+	switch (attr) {
+	case hwmon_power_average_min:
+	case hwmon_power_average_max:
+		if (resource->caps.flags & POWER_METER_CAN_TRIP)
+			return 0644;
+		break;
+	case hwmon_power_average:
+	case hwmon_power_accuracy:
+	case hwmon_power_average_interval_min:
+	case hwmon_power_average_interval_max:
+		if (resource->caps.flags & POWER_METER_CAN_MEASURE)
+			return 0444;
+		break;
+	case hwmon_power_average_interval:
+		if (resource->caps.flags & POWER_METER_CAN_MEASURE)
+			return 0644;
+		break;
+	case hwmon_power_cap:
+		if (!can_cap_in_hardware())
+			return 0;
+		if (!(resource->caps.flags & POWER_METER_CAN_CAP))
+			return 0;
+		if (resource->caps.configurable_cap)
+			return 0644;
+		return 0444;
+		break;
+	case hwmon_power_cap_min:
+	case hwmon_power_cap_max:
+	case hwmon_power_cap_hyst:
+	case hwmon_power_cap_alarm:
+		if (!can_cap_in_hardware())
+			return 0;
+		if (resource->caps.flags & POWER_METER_CAN_CAP)
+			return 0444;
+		break;
+	default:
+		break;
 	}
 
-	remove_domain_devices(resource);
-
-	resource->num_sensors = 0;
+	return 0;
 }
 
 static int setup_attrs(struct acpi_power_meter_resource *resource)
@@ -680,47 +621,11 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
 	if (res)
 		return res;
 
-	if (resource->caps.flags & POWER_METER_CAN_MEASURE) {
-		res = register_attrs(resource, meter_attrs);
-		if (res)
-			goto error;
+	if (resource->caps.flags & POWER_METER_CAN_CAP && !can_cap_in_hardware()) {
+		dev_warn(&resource->acpi_dev->dev,
+			 "Ignoring unsafe software power cap!\n");
 	}
-
-	if (resource->caps.flags & POWER_METER_CAN_CAP) {
-		if (!can_cap_in_hardware()) {
-			dev_warn(&resource->acpi_dev->dev,
-				 "Ignoring unsafe software power cap!\n");
-			goto skip_unsafe_cap;
-		}
-
-		if (resource->caps.configurable_cap)
-			res = register_attrs(resource, rw_cap_attrs);
-		else
-			res = register_attrs(resource, ro_cap_attrs);
-
-		if (res)
-			goto error;
-
-		res = register_attrs(resource, misc_cap_attrs);
-		if (res)
-			goto error;
-	}
-
-skip_unsafe_cap:
-	if (resource->caps.flags & POWER_METER_CAN_TRIP) {
-		res = register_attrs(resource, trip_attrs);
-		if (res)
-			goto error;
-	}
-
-	res = register_attrs(resource, misc_attrs);
-	if (res)
-		goto error;
-
-	return res;
-error:
-	remove_attrs(resource);
-	return res;
+	return 0;
 }
 
 static void free_capabilities(struct acpi_power_meter_resource *resource)
@@ -795,7 +700,6 @@ static int read_capabilities(struct acpi_power_meter_resource *resource)
 			res = -EINVAL;
 			goto error;
 		}
-
 		*str = kcalloc(element->string.length + 1, sizeof(u8),
 			       GFP_KERNEL);
 		if (!*str) {
@@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 		if (res)
 			break;
 
-		remove_attrs(resource);
+		remove_domain_devices(resource);
 		setup_attrs(resource);
 		break;
 	case METER_NOTIFY_TRIP:
-		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
+		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
 		break;
 	case METER_NOTIFY_CAP:
-		sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
+		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_cap, 0);
 		break;
 	case METER_NOTIFY_INTERVAL:
-		sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
+		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average_interval, 0);
 		break;
 	case METER_NOTIFY_CAPPING:
-		sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
+		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_alarm, 0);
 		dev_info(&device->dev, "Capping in progress.\n");
 		break;
 	default:
@@ -861,6 +765,28 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 					dev_name(&device->dev), event, 0);
 }
 
+static const struct hwmon_channel_info *acpi_power_info[] = {
+	HWMON_CHANNEL_INFO(power,
+			   HWMON_P_AVERAGE | HWMON_P_AVERAGE_INTERVAL |
+			   HWMON_P_AVERAGE_MIN | HWMON_P_AVERAGE_MAX |
+			   HWMON_P_CAP | HWMON_P_CAP_HYST |
+			   HWMON_P_CAP_MIN | HWMON_P_CAP_MAX |
+			   HWMON_P_ACCURACY
+			   ),
+	NULL,
+};
+
+static const struct hwmon_ops acpi_power_hwmon_ops = {
+	.is_visible = acpi_power_is_visible,
+	.read = acpi_power_read,
+	.write = acpi_power_write,
+};
+
+static const struct hwmon_chip_info acpi_power_chip_info = {
+	.ops = &acpi_power_hwmon_ops,
+	.info = acpi_power_info,
+};
+
 static int acpi_power_meter_add(struct acpi_device *device)
 {
 	int res;
@@ -891,7 +817,10 @@ static int acpi_power_meter_add(struct acpi_device *device)
 	if (res)
 		goto exit_free_capability;
 
-	resource->hwmon_dev = hwmon_device_register(&device->dev);
+	resource->hwmon_dev = hwmon_device_register_with_info(&device->dev,
+							      ACPI_POWER_METER_NAME,
+							      resource, &acpi_power_chip_info,
+							      acpi_power_groups);
 	if (IS_ERR(resource->hwmon_dev)) {
 		res = PTR_ERR(resource->hwmon_dev);
 		goto exit_remove;
@@ -901,7 +830,7 @@ static int acpi_power_meter_add(struct acpi_device *device)
 	goto exit;
 
 exit_remove:
-	remove_attrs(resource);
+	remove_domain_devices(resource);
 exit_free_capability:
 	free_capabilities(resource);
 exit_free:
@@ -920,7 +849,7 @@ static int acpi_power_meter_remove(struct acpi_device *device)
 	resource = acpi_driver_data(device);
 	hwmon_device_unregister(resource->hwmon_dev);
 
-	remove_attrs(resource);
+	remove_domain_devices(resource);
 	free_capabilities(resource);
 
 	kfree(resource);
-- 
2.35.1

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

On Mon, May 09, 2022 at 06:30:10AM +0000, Corentin Labbe wrote:
> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> So let's convert the driver to use hwmon_device_register_with_info().
> 

This is hardly readable, and results in a checkpatch warning.
Please rephrase.

> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  drivers/hwmon/acpi_power_meter.c | 509 +++++++++++++------------------
>  1 file changed, 219 insertions(+), 290 deletions(-)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index d2545a1be9fc..03a144c884aa 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c

The following include files seem unnecessary.

linux/hwmon-sysfs.h
linux/kdev_t.h
linux/sched.h

On the other side,

linux/kobject.h

should be included.

> @@ -23,7 +23,8 @@
>  #define ACPI_POWER_METER_DEVICE_NAME	"Power Meter"
>  #define ACPI_POWER_METER_CLASS		"pwr_meter_resource"
>  
> -#define NUM_SENSORS			17
> +#define TRIP_MIN 0
> +#define TRIP_MAX 1

Please use

#define<space>DEFINE<tab>value

>  
>  #define POWER_METER_CAN_MEASURE	(1 << 0)
>  #define POWER_METER_CAN_TRIP	(1 << 1)
> @@ -38,11 +39,6 @@
>  #define METER_NOTIFY_CAPPING	0x83
>  #define METER_NOTIFY_INTERVAL	0x84
>  
> -#define POWER_AVERAGE_NAME	"power1_average"
> -#define POWER_CAP_NAME		"power1_cap"
> -#define POWER_AVG_INTERVAL_NAME	"power1_average_interval"
> -#define POWER_ALARM_NAME	"power1_alarm"
> -
>  static int cap_in_hardware;
>  static bool force_cap_on;
>  
> @@ -85,8 +81,6 @@ struct acpi_power_meter_resource {
>  	u64		avg_interval;
>  	int			sensors_valid;
>  	unsigned long		sensors_last_updated;
> -	struct sensor_device_attribute	sensors[NUM_SENSORS];
> -	int			num_sensors;
>  	s64			trip[2];
>  	int			num_domain_devices;
>  	struct acpi_device	**domain_devices;

Around here is struct sensor_template which is now unused.
Please remove.

> @@ -122,47 +116,32 @@ static int update_avg_interval(struct acpi_power_meter_resource *resource)
>  	return 0;
>  }
>  
> -static ssize_t show_avg_interval(struct device *dev,
> -				 struct device_attribute *devattr,
> -				 char *buf)
> +static int acpi_power_average_interval_read(struct acpi_power_meter_resource *resource)
>  {
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> -
>  	mutex_lock(&resource->lock);
>  	update_avg_interval(resource);
>  	mutex_unlock(&resource->lock);
>  
> -	return sprintf(buf, "%llu\n", resource->avg_interval);
> +	return resource->avg_interval;
>  }
>  
> -static ssize_t set_avg_interval(struct device *dev,
> -				struct device_attribute *devattr,
> -				const char *buf, size_t count)
> +static int set_average_interval(struct acpi_power_meter_resource *resource, long val)
>  {
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>  	struct acpi_object_list args = { 1, &arg0 };
> -	int res;
> -	unsigned long temp;
>  	unsigned long long data;
>  	acpi_status status;
>  
> -	res = kstrtoul(buf, 10, &temp);
> -	if (res)
> -		return res;
> -
> -	if (temp > resource->caps.max_avg_interval ||
> -	    temp < resource->caps.min_avg_interval)
> +	if (val > resource->caps.max_avg_interval ||
> +	    val < resource->caps.min_avg_interval)
>  		return -EINVAL;
> -	arg0.integer.value = temp;
> +	arg0.integer.value = val;
>  
>  	mutex_lock(&resource->lock);
>  	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_PAI",
>  				       &args, &data);
>  	if (ACPI_SUCCESS(status))
> -		resource->avg_interval = temp;
> +		resource->avg_interval = val;
>  	mutex_unlock(&resource->lock);
>  
>  	if (ACPI_FAILURE(status)) {
> @@ -175,7 +154,7 @@ static ssize_t set_avg_interval(struct device *dev,
>  	if (data)
>  		return -EINVAL;
>  
> -	return count;
> +	return 0;
>  }
>  
>  /* Cap functions */
> @@ -196,46 +175,33 @@ static int update_cap(struct acpi_power_meter_resource *resource)
>  	return 0;
>  }
>  
> -static ssize_t show_cap(struct device *dev,
> -			struct device_attribute *devattr,
> -			char *buf)
> +static int acpi_power_cap_read(struct acpi_power_meter_resource *resource,
> +			       long *val)
>  {
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> -
>  	mutex_lock(&resource->lock);
>  	update_cap(resource);
>  	mutex_unlock(&resource->lock);
>  
> -	return sprintf(buf, "%llu\n", resource->cap * 1000);
> +	return resource->cap * 1000;
>  }
>  
> -static ssize_t set_cap(struct device *dev, struct device_attribute *devattr,
> -		       const char *buf, size_t count)
> +static int set_cap(struct acpi_power_meter_resource *resource, long val)
>  {
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
>  	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>  	struct acpi_object_list args = { 1, &arg0 };
> -	int res;
> -	unsigned long temp;
>  	unsigned long long data;
>  	acpi_status status;
>  
> -	res = kstrtoul(buf, 10, &temp);
> -	if (res)
> -		return res;
> -
> -	temp = DIV_ROUND_CLOSEST(temp, 1000);
> -	if (temp > resource->caps.max_cap || temp < resource->caps.min_cap)
> +	val = DIV_ROUND_CLOSEST(val, 1000);
> +	if (val > resource->caps.max_cap || val < resource->caps.min_cap)
>  		return -EINVAL;
> -	arg0.integer.value = temp;
> +	arg0.integer.value = val;
>  
>  	mutex_lock(&resource->lock);
>  	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_SHL",
>  				       &args, &data);
>  	if (ACPI_SUCCESS(status))
> -		resource->cap = temp;
> +		resource->cap = val;
>  	mutex_unlock(&resource->lock);
>  
>  	if (ACPI_FAILURE(status)) {
> @@ -248,7 +214,7 @@ static ssize_t set_cap(struct device *dev, struct device_attribute *devattr,
>  	if (data)
>  		return -EINVAL;
>  
> -	return count;
> +	return 0;
>  }
>  
>  /* Power meter trip points */
> @@ -263,12 +229,12 @@ static int set_acpi_trip(struct acpi_power_meter_resource *resource)
>  	acpi_status status;
>  
>  	/* Both trip levels must be set */
> -	if (resource->trip[0] < 0 || resource->trip[1] < 0)
> +	if (resource->trip[TRIP_MIN] < 0 || resource->trip[TRIP_MAX] < 0)
>  		return 0;
>  
>  	/* This driver stores min, max; ACPI wants max, min. */
> -	arg_objs[0].integer.value = resource->trip[1];
> -	arg_objs[1].integer.value = resource->trip[0];
> +	arg_objs[0].integer.value = resource->trip[TRIP_MAX];
> +	arg_objs[1].integer.value = resource->trip[TRIP_MIN];
>  
>  	status = acpi_evaluate_integer(resource->acpi_dev->handle, "_PTP",
>  				       &args, &data);
> @@ -285,30 +251,18 @@ static int set_acpi_trip(struct acpi_power_meter_resource *resource)
>  	return 0;
>  }
>  
> -static ssize_t set_trip(struct device *dev, struct device_attribute *devattr,
> -			const char *buf, size_t count)
> +static int set_trip(struct acpi_power_meter_resource *resource, long val, int triptype)
>  {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
>  	int res;
> -	unsigned long temp;
>  
> -	res = kstrtoul(buf, 10, &temp);
> -	if (res)
> -		return res;
> -
> -	temp = DIV_ROUND_CLOSEST(temp, 1000);
> +	val = DIV_ROUND_CLOSEST(val, 1000);
>  
>  	mutex_lock(&resource->lock);
> -	resource->trip[attr->index - 7] = temp;
> +	resource->trip[triptype] = val;
>  	res = set_acpi_trip(resource);
>  	mutex_unlock(&resource->lock);
>  
> -	if (res)
> -		return res;
> -
> -	return count;
> +	return res;
>  }
>  
>  /* Power meter */
> @@ -337,33 +291,26 @@ static int update_meter(struct acpi_power_meter_resource *resource)
>  	return 0;
>  }
>  
> -static ssize_t show_power(struct device *dev,
> -			  struct device_attribute *devattr,
> -			  char *buf)
> +static int acpi_power_power_read(struct acpi_power_meter_resource *resource,
> +				 long *val)
>  {
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> -
>  	mutex_lock(&resource->lock);
>  	update_meter(resource);
>  	mutex_unlock(&resource->lock);
>  
> -	return sprintf(buf, "%llu\n", resource->power * 1000);
> +	*val = resource->power * 1000;
> +	return 0;
>  }
>  
>  /* Miscellaneous */
> -static ssize_t show_str(struct device *dev,
> -			struct device_attribute *devattr,
> -			char *buf)
> +static ssize_t show_str(struct device *dev, int index, char *buf)
>  {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> +	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
>  	acpi_string val;
>  	int ret;
>  
>  	mutex_lock(&resource->lock);
> -	switch (attr->index) {
> +	switch (index) {
>  	case 0:
>  		val = resource->model_number;
>  		break;
> @@ -375,7 +322,7 @@ static ssize_t show_str(struct device *dev,
>  		break;
>  	default:
>  		WARN(1, "Implementation error: unexpected attribute index %d\n",
> -		     attr->index);
> +		     index);
>  		val = "";
>  		break;
>  	}
> @@ -384,141 +331,138 @@ static ssize_t show_str(struct device *dev,
>  	return ret;
>  }
>  
> -static ssize_t show_val(struct device *dev,
> -			struct device_attribute *devattr,
> -			char *buf)
> +static ssize_t power1_is_battery_show(struct device *dev,
> +				      struct device_attribute *attr,
> +				      char *buf)
>  {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	struct acpi_device *acpi_dev = to_acpi_device(dev);
>  	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> -	u64 val = 0;
> +	int val;
>  
> -	switch (attr->index) {
> -	case 0:
> -		val = resource->caps.min_avg_interval;
> +	if (resource->caps.flags & POWER_METER_IS_BATTERY)
> +		val = 1;
> +	else
> +		val = 0;

	val = !!(resource->caps.flags & POWER_METER_IS_BATTERY);

would avoid the if()

> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t power1_model_number_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	return show_str(dev, 0, buf);
> +}
> +
> +static ssize_t power1_serial_number_show(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	return show_str(dev, 1, buf);
> +}
> +
> +static ssize_t power1_oem_info_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	return show_str(dev, 2, buf);
> +}
> +
> +static int acpi_power_read(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long *val)
> +{
> +	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_power_average:
> +		return acpi_power_power_read(resource, val);
> +	case hwmon_power_average_interval_min:
> +		*val = resource->caps.min_avg_interval;
>  		break;
> -	case 1:
> -		val = resource->caps.max_avg_interval;
> +	case hwmon_power_average_interval_max:
> +		*val = resource->caps.max_avg_interval;
>  		break;
> -	case 2:
> -		val = resource->caps.min_cap * 1000;
> +	case hwmon_power_cap_min:
> +		*val = resource->caps.min_cap * 1000;
>  		break;
> -	case 3:
> -		val = resource->caps.max_cap * 1000;
> +	case hwmon_power_cap_max:
> +		*val = resource->caps.max_cap * 1000;
>  		break;
> -	case 4:
> +	case hwmon_power_cap:
> +		*val = acpi_power_cap_read(resource, val);
> +		break;
> +	case hwmon_power_cap_hyst:
>  		if (resource->caps.hysteresis == UNKNOWN_HYSTERESIS)
> -			return sprintf(buf, "unknown\n");
> +			return -EINVAL;

			return -ENODATA;

>  
> -		val = resource->caps.hysteresis * 1000;
> +		*val = resource->caps.hysteresis * 1000;
>  		break;
> -	case 5:
> -		if (resource->caps.flags & POWER_METER_IS_BATTERY)
> -			val = 1;
> -		else
> -			val = 0;
> -		break;
> -	case 6:
> +	case hwmon_power_alarm:
>  		if (resource->power > resource->cap)
> -			val = 1;
> +			*val = 1;
>  		else
> -			val = 0;
> +			*val = 0;

		*val = !!(resource->power > resource->cap);

>  		break;
> -	case 7:
> -	case 8:
> -		if (resource->trip[attr->index - 7] < 0)
> -			return sprintf(buf, "unknown\n");
> -
> -		val = resource->trip[attr->index - 7] * 1000;
> +	case hwmon_power_average_min:
> +		if (resource->trip[TRIP_MIN] < 0)
> +			return -EINVAL;

			return -ENODATA;

> +		*val = resource->trip[TRIP_MIN] * 1000;
> +		break;
> +	case hwmon_power_average_max:
> +		if (resource->trip[TRIP_MAX] < 0)
> +			return -EINVAL;

			return -ENODATA;

> +		*val = resource->trip[TRIP_MAX] * 1000;
> +		break;
> +	case hwmon_power_average_interval:
> +		*val = acpi_power_average_interval_read(resource);
> +		break;
> +	case hwmon_power_accuracy:
> +		*val = div_u64(resource->caps.accuracy, 1000);
>  		break;
>  	default:
>  		WARN(1, "Implementation error: unexpected attribute index %d\n",
> -		     attr->index);
> -		break;
> +		     attr);
> +		return -EOPNOTSUPP;
>  	}
>  
> -	return sprintf(buf, "%llu\n", val);
> -}
> -
> -static ssize_t show_accuracy(struct device *dev,
> -			     struct device_attribute *devattr,
> -			     char *buf)
> -{
> -	struct acpi_device *acpi_dev = to_acpi_device(dev);
> -	struct acpi_power_meter_resource *resource = acpi_dev->driver_data;
> -	unsigned int acc = resource->caps.accuracy;
> -
> -	return sprintf(buf, "%u.%u%%\n", acc / 1000, acc % 1000);
> +	return 0;
>  }
>  
> -static ssize_t show_name(struct device *dev,
> -			 struct device_attribute *devattr,
> -			 char *buf)
> +static int acpi_power_write(struct device *dev, enum hwmon_sensor_types type,
> +			    u32 attr, int channel, long val)
>  {
> -	return sprintf(buf, "%s\n", ACPI_POWER_METER_NAME);
> -}
> -
> -#define RO_SENSOR_TEMPLATE(_label, _show, _index)	\
> -	{						\
> -		.label = _label,			\
> -		.show  = _show,				\
> -		.index = _index,			\
> -	}
> -
> -#define RW_SENSOR_TEMPLATE(_label, _show, _set, _index)	\
> -	{						\
> -		.label = _label,			\
> -		.show  = _show,				\
> -		.set   = _set,				\
> -		.index = _index,			\
> +	struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_power_average_min:
> +		return set_trip(resource, TRIP_MIN, val);
> +	case hwmon_power_average_max:
> +		return set_trip(resource, TRIP_MAX, val);
> +	case hwmon_power_cap:
> +		if (resource->caps.configurable_cap)
> +			return set_cap(resource, val);
> +		else

else after return is unnecessary.

> +			return -EINVAL;
> +	case hwmon_power_average_interval:
> +		return set_average_interval(resource, val);
> +	default:
> +		return -EOPNOTSUPP;
>  	}
> +}
>  
> -/* Sensor descriptions.  If you add a sensor, update NUM_SENSORS above! */
> -static struct sensor_template meter_attrs[] = {
> -	RO_SENSOR_TEMPLATE(POWER_AVERAGE_NAME, show_power, 0),
> -	RO_SENSOR_TEMPLATE("power1_accuracy", show_accuracy, 0),
> -	RO_SENSOR_TEMPLATE("power1_average_interval_min", show_val, 0),
> -	RO_SENSOR_TEMPLATE("power1_average_interval_max", show_val, 1),
> -	RO_SENSOR_TEMPLATE("power1_is_battery", show_val, 5),
> -	RW_SENSOR_TEMPLATE(POWER_AVG_INTERVAL_NAME, show_avg_interval,
> -			   set_avg_interval, 0),
> -	{},
> -};
> -
> -static struct sensor_template misc_cap_attrs[] = {
> -	RO_SENSOR_TEMPLATE("power1_cap_min", show_val, 2),
> -	RO_SENSOR_TEMPLATE("power1_cap_max", show_val, 3),
> -	RO_SENSOR_TEMPLATE("power1_cap_hyst", show_val, 4),
> -	RO_SENSOR_TEMPLATE(POWER_ALARM_NAME, show_val, 6),
> -	{},
> -};
> -
> -static struct sensor_template ro_cap_attrs[] = {
> -	RO_SENSOR_TEMPLATE(POWER_CAP_NAME, show_cap, 0),
> -	{},
> -};
> -
> -static struct sensor_template rw_cap_attrs[] = {
> -	RW_SENSOR_TEMPLATE(POWER_CAP_NAME, show_cap, set_cap, 0),
> -	{},
> -};
> -
> -static struct sensor_template trip_attrs[] = {
> -	RW_SENSOR_TEMPLATE("power1_average_min", show_val, set_trip, 7),
> -	RW_SENSOR_TEMPLATE("power1_average_max", show_val, set_trip, 8),
> -	{},
> -};
> -
> -static struct sensor_template misc_attrs[] = {
> -	RO_SENSOR_TEMPLATE("name", show_name, 0),
> -	RO_SENSOR_TEMPLATE("power1_model_number", show_str, 0),
> -	RO_SENSOR_TEMPLATE("power1_oem_info", show_str, 2),
> -	RO_SENSOR_TEMPLATE("power1_serial_number", show_str, 1),
> -	{},
> +static DEVICE_ATTR_RO(power1_is_battery);
> +static DEVICE_ATTR_RO(power1_model_number);
> +static DEVICE_ATTR_RO(power1_oem_info);
> +static DEVICE_ATTR_RO(power1_serial_number);
> +
> +static struct attribute *acpi_power_attrs[] = {
> +	&dev_attr_power1_is_battery.attr,
> +	&dev_attr_power1_model_number.attr,
> +	&dev_attr_power1_oem_info.attr,
> +	&dev_attr_power1_serial_number.attr,
> +	NULL
>  };
>  
> -#undef RO_SENSOR_TEMPLATE
> -#undef RW_SENSOR_TEMPLATE
> +ATTRIBUTE_GROUPS(acpi_power);
>  
>  /* Read power domain data */
>  static void remove_domain_devices(struct acpi_power_meter_resource *resource)
> @@ -621,55 +565,52 @@ static int read_domain_devices(struct acpi_power_meter_resource *resource)
>  	return res;
>  }
>  
> -/* Registration and deregistration */
> -static int register_attrs(struct acpi_power_meter_resource *resource,
> -			  struct sensor_template *attrs)
> -{
> -	struct device *dev = &resource->acpi_dev->dev;
> -	struct sensor_device_attribute *sensors =
> -		&resource->sensors[resource->num_sensors];
> -	int res = 0;
> -
> -	while (attrs->label) {
> -		sensors->dev_attr.attr.name = attrs->label;
> -		sensors->dev_attr.attr.mode = 0444;
> -		sensors->dev_attr.show = attrs->show;
> -		sensors->index = attrs->index;
> -
> -		if (attrs->set) {
> -			sensors->dev_attr.attr.mode |= 0200;
> -			sensors->dev_attr.store = attrs->set;
> -		}
> -
> -		sysfs_attr_init(&sensors->dev_attr.attr);
> -		res = device_create_file(dev, &sensors->dev_attr);
> -		if (res) {
> -			sensors->dev_attr.attr.name = NULL;
> -			goto error;
> -		}
> -		sensors++;
> -		resource->num_sensors++;
> -		attrs++;
> -	}
> -
> -error:
> -	return res;
> -}
> -
> -static void remove_attrs(struct acpi_power_meter_resource *resource)
> +static umode_t acpi_power_is_visible(const void *data,
> +				     enum hwmon_sensor_types type,
> +				     u32 attr, int channel)
>  {
> -	int i;
> +	const struct acpi_power_meter_resource *resource = data;
>  
> -	for (i = 0; i < resource->num_sensors; i++) {
> -		if (!resource->sensors[i].dev_attr.attr.name)
> -			continue;
> -		device_remove_file(&resource->acpi_dev->dev,
> -				   &resource->sensors[i].dev_attr);
> +	switch (attr) {
> +	case hwmon_power_average_min:
> +	case hwmon_power_average_max:
> +		if (resource->caps.flags & POWER_METER_CAN_TRIP)
> +			return 0644;
> +		break;
> +	case hwmon_power_average:
> +	case hwmon_power_accuracy:
> +	case hwmon_power_average_interval_min:
> +	case hwmon_power_average_interval_max:
> +		if (resource->caps.flags & POWER_METER_CAN_MEASURE)
> +			return 0444;
> +		break;
> +	case hwmon_power_average_interval:
> +		if (resource->caps.flags & POWER_METER_CAN_MEASURE)
> +			return 0644;
> +		break;
> +	case hwmon_power_cap:
> +		if (!can_cap_in_hardware())
> +			return 0;
> +		if (!(resource->caps.flags & POWER_METER_CAN_CAP))
> +			return 0;
> +		if (resource->caps.configurable_cap)
> +			return 0644;
> +		return 0444;
> +		break;

Drop "break;" after return.

> +	case hwmon_power_cap_min:
> +	case hwmon_power_cap_max:
> +	case hwmon_power_cap_hyst:
> +	case hwmon_power_cap_alarm:
> +		if (!can_cap_in_hardware())
> +			return 0;
> +		if (resource->caps.flags & POWER_METER_CAN_CAP)
> +			return 0444;

Consistency would be nice: In the above code the condition is negated.

> +		break;
> +	default:
> +		break;
>  	}
>  
> -	remove_domain_devices(resource);
> -
> -	resource->num_sensors = 0;
> +	return 0;
>  }
>  
>  static int setup_attrs(struct acpi_power_meter_resource *resource)
> @@ -680,47 +621,11 @@ static int setup_attrs(struct acpi_power_meter_resource *resource)
>  	if (res)
>  		return res;
>  
> -	if (resource->caps.flags & POWER_METER_CAN_MEASURE) {
> -		res = register_attrs(resource, meter_attrs);
> -		if (res)
> -			goto error;
> +	if (resource->caps.flags & POWER_METER_CAN_CAP && !can_cap_in_hardware()) {
> +		dev_warn(&resource->acpi_dev->dev,
> +			 "Ignoring unsafe software power cap!\n");
>  	}
> -
> -	if (resource->caps.flags & POWER_METER_CAN_CAP) {
> -		if (!can_cap_in_hardware()) {
> -			dev_warn(&resource->acpi_dev->dev,
> -				 "Ignoring unsafe software power cap!\n");
> -			goto skip_unsafe_cap;
> -		}
> -
> -		if (resource->caps.configurable_cap)
> -			res = register_attrs(resource, rw_cap_attrs);
> -		else
> -			res = register_attrs(resource, ro_cap_attrs);
> -
> -		if (res)
> -			goto error;
> -
> -		res = register_attrs(resource, misc_cap_attrs);
> -		if (res)
> -			goto error;
> -	}
> -
> -skip_unsafe_cap:
> -	if (resource->caps.flags & POWER_METER_CAN_TRIP) {
> -		res = register_attrs(resource, trip_attrs);
> -		if (res)
> -			goto error;
> -	}
> -
> -	res = register_attrs(resource, misc_attrs);
> -	if (res)
> -		goto error;
> -
> -	return res;
> -error:
> -	remove_attrs(resource);
> -	return res;
> +	return 0;
>  }
>  
>  static void free_capabilities(struct acpi_power_meter_resource *resource)
> @@ -795,7 +700,6 @@ static int read_capabilities(struct acpi_power_meter_resource *resource)
>  			res = -EINVAL;
>  			goto error;
>  		}
> -
>  		*str = kcalloc(element->string.length + 1, sizeof(u8),
>  			       GFP_KERNEL);
>  		if (!*str) {
> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>  		if (res)
>  			break;
>  
> -		remove_attrs(resource);
> +		remove_domain_devices(resource);
>  		setup_attrs(resource);
>  		break;
>  	case METER_NOTIFY_TRIP:
> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>  		break;
>  	case METER_NOTIFY_CAP:
> -		sysfs_notify(&device->dev.kobj, NULL, POWER_CAP_NAME);
> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_cap, 0);
>  		break;
>  	case METER_NOTIFY_INTERVAL:
> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVG_INTERVAL_NAME);
> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average_interval, 0);
>  		break;
>  	case METER_NOTIFY_CAPPING:
> -		sysfs_notify(&device->dev.kobj, NULL, POWER_ALARM_NAME);
> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_alarm, 0);
>  		dev_info(&device->dev, "Capping in progress.\n");
>  		break;
>  	default:
> @@ -861,6 +765,28 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>  					dev_name(&device->dev), event, 0);
>  }
>  
> +static const struct hwmon_channel_info *acpi_power_info[] = {
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_AVERAGE | HWMON_P_AVERAGE_INTERVAL |
> +			   HWMON_P_AVERAGE_MIN | HWMON_P_AVERAGE_MAX |
> +			   HWMON_P_CAP | HWMON_P_CAP_HYST |
> +			   HWMON_P_CAP_MIN | HWMON_P_CAP_MAX |
> +			   HWMON_P_ACCURACY
> +			   ),
> +	NULL,
> +};
> +
> +static const struct hwmon_ops acpi_power_hwmon_ops = {
> +	.is_visible = acpi_power_is_visible,
> +	.read = acpi_power_read,
> +	.write = acpi_power_write,
> +};
> +
> +static const struct hwmon_chip_info acpi_power_chip_info = {
> +	.ops = &acpi_power_hwmon_ops,
> +	.info = acpi_power_info,
> +};
> +
>  static int acpi_power_meter_add(struct acpi_device *device)
>  {
>  	int res;
> @@ -891,7 +817,10 @@ static int acpi_power_meter_add(struct acpi_device *device)
>  	if (res)
>  		goto exit_free_capability;
>  
> -	resource->hwmon_dev = hwmon_device_register(&device->dev);
> +	resource->hwmon_dev = hwmon_device_register_with_info(&device->dev,
> +							      ACPI_POWER_METER_NAME,
> +							      resource, &acpi_power_chip_info,
> +							      acpi_power_groups);
>  	if (IS_ERR(resource->hwmon_dev)) {
>  		res = PTR_ERR(resource->hwmon_dev);
>  		goto exit_remove;
> @@ -901,7 +830,7 @@ static int acpi_power_meter_add(struct acpi_device *device)
>  	goto exit;
>  
>  exit_remove:
> -	remove_attrs(resource);
> +	remove_domain_devices(resource);
>  exit_free_capability:
>  	free_capabilities(resource);
>  exit_free:
> @@ -920,7 +849,7 @@ static int acpi_power_meter_remove(struct acpi_device *device)
>  	resource = acpi_driver_data(device);
>  	hwmon_device_unregister(resource->hwmon_dev);
>  
> -	remove_attrs(resource);
> +	remove_domain_devices(resource);
>  	free_capabilities(resource);
>  
>  	kfree(resource);

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

Corentin,

On 5/8/22 23:30, Corentin Labbe wrote:
> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> So let's convert the driver to use hwmon_device_register_with_info().
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
[ ... ]

> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>   		if (res)
>   			break;
>   
> -		remove_attrs(resource);
> +		remove_domain_devices(resource);
>   		setup_attrs(resource);

Zhang Rui found an interesting problem with this code:
It needs a call to sysfs_update_groups(hwmon_dev->groups)
to update sysfs attribute visibility, probably between
remove_domain_devices() and setup_attrs().

>   		break;
>   	case METER_NOTIFY_TRIP:
> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);

... which makes realize: The notification device should be the hwmon device.
That would be resource->hwmon_dev, not the acpi device.

Thanks,
Guenter

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[LABBE Corentin]

Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> Corentin,
> 
> On 5/8/22 23:30, Corentin Labbe wrote:
> > Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> > So let's convert the driver to use hwmon_device_register_with_info().
> > 
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> [ ... ]
> 
> > @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >   		if (res)
> >   			break;
> >   
> > -		remove_attrs(resource);
> > +		remove_domain_devices(resource);
> >   		setup_attrs(resource);
> 
> Zhang Rui found an interesting problem with this code:
> It needs a call to sysfs_update_groups(hwmon_dev->groups)
> to update sysfs attribute visibility, probably between
> remove_domain_devices() and setup_attrs().
> 
> >   		break;
> >   	case METER_NOTIFY_TRIP:
> > -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> > +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> 
> ... which makes realize: The notification device should be the hwmon device.
> That would be resource->hwmon_dev, not the acpi device.
> 

Hello

I will fix this, but do you have an example how to test thoses code path easily ?

Thanks

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Zhang Rui]

On Fri, 2022-05-13 at 10:02 +0200, LABBE Corentin wrote:
> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> > Corentin,
> > 
> > On 5/8/22 23:30, Corentin Labbe wrote:
> > > Booting lead to a hwmon_device_register() is deprecated. Please
> > > convert the driver to use hwmon_device_register_with_info().
> > > So let's convert the driver to use
> > > hwmon_device_register_with_info().
> > > 
> > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > > ---
> > 
> > [ ... ]
> > 
> > > @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct
> > > acpi_device *device, u32 event)
> > >   		if (res)
> > >   			break;
> > >   
> > > -		remove_attrs(resource);
> > > +		remove_domain_devices(resource);
> > >   		setup_attrs(resource);
> > 
> > Zhang Rui found an interesting problem with this code:
> > It needs a call to sysfs_update_groups(hwmon_dev->groups)
> > to update sysfs attribute visibility, probably between
> > remove_domain_devices() and setup_attrs().
> > 
> > >   		break;
> > >   	case METER_NOTIFY_TRIP:
> > > -		sysfs_notify(&device->dev.kobj, NULL,
> > > POWER_AVERAGE_NAME);
> > > +		hwmon_notify_event(&device->dev, hwmon_power,
> > > hwmon_power_average, 0);
> > 
> > ... which makes realize: The notification device should be the
> > hwmon device.
> > That would be resource->hwmon_dev, not the acpi device.
> > 
> 
> Hello
> 
> I will fix this, but do you have an example how to test thoses code
> path easily ?

No, I don't have one.

-rui

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

On 5/13/22 01:02, LABBE Corentin wrote:
> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>> Corentin,
>>
>> On 5/8/22 23:30, Corentin Labbe wrote:
>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>
>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>> ---
>> [ ... ]
>>
>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>    		if (res)
>>>    			break;
>>>    
>>> -		remove_attrs(resource);
>>> +		remove_domain_devices(resource);
>>>    		setup_attrs(resource);
>>
>> Zhang Rui found an interesting problem with this code:
>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>> to update sysfs attribute visibility, probably between
>> remove_domain_devices() and setup_attrs().
>>
>>>    		break;
>>>    	case METER_NOTIFY_TRIP:
>>> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>
>> ... which makes realize: The notification device should be the hwmon device.
>> That would be resource->hwmon_dev, not the acpi device.
>>
> 
> Hello
> 
> I will fix this, but do you have an example how to test thoses code path easily ?
> 

No, sorry.

Guenter

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[LABBE Corentin]

Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> Corentin,
> 
> On 5/8/22 23:30, Corentin Labbe wrote:
> > Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> > So let's convert the driver to use hwmon_device_register_with_info().
> > 
> > Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> > ---
> [ ... ]
> 
> > @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >   		if (res)
> >   			break;
> >   
> > -		remove_attrs(resource);
> > +		remove_domain_devices(resource);
> >   		setup_attrs(resource);
> 
> Zhang Rui found an interesting problem with this code:
> It needs a call to sysfs_update_groups(hwmon_dev->groups)
> to update sysfs attribute visibility, probably between
> remove_domain_devices() and setup_attrs().
> 
> >   		break;
> >   	case METER_NOTIFY_TRIP:
> > -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> > +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> 
> ... which makes realize: The notification device should be the hwmon device.
> That would be resource->hwmon_dev, not the acpi device.
> 

Hello

Since my hardware lacks capabilities testing this, I have emulated it on qemu:
https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b

I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).

For testing config change I have tried lot of way:
                res = read_capabilities(resource);
@@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 
                remove_domain_devices(resource);
                setup_attrs(resource);
+               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
+               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
                break;
        case METER_NOTIFY_TRIP:
-               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
                break;
        case METER_NOTIFY_CAP:
-               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_cap, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
                break;
        case METER_NOTIFY_INTERVAL:
-               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average_interval, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average_interval, 0);
                break;
        case METER_NOTIFY_CAPPING:
-               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_alarm, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_alarm, 0);
                dev_info(&device->dev, "Capping in progress.\n");
                break;
        default:

But nothing force visibility to be rerun.

Any idea on how to force visibility to be re-run ?

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

On 5/15/22 12:36, LABBE Corentin wrote:
> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>> Corentin,
>>
>> On 5/8/22 23:30, Corentin Labbe wrote:
>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>
>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>> ---
>> [ ... ]
>>
>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>    		if (res)
>>>    			break;
>>>    
>>> -		remove_attrs(resource);
>>> +		remove_domain_devices(resource);
>>>    		setup_attrs(resource);
>>
>> Zhang Rui found an interesting problem with this code:
>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>> to update sysfs attribute visibility, probably between
>> remove_domain_devices() and setup_attrs().
>>
>>>    		break;
>>>    	case METER_NOTIFY_TRIP:
>>> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>
>> ... which makes realize: The notification device should be the hwmon device.
>> That would be resource->hwmon_dev, not the acpi device.
>>
> 
> Hello
> 
> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
> 
> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
> 
> For testing config change I have tried lot of way:
>                  res = read_capabilities(resource);
> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>   
>                  remove_domain_devices(resource);
>                  setup_attrs(resource);
> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);

Did you add a debug log here ?

acpi_power_groups would be the wrong parameter for sysfs_update_groups().
It would have to be resource->hwmon_dev->groups.

Guenter

>                  break;
>          case METER_NOTIFY_TRIP:
> -               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>                  break;
>          case METER_NOTIFY_CAP:
> -               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_cap, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>                  break;
>          case METER_NOTIFY_INTERVAL:
> -               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average_interval, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average_interval, 0);
>                  break;
>          case METER_NOTIFY_CAPPING:
> -               hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_alarm, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_alarm, 0);
>                  dev_info(&device->dev, "Capping in progress.\n");
>                  break;
>          default:
> 
> But nothing force visibility to be rerun.
> 
> Any idea on how to force visibility to be re-run ?

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[LABBE Corentin]

Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
> On 5/15/22 12:36, LABBE Corentin wrote:
> > Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> >> Corentin,
> >>
> >> On 5/8/22 23:30, Corentin Labbe wrote:
> >>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> >>> So let's convert the driver to use hwmon_device_register_with_info().
> >>>
> >>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> >>> ---
> >> [ ... ]
> >>
> >>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>>    		if (res)
> >>>    			break;
> >>>    
> >>> -		remove_attrs(resource);
> >>> +		remove_domain_devices(resource);
> >>>    		setup_attrs(resource);
> >>
> >> Zhang Rui found an interesting problem with this code:
> >> It needs a call to sysfs_update_groups(hwmon_dev->groups)
> >> to update sysfs attribute visibility, probably between
> >> remove_domain_devices() and setup_attrs().
> >>
> >>>    		break;
> >>>    	case METER_NOTIFY_TRIP:
> >>> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> >>> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> >>
> >> ... which makes realize: The notification device should be the hwmon device.
> >> That would be resource->hwmon_dev, not the acpi device.
> >>
> > 
> > Hello
> > 
> > Since my hardware lacks capabilities testing this, I have emulated it on qemu:
> > https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
> > 
> > I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
> > 
> > For testing config change I have tried lot of way:
> >                  res = read_capabilities(resource);
> > @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >   
> >                  remove_domain_devices(resource);
> >                  setup_attrs(resource);
> > +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
> > +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
> > +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> > +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
> 
> Did you add a debug log here ?

Yes I added debug log to check what is called.

> 
> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
> It would have to be resource->hwmon_dev->groups.
> 

Even with that, no call to is_visible:
@@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 
                remove_domain_devices(resource);
                setup_attrs(resource);
+               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
+               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
+               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
                break;

I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
So perhaps it explain why it is never called.

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

On 5/15/22 23:21, LABBE Corentin wrote:
> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
>> On 5/15/22 12:36, LABBE Corentin wrote:
>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>>>> Corentin,
>>>>
>>>> On 5/8/22 23:30, Corentin Labbe wrote:
>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>>>
>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>>>> ---
>>>> [ ... ]
>>>>
>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>>     		if (res)
>>>>>     			break;
>>>>>     
>>>>> -		remove_attrs(resource);
>>>>> +		remove_domain_devices(resource);
>>>>>     		setup_attrs(resource);
>>>>
>>>> Zhang Rui found an interesting problem with this code:
>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>>>> to update sysfs attribute visibility, probably between
>>>> remove_domain_devices() and setup_attrs().
>>>>
>>>>>     		break;
>>>>>     	case METER_NOTIFY_TRIP:
>>>>> -		sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>>>> +		hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>>>
>>>> ... which makes realize: The notification device should be the hwmon device.
>>>> That would be resource->hwmon_dev, not the acpi device.
>>>>
>>>
>>> Hello
>>>
>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
>>>
>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
>>>
>>> For testing config change I have tried lot of way:
>>>                   res = read_capabilities(resource);
>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>    
>>>                   remove_domain_devices(resource);
>>>                   setup_attrs(resource);
>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>
>> Did you add a debug log here ?
> 
> Yes I added debug log to check what is called.
> 
>>
>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
>> It would have to be resource->hwmon_dev->groups.
>>
> 
> Even with that, no call to is_visible:
> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>   
>                  remove_domain_devices(resource);
>                  setup_attrs(resource);
> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>                  break;
> 
> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
> So perhaps it explain why it is never called.

Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
Effectively that means we'll have to rework the hwmon core to generate attributes
anyway and leave it up to the driver core to call the is_visible function.

Guenter

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

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

On 5/16/22 05:21, Guenter Roeck wrote:
> On 5/15/22 23:21, LABBE Corentin wrote:
>> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
>>> On 5/15/22 12:36, LABBE Corentin wrote:
>>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>>>>> Corentin,
>>>>>
>>>>> On 5/8/22 23:30, Corentin Labbe wrote:
>>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>>>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>>>>
>>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>>>>> ---
>>>>> [ ... ]
>>>>>
>>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>>>             if (res)
>>>>>>                 break;
>>>>>> -        remove_attrs(resource);
>>>>>> +        remove_domain_devices(resource);
>>>>>>             setup_attrs(resource);
>>>>>
>>>>> Zhang Rui found an interesting problem with this code:
>>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>>>>> to update sysfs attribute visibility, probably between
>>>>> remove_domain_devices() and setup_attrs().
>>>>>
>>>>>>             break;
>>>>>>         case METER_NOTIFY_TRIP:
>>>>>> -        sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>>>>> +        hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>>>>
>>>>> ... which makes realize: The notification device should be the hwmon device.
>>>>> That would be resource->hwmon_dev, not the acpi device.
>>>>>
>>>>
>>>> Hello
>>>>
>>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
>>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
>>>>
>>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
>>>>
>>>> For testing config change I have tried lot of way:
>>>>                   res = read_capabilities(resource);
>>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>                   remove_domain_devices(resource);
>>>>                   setup_attrs(resource);
>>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
>>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>>
>>> Did you add a debug log here ?
>>
>> Yes I added debug log to check what is called.
>>
>>>
>>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
>>> It would have to be resource->hwmon_dev->groups.
>>>
>>
>> Even with that, no call to is_visible:
>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>                  remove_domain_devices(resource);
>>                  setup_attrs(resource);
>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>                  break;
>>
>> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
>> So perhaps it explain why it is never called.
> 
> Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
> Effectively that means we'll have to rework the hwmon core to generate attributes
> anyway and leave it up to the driver core to call the is_visible function.
> 

Attached is an outline of what would be needed in the hwmon core.
Completely untested. I wonder if it may be easier to always
create all attributes and have them return -ENODATA if not
supported.

Guenter

[-- Attachment #2: 0001-hwmon-Implement-hwmon_update_groups.patch --]
[-- Type: text/x-patch, Size: 6332 bytes --]

From 0ff1a316c0f0ca36c49f2415f41d49ef1581808d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Fri, 20 May 2022 21:54:48 -0700
Subject: [PATCH] hwmon: Implement hwmon_update_groups()

In some situations the visibility of hwmon sysfs attributes may change.
Support this by providing a new API function hwmon_update_groups().

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/hwmon.c | 95 +++++++++++++++++++++++++++++++++----------
 include/linux/hwmon.h |  2 +
 2 files changed, 76 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 2e2cd79d89eb..d842ab607ccf 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -248,7 +248,19 @@ static int hwmon_thermal_add_sensor(struct device *dev, int index)
 	return 0;
 }
 
-static int hwmon_thermal_register_sensors(struct device *dev)
+static struct hwmon_thermal_data *hwmon_thermal_find_tz(struct device *dev, int index)
+{
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
+	struct hwmon_thermal_data *tzdata;
+
+	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
+		if (tzdata->index == index)
+			return tzdata;
+	}
+	return NULL;
+}
+
+static int hwmon_thermal_register_sensors(struct device *dev, bool update)
 {
 	struct hwmon_device *hwdev = to_hwmon_device(dev);
 	const struct hwmon_chip_info *chip = hwdev->chip;
@@ -263,16 +275,30 @@ static int hwmon_thermal_register_sensors(struct device *dev)
 			continue;
 
 		for (j = 0; info[i]->config[j]; j++) {
+			umode_t mode;
 			int err;
 
-			if (!(info[i]->config[j] & HWMON_T_INPUT) ||
-			    !chip->ops->is_visible(drvdata, hwmon_temp,
-						   hwmon_temp_input, j))
+			if (!(info[i]->config[j] & HWMON_T_INPUT))
 				continue;
-
-			err = hwmon_thermal_add_sensor(dev, j);
-			if (err)
-				return err;
+			mode = chip->ops->is_visible(drvdata, hwmon_temp,
+						     hwmon_temp_input, j);
+			if (!mode) {
+				struct hwmon_thermal_data *tzdata;
+
+				if (!update)
+					continue;
+				tzdata = hwmon_thermal_find_tz(dev, j);
+				if (tzdata) {
+					devm_thermal_zone_of_sensor_unregister(dev, tzdata->tzd);
+					devm_release_action(dev, hwmon_thermal_remove_sensor, &tzdata->node);
+				}
+			} else {
+				if (!update || !hwmon_thermal_find_tz(dev, j)) {
+					err = hwmon_thermal_add_sensor(dev, j);
+					if (err)
+						return err;
+				}
+			}
 		}
 	}
 
@@ -281,15 +307,11 @@ static int hwmon_thermal_register_sensors(struct device *dev)
 
 static void hwmon_thermal_notify(struct device *dev, int index)
 {
-	struct hwmon_device *hwdev = to_hwmon_device(dev);
-	struct hwmon_thermal_data *tzdata;
+	struct hwmon_thermal_data *tzdata = hwmon_thermal_find_tz(dev, index);
 
-	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
-		if (tzdata->index == index) {
-			thermal_zone_device_update(tzdata->tzd,
-						   THERMAL_EVENT_UNSPECIFIED);
-		}
-	}
+	if (tzdata)
+		thermal_zone_device_update(tzdata->tzd,
+					   THERMAL_EVENT_UNSPECIFIED);
 }
 
 #else
@@ -401,10 +423,12 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	if (!template)
 		return ERR_PTR(-ENOENT);
 
+	/*
+	 * Basic mode sanity check. This is less than perfect since
+	 * attribute visibility and with it the mode can change during
+	 * runtime, but it is the best we can do.
+	 */
 	mode = ops->is_visible(drvdata, type, attr, index);
-	if (!mode)
-		return ERR_PTR(-ENOENT);
-
 	if ((mode & 0444) && ((is_string && !ops->read_string) ||
 				 (!is_string && !ops->read)))
 		return ERR_PTR(-EINVAL);
@@ -435,7 +459,7 @@ static struct attribute *hwmon_genattr(const void *drvdata,
 	a = &dattr->attr;
 	sysfs_attr_init(a);
 	a->name = name;
-	a->mode = mode;
+	a->mode = ops->write ? 0644 : 0444;	/* updated when attributes are generated */
 
 	return a;
 }
@@ -638,6 +662,24 @@ static const int __templates_size[] = {
 	[hwmon_intrusion] = ARRAY_SIZE(hwmon_intrusion_attr_templates),
 };
 
+int hwmon_update_groups(struct device *dev)
+{
+	struct hwmon_device *hwdev = to_hwmon_device(dev);
+	const struct hwmon_chip_info *chip = hwdev->chip;
+	const struct hwmon_channel_info **info = chip->info;
+	int ret;
+
+	ret = sysfs_update_groups(&dev->kobj, dev->groups);
+	if (ret)
+		return ret;
+
+	if (info[0]->type != hwmon_chip || !(info[0]->config[0] & HWMON_C_REGISTER_TZ))
+		return 0;
+
+	return hwmon_thermal_register_sensors(dev, true);
+}
+EXPORT_SYMBOL_GPL(hwmon_update_groups);
+
 int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 		       u32 attr, int channel)
 {
@@ -748,6 +790,16 @@ __hwmon_create_attrs(const void *drvdata, const struct hwmon_chip_info *chip)
 	return attrs;
 }
 
+static umode_t hwmon_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct device_attribute *dattr = to_dev_attr(attr);
+	struct hwmon_device_attribute *hattr = to_hwmon_attr(dattr);
+	struct device *dev = kobj_to_dev(kobj);
+	void *drvdata = dev_get_drvdata(dev);
+
+	return hattr->ops->is_visible(drvdata, hattr->type, hattr->attr, hattr->index);
+}
+
 static struct device *
 __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 			const struct hwmon_chip_info *chip,
@@ -797,6 +849,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 		}
 
 		hwdev->group.attrs = attrs;
+		hwdev->group.is_visible = hwmon_is_visible;
 		ngroups = 0;
 		hwdev->groups[ngroups++] = &hwdev->group;
 
@@ -840,7 +893,7 @@ __hwmon_device_register(struct device *dev, const char *name, void *drvdata,
 	if (dev && dev->of_node && chip && chip->ops->read &&
 	    chip->info[0]->type == hwmon_chip &&
 	    (chip->info[0]->config[0] & HWMON_C_REGISTER_TZ)) {
-		err = hwmon_thermal_register_sensors(hdev);
+		err = hwmon_thermal_register_sensors(hdev, false);
 		if (err) {
 			device_unregister(hdev);
 			/*
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 14325f93c6b2..389ed45d4bd7 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -467,6 +467,8 @@ int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
 char *hwmon_sanitize_name(const char *name);
 char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
 
+int hwmon_update_groups(struct device *dev);
+
 /**
  * hwmon_is_bad_char - Is the char invalid in a hwmon name
  * @ch: the char to be considered
-- 
2.35.1

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[LABBE Corentin]

Le Sat, May 21, 2022 at 06:52:15AM -0700, Guenter Roeck a écrit :
> On 5/16/22 05:21, Guenter Roeck wrote:
> > On 5/15/22 23:21, LABBE Corentin wrote:
> >> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
> >>> On 5/15/22 12:36, LABBE Corentin wrote:
> >>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> >>>>> Corentin,
> >>>>>
> >>>>> On 5/8/22 23:30, Corentin Labbe wrote:
> >>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> >>>>>> So let's convert the driver to use hwmon_device_register_with_info().
> >>>>>>
> >>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> >>>>>> ---
> >>>>> [ ... ]
> >>>>>
> >>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>>>>>             if (res)
> >>>>>>                 break;
> >>>>>> -        remove_attrs(resource);
> >>>>>> +        remove_domain_devices(resource);
> >>>>>>             setup_attrs(resource);
> >>>>>
> >>>>> Zhang Rui found an interesting problem with this code:
> >>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
> >>>>> to update sysfs attribute visibility, probably between
> >>>>> remove_domain_devices() and setup_attrs().
> >>>>>
> >>>>>>             break;
> >>>>>>         case METER_NOTIFY_TRIP:
> >>>>>> -        sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> >>>>>> +        hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> >>>>>
> >>>>> ... which makes realize: The notification device should be the hwmon device.
> >>>>> That would be resource->hwmon_dev, not the acpi device.
> >>>>>
> >>>>
> >>>> Hello
> >>>>
> >>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
> >>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
> >>>>
> >>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
> >>>>
> >>>> For testing config change I have tried lot of way:
> >>>>                   res = read_capabilities(resource);
> >>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>>>                   remove_domain_devices(resource);
> >>>>                   setup_attrs(resource);
> >>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
> >>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
> >>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> >>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
> >>>
> >>> Did you add a debug log here ?
> >>
> >> Yes I added debug log to check what is called.
> >>
> >>>
> >>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
> >>> It would have to be resource->hwmon_dev->groups.
> >>>
> >>
> >> Even with that, no call to is_visible:
> >> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>                  remove_domain_devices(resource);
> >>                  setup_attrs(resource);
> >> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
> >> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
> >> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> >> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
> >>                  break;
> >>
> >> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
> >> So perhaps it explain why it is never called.
> > 
> > Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
> > Effectively that means we'll have to rework the hwmon core to generate attributes
> > anyway and leave it up to the driver core to call the is_visible function.
> > 
> 
> Attached is an outline of what would be needed in the hwmon core.
> Completely untested. I wonder if it may be easier to always
> create all attributes and have them return -ENODATA if not
> supported.
> 

With your patch, the notify config change lead to new attributes to be displayed.

Your patch just needs:
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -315,7 +315,7 @@ static void hwmon_thermal_notify(struct device *dev, int index)
 }
 
 #else
-static int hwmon_thermal_register_sensors(struct device *dev)
+static int hwmon_thermal_register_sensors(struct device *dev, bool update)
 {
        return 0;
 }

Tested with:
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -40,7 +40,7 @@
 #define METER_NOTIFY_INTERVAL  0x84
 
 static int cap_in_hardware;
-static bool force_cap_on;
+static bool force_cap_on = 1;
 
 static int can_cap_in_hardware(void)
 {
@@ -337,6 +337,16 @@ static ssize_t power1_is_battery_show(struct device *dev,
 {
        struct acpi_power_meter_resource *resource = dev_get_drvdata(dev);
        int val;
+       unsigned long long data;
+       acpi_status status;
+
+       status = acpi_evaluate_integer(resource->acpi_dev->handle, "_DBX",
+                                      NULL, &data);
+       if (ACPI_FAILURE(status)) {
+               acpi_evaluation_failure_warn(resource->acpi_dev->handle, "_DBX",
+                                            status);
+       }
+       pr_info("DBX give %llu\n", data);
 
        if (resource->caps.flags & POWER_METER_IS_BATTERY)
                val = 1;
@@ -570,6 +580,8 @@ static umode_t acpi_power_is_visible(const void *data,
 {
        const struct acpi_power_meter_resource *resource = data;
 
+       pr_info("%s\n", __func__);
+
        switch (attr) {
        case hwmon_power_average_min:
        case hwmon_power_average_max:
@@ -741,7 +753,7 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
 
                remove_domain_devices(resource);
                setup_attrs(resource);
-               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
+               res = hwmon_update_groups(resource->hwmon_dev);
                break;
        case METER_NOTIFY_TRIP:
On a microvm qemu with my ACPI power meter emulation:
/ # ls /sys/class/hwmon/hwmon0/
device		 power1_average		  power1_is_battery	subsystem
name		 power1_average_interval  power1_model_number	uevent
power		 power1_interval_max	  power1_oem_info
power1_accuracy  power1_interval_min	  power1_serial_number
/ # cat /sys/class/hwmon/hwmon0/
device/                  power1_average_interval  power1_oem_info
name                     power1_interval_max      power1_serial_number
power/                   power1_interval_min      subsystem/
power1_accuracy          power1_is_battery        uevent
power1_average           power1_model_number
/ # cat /sys/class/hwmon/hwmon0/power1_is_battery 
[   14.969697] power_meter ACPI000D:00: Found ACPI power meter.
[   14.970114] acpi_power_is_visible
[   14.970133] acpi_power_is_visible
[   14.970141] acpi_power_is_visible
[   14.970147] acpi_power_is_visible
[   14.970153] acpi_power_is_visible
[   14.970158] acpi_power_is_visible
[   14.970164] acpi_power_is_visible
[   14.970169] acpi_power_is_visible
[   14.970187] acpi_power_is_visible
[   14.970193] acpi_power_is_visible
[   14.970202] acpi_power_is_visible
[   14.970346] DBX give 40
[   14.975185] power_meter ACPI000D:00: Capping in progress.
0
cat: /sys/class/hwmon/hwmon0/power1_is_battery: No such device
/ # ls /sys/class/hwmon/hwmon0/
device			 power1_average_min   power1_is_battery
name			 power1_cap	      power1_model_number
power			 power1_cap_hyst      power1_oem_info
power1_accuracy		 power1_cap_max       power1_serial_number
power1_average		 power1_cap_min       subsystem
power1_average_interval  power1_interval_max  uevent
power1_average_max	 power1_interval_min

Thanks

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[LABBE Corentin]

Le Sat, May 21, 2022 at 06:52:15AM -0700, Guenter Roeck a écrit :
> On 5/16/22 05:21, Guenter Roeck wrote:
> > On 5/15/22 23:21, LABBE Corentin wrote:
> >> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
> >>> On 5/15/22 12:36, LABBE Corentin wrote:
> >>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
> >>>>> Corentin,
> >>>>>
> >>>>> On 5/8/22 23:30, Corentin Labbe wrote:
> >>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
> >>>>>> So let's convert the driver to use hwmon_device_register_with_info().
> >>>>>>
> >>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> >>>>>> ---
> >>>>> [ ... ]
> >>>>>
> >>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>>>>>             if (res)
> >>>>>>                 break;
> >>>>>> -        remove_attrs(resource);
> >>>>>> +        remove_domain_devices(resource);
> >>>>>>             setup_attrs(resource);
> >>>>>
> >>>>> Zhang Rui found an interesting problem with this code:
> >>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
> >>>>> to update sysfs attribute visibility, probably between
> >>>>> remove_domain_devices() and setup_attrs().
> >>>>>
> >>>>>>             break;
> >>>>>>         case METER_NOTIFY_TRIP:
> >>>>>> -        sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
> >>>>>> +        hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
> >>>>>
> >>>>> ... which makes realize: The notification device should be the hwmon device.
> >>>>> That would be resource->hwmon_dev, not the acpi device.
> >>>>>
> >>>>
> >>>> Hello
> >>>>
> >>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
> >>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
> >>>>
> >>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
> >>>>
> >>>> For testing config change I have tried lot of way:
> >>>>                   res = read_capabilities(resource);
> >>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>>>                   remove_domain_devices(resource);
> >>>>                   setup_attrs(resource);
> >>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
> >>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
> >>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> >>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
> >>>
> >>> Did you add a debug log here ?
> >>
> >> Yes I added debug log to check what is called.
> >>
> >>>
> >>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
> >>> It would have to be resource->hwmon_dev->groups.
> >>>
> >>
> >> Even with that, no call to is_visible:
> >> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
> >>                  remove_domain_devices(resource);
> >>                  setup_attrs(resource);
> >> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
> >> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
> >> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
> >> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
> >>                  break;
> >>
> >> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
> >> So perhaps it explain why it is never called.
> > 
> > Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
> > Effectively that means we'll have to rework the hwmon core to generate attributes
> > anyway and leave it up to the driver core to call the is_visible function.
> > 
> 
> Attached is an outline of what would be needed in the hwmon core.
> Completely untested. I wonder if it may be easier to always
> create all attributes and have them return -ENODATA if not
> supported.
> 
> Guenter


Hello

Do you plan to send your change as patch ?
My patch serie is stuck since now it depend on it.

Or can I send a new iteration of my serie with partial support for notify.

Regards

Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info

[Guenter Roeck]

On 6/30/22 07:49, LABBE Corentin wrote:
> Le Sat, May 21, 2022 at 06:52:15AM -0700, Guenter Roeck a écrit :
>> On 5/16/22 05:21, Guenter Roeck wrote:
>>> On 5/15/22 23:21, LABBE Corentin wrote:
>>>> Le Sun, May 15, 2022 at 05:29:54PM -0700, Guenter Roeck a écrit :
>>>>> On 5/15/22 12:36, LABBE Corentin wrote:
>>>>>> Le Wed, May 11, 2022 at 07:10:29PM -0700, Guenter Roeck a écrit :
>>>>>>> Corentin,
>>>>>>>
>>>>>>> On 5/8/22 23:30, Corentin Labbe wrote:
>>>>>>>> Booting lead to a hwmon_device_register() is deprecated. Please convert the driver to use hwmon_device_register_with_info().
>>>>>>>> So let's convert the driver to use hwmon_device_register_with_info().
>>>>>>>>
>>>>>>>> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
>>>>>>>> ---
>>>>>>> [ ... ]
>>>>>>>
>>>>>>>> @@ -836,20 +740,20 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>>>>>              if (res)
>>>>>>>>                  break;
>>>>>>>> -        remove_attrs(resource);
>>>>>>>> +        remove_domain_devices(resource);
>>>>>>>>              setup_attrs(resource);
>>>>>>>
>>>>>>> Zhang Rui found an interesting problem with this code:
>>>>>>> It needs a call to sysfs_update_groups(hwmon_dev->groups)
>>>>>>> to update sysfs attribute visibility, probably between
>>>>>>> remove_domain_devices() and setup_attrs().
>>>>>>>
>>>>>>>>              break;
>>>>>>>>          case METER_NOTIFY_TRIP:
>>>>>>>> -        sysfs_notify(&device->dev.kobj, NULL, POWER_AVERAGE_NAME);
>>>>>>>> +        hwmon_notify_event(&device->dev, hwmon_power, hwmon_power_average, 0);
>>>>>>>
>>>>>>> ... which makes realize: The notification device should be the hwmon device.
>>>>>>> That would be resource->hwmon_dev, not the acpi device.
>>>>>>>
>>>>>>
>>>>>> Hello
>>>>>>
>>>>>> Since my hardware lacks capabilities testing this, I have emulated it on qemu:
>>>>>> https://github.com/montjoie/qemu/commit/320f2ddacb954ab308ef699f66fca6313f75bc2b
>>>>>>
>>>>>> I have added a custom ACPI _DBX method for triggering some ACPI state change. (like config change, like enabling CAP).
>>>>>>
>>>>>> For testing config change I have tried lot of way:
>>>>>>                    res = read_capabilities(resource);
>>>>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>>>                    remove_domain_devices(resource);
>>>>>>                    setup_attrs(resource);
>>>>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, acpi_power_groups);
>>>>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, acpi_power_groups);
>>>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>>>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>>>>
>>>>> Did you add a debug log here ?
>>>>
>>>> Yes I added debug log to check what is called.
>>>>
>>>>>
>>>>> acpi_power_groups would be the wrong parameter for sysfs_update_groups().
>>>>> It would have to be resource->hwmon_dev->groups.
>>>>>
>>>>
>>>> Even with that, no call to is_visible:
>>>> @@ -742,18 +758,22 @@ static void acpi_power_meter_notify(struct acpi_device *device, u32 event)
>>>>                   remove_domain_devices(resource);
>>>>                   setup_attrs(resource);
>>>> +               res = sysfs_update_groups(&resource->hwmon_dev->kobj, resource->hwmon_dev->groups);
>>>> +               res = sysfs_update_groups(&resource->acpi_dev->dev.kobj, resource->hwmon_dev->groups);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_cap, 0);
>>>> +               res = hwmon_notify_event(resource->hwmon_dev, hwmon_power, hwmon_power_average, 0);
>>>>                   break;
>>>>
>>>> I checked drivers/hwmon/hwmon.c is seems that is_visible is only called by gen_attr/gen_attrs which is only called by __hwmon_create_attrs and then by registers functions.
>>>> So perhaps it explain why it is never called.
>>>
>>> Ah yes, you are correct. Sorry, it has been too long ago that I wrote that code.
>>> Effectively that means we'll have to rework the hwmon core to generate attributes
>>> anyway and leave it up to the driver core to call the is_visible function.
>>>
>>
>> Attached is an outline of what would be needed in the hwmon core.
>> Completely untested. I wonder if it may be easier to always
>> create all attributes and have them return -ENODATA if not
>> supported.
>>
>> Guenter
> 
> 
> Hello
> 
> Do you plan to send your change as patch ?
> My patch serie is stuck since now it depend on it.
> 
> Or can I send a new iteration of my serie with partial support for notify.
> 

Unfortunately I won't have time to work on this anytime soon.

Guenter