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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
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 <[email protected]>
> ---
[ ... ]
> @@ -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
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 <[email protected]>
>>> ---
>> [ ... ]
>>
>>> @@ -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
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 <[email protected]>
> > ---
> [ ... ]
>
> > @@ -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
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 <[email protected]>
> > > ---
> >
> > [ ... ]
> >
> > > @@ -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
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 <[email protected]>
> > ---
> [ ... ]
>
> > @@ -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 ?
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 <[email protected]>
>>> ---
>> [ ... ]
>>
>>> @@ -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 ?
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 <[email protected]>
> >>> ---
> >> [ ... ]
> >>
> >>> @@ -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.
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 <[email protected]>
>>>>> ---
>>>> [ ... ]
>>>>
>>>>> @@ -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
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 <[email protected]>
>>>>>> ---
>>>>> [ ... ]
>>>>>
>>>>>> @@ -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
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 <[email protected]>
> >>>>>> ---
> >>>>> [ ... ]
> >>>>>
> >>>>>> @@ -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
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 <[email protected]>
> >>>>>> ---
> >>>>> [ ... ]
> >>>>>
> >>>>>> @@ -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
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 <[email protected]>
>>>>>>>> ---
>>>>>>> [ ... ]
>>>>>>>
>>>>>>>> @@ -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