* [PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API @ 2022-05-09 6:30 Corentin Labbe 2022-05-09 6:30 ` [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue Corentin Labbe 2022-05-09 6:30 ` [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info Corentin Labbe 0 siblings, 2 replies; 17+ messages in thread From: Corentin Labbe @ 2022-05-09 6:30 UTC (permalink / raw) To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue 2022-05-09 6:30 [PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API Corentin Labbe @ 2022-05-09 6:30 ` Corentin Labbe 2022-05-10 2:39 ` Guenter Roeck 2022-05-09 6:30 ` [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info Corentin Labbe 1 sibling, 1 reply; 17+ messages in thread From: Corentin Labbe @ 2022-05-09 6:30 UTC (permalink / raw) To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue 2022-05-09 6:30 ` [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue Corentin Labbe @ 2022-05-10 2:39 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2022-05-10 2:39 UTC (permalink / raw) To: Corentin Labbe; +Cc: jdelvare, linux-hwmon, linux-kernel 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) ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-09 6:30 [PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API Corentin Labbe 2022-05-09 6:30 ` [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue Corentin Labbe @ 2022-05-09 6:30 ` Corentin Labbe 2022-05-10 3:05 ` Guenter Roeck 2022-05-12 2:10 ` Guenter Roeck 1 sibling, 2 replies; 17+ messages in thread From: Corentin Labbe @ 2022-05-09 6:30 UTC (permalink / raw) To: jdelvare, linux; +Cc: linux-hwmon, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-09 6:30 ` [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info Corentin Labbe @ 2022-05-10 3:05 ` Guenter Roeck 2022-05-12 2:10 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2022-05-10 3:05 UTC (permalink / raw) To: Corentin Labbe; +Cc: jdelvare, linux-hwmon, linux-kernel 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); ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-09 6:30 ` [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info Corentin Labbe 2022-05-10 3:05 ` Guenter Roeck @ 2022-05-12 2:10 ` Guenter Roeck 2022-05-13 8:02 ` LABBE Corentin 2022-05-15 19:36 ` LABBE Corentin 1 sibling, 2 replies; 17+ messages in thread From: Guenter Roeck @ 2022-05-12 2:10 UTC (permalink / raw) To: Corentin Labbe, jdelvare; +Cc: linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-12 2:10 ` Guenter Roeck @ 2022-05-13 8:02 ` LABBE Corentin 2022-05-13 11:33 ` Zhang Rui 2022-05-13 13:03 ` Guenter Roeck 2022-05-15 19:36 ` LABBE Corentin 1 sibling, 2 replies; 17+ messages in thread From: LABBE Corentin @ 2022-05-13 8:02 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang 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 <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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-13 8:02 ` LABBE Corentin @ 2022-05-13 11:33 ` Zhang Rui 2022-05-13 13:03 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Zhang Rui @ 2022-05-13 11:33 UTC (permalink / raw) To: LABBE Corentin, Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-13 8:02 ` LABBE Corentin 2022-05-13 11:33 ` Zhang Rui @ 2022-05-13 13:03 ` Guenter Roeck 1 sibling, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2022-05-13 13:03 UTC (permalink / raw) To: LABBE Corentin; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-12 2:10 ` Guenter Roeck 2022-05-13 8:02 ` LABBE Corentin @ 2022-05-15 19:36 ` LABBE Corentin 2022-05-16 0:29 ` Guenter Roeck 1 sibling, 1 reply; 17+ messages in thread From: LABBE Corentin @ 2022-05-15 19:36 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang 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 <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 ? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-15 19:36 ` LABBE Corentin @ 2022-05-16 0:29 ` Guenter Roeck 2022-05-16 6:21 ` LABBE Corentin 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2022-05-16 0:29 UTC (permalink / raw) To: LABBE Corentin; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-16 0:29 ` Guenter Roeck @ 2022-05-16 6:21 ` LABBE Corentin 2022-05-16 12:21 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: LABBE Corentin @ 2022-05-16 6:21 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-16 6:21 ` LABBE Corentin @ 2022-05-16 12:21 ` Guenter Roeck 2022-05-21 13:52 ` Guenter Roeck 0 siblings, 1 reply; 17+ messages in thread From: Guenter Roeck @ 2022-05-16 12:21 UTC (permalink / raw) To: LABBE Corentin; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-16 12:21 ` Guenter Roeck @ 2022-05-21 13:52 ` Guenter Roeck 2022-05-21 19:16 ` LABBE Corentin 2022-06-30 14:49 ` LABBE Corentin 0 siblings, 2 replies; 17+ messages in thread From: Guenter Roeck @ 2022-05-21 13:52 UTC (permalink / raw) To: LABBE Corentin; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui [-- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-21 13:52 ` Guenter Roeck @ 2022-05-21 19:16 ` LABBE Corentin 2022-06-30 14:49 ` LABBE Corentin 1 sibling, 0 replies; 17+ messages in thread From: LABBE Corentin @ 2022-05-21 19:16 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-05-21 13:52 ` Guenter Roeck 2022-05-21 19:16 ` LABBE Corentin @ 2022-06-30 14:49 ` LABBE Corentin 2022-06-30 15:13 ` Guenter Roeck 1 sibling, 1 reply; 17+ messages in thread From: LABBE Corentin @ 2022-06-30 14:49 UTC (permalink / raw) To: Guenter Roeck; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info 2022-06-30 14:49 ` LABBE Corentin @ 2022-06-30 15:13 ` Guenter Roeck 0 siblings, 0 replies; 17+ messages in thread From: Guenter Roeck @ 2022-06-30 15:13 UTC (permalink / raw) To: LABBE Corentin; +Cc: jdelvare, linux-hwmon, linux-kernel, Zhang Rui 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-06-30 15:13 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-09 6:30 [PATCH v3 0/2] hwmon: acpi_power_meter: convert to new hwmon API Corentin Labbe 2022-05-09 6:30 ` [PATCH v3 1/2] hwmon: acpi_power_meter: fix style issue Corentin Labbe 2022-05-10 2:39 ` Guenter Roeck 2022-05-09 6:30 ` [PATCH v3 2/2] hwmon: acpi_power_meter: convert to hwmon_device_register_with_info Corentin Labbe 2022-05-10 3:05 ` Guenter Roeck 2022-05-12 2:10 ` Guenter Roeck 2022-05-13 8:02 ` LABBE Corentin 2022-05-13 11:33 ` Zhang Rui 2022-05-13 13:03 ` Guenter Roeck 2022-05-15 19:36 ` LABBE Corentin 2022-05-16 0:29 ` Guenter Roeck 2022-05-16 6:21 ` LABBE Corentin 2022-05-16 12:21 ` Guenter Roeck 2022-05-21 13:52 ` Guenter Roeck 2022-05-21 19:16 ` LABBE Corentin 2022-06-30 14:49 ` LABBE Corentin 2022-06-30 15:13 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).