On Mon, 19 Jun 2017, Stanislav Fomichev wrote:
> Use hwmon_device_register_with_groups instead of deprecated
> hwmon_device_register and fix a dmesg warning.
>
> This patch however changes the userspace API.
> hwmon_device_register_with_groups takes `hwmon' name as an argument and creates
> a name file in the `hwmon' device, not in the `platform_device'. This
> allows us to remove custom `name' device attribute, but in order to make
> lm-sensors happy we also have to move fans and thermal attributes to the
> `hwmon' device.
Can you clarify that with a ls -l of the "before" and "after"?
Also, does it change anything visible to lmsensors users (such as the
sensor/adapter name or address)? If so, please post a "before" and
"after"...
> Even though this patch changes userspace API, it's still compatible with
> the lm-sensors. Starting with lm-sensors 3.0 (circa 2007), it looks at both
> hwmon and the backing device for the name and other attributes.
I am not sure this is OK or not, it *is* an userspace ABI break. I am
not really opposed to it, as long as it is done properly. Let's see
what our subsystem maintainers think of it.
That said: the patch is still incomplete, because this driver has
documentation that also needs to be updated...
Please update Documentation/laptops/thinkpad-acpi.txt with the changes
to the thinkpad_hwmon sensor interfaces. Search for "hwmon" on that
text, that should locate everything.
Also, please bump "#define TPACPI_SYSFS_VERSION 0x020700" to 0x020800 in
drivers/platform/x86/thinkpad_acpi.c, and update the interface changelog
table at the end of Documentation/laptops/thinkpad-acpi.txt.
(that assumes the change is not a major ABI break, please reply with
that "ls -l" I asked above: we might need to bump that
TPACPI_SYSFS_VERSION to 0x030000, instead...)
> Signed-off-by: Stanislav Fomichev <kernel@xxxxxxxxxxx>
> ---
> drivers/platform/x86/thinkpad_acpi.c | 36 ++++++++++--------------------------
> 1 file changed, 10 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 7b6cb0c69b02..0e0a1616f273 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -6401,7 +6401,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
>
> switch (thermal_read_mode) {
> case TPACPI_THERMAL_TPEC_16:
> - res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
> + res = sysfs_create_group(&tpacpi_hwmon->kobj,
> &thermal_temp_input16_group);
> if (res)
> return res;
> @@ -6409,7 +6409,7 @@ static int __init thermal_init(struct ibm_init_struct *iibm)
> case TPACPI_THERMAL_TPEC_8:
> case TPACPI_THERMAL_ACPI_TMP07:
> case TPACPI_THERMAL_ACPI_UPDT:
> - res = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
> + res = sysfs_create_group(&tpacpi_hwmon->kobj,
> &thermal_temp_input8_group);
> if (res)
> return res;
> @@ -6426,13 +6426,13 @@ static void thermal_exit(void)
> {
> switch (thermal_read_mode) {
> case TPACPI_THERMAL_TPEC_16:
> - sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
> + sysfs_remove_group(&tpacpi_hwmon->kobj,
> &thermal_temp_input16_group);
> break;
> case TPACPI_THERMAL_TPEC_8:
> case TPACPI_THERMAL_ACPI_TMP07:
> case TPACPI_THERMAL_ACPI_UPDT:
> - sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
> + sysfs_remove_group(&tpacpi_hwmon->kobj,
> &thermal_temp_input8_group);
> break;
> case TPACPI_THERMAL_NONE:
> @@ -8773,7 +8773,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> fan_attributes[ARRAY_SIZE(fan_attributes)-2] =
> &dev_attr_fan2_input.attr;
> }
> - rc = sysfs_create_group(&tpacpi_sensors_pdev->dev.kobj,
> + rc = sysfs_create_group(&tpacpi_hwmon->kobj,
> &fan_attr_group);
> if (rc < 0)
> return rc;
> @@ -8781,7 +8781,7 @@ static int __init fan_init(struct ibm_init_struct *iibm)
> rc = driver_create_file(&tpacpi_hwmon_pdriver.driver,
> &driver_attr_fan_watchdog);
> if (rc < 0) {
> - sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj,
> + sysfs_remove_group(&tpacpi_hwmon->kobj,
> &fan_attr_group);
> return rc;
> }
> @@ -8796,7 +8796,7 @@ static void fan_exit(void)
> "cancelling any pending fan watchdog tasks\n");
>
> /* FIXME: can we really do this unconditionally? */
> - sysfs_remove_group(&tpacpi_sensors_pdev->dev.kobj, &fan_attr_group);
> + sysfs_remove_group(&tpacpi_hwmon->kobj, &fan_attr_group);
> driver_remove_file(&tpacpi_hwmon_pdriver.driver,
> &driver_attr_fan_watchdog);
>
> @@ -9229,16 +9229,6 @@ static void hotkey_driver_event(const unsigned int scancode)
> tpacpi_driver_event(TP_HKEY_EV_HOTKEY_BASE + scancode);
> }
>
> -/* sysfs name ---------------------------------------------------------- */
> -static ssize_t thinkpad_acpi_pdev_name_show(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> -{
> - return snprintf(buf, PAGE_SIZE, "%s\n", TPACPI_NAME);
> -}
> -
> -static DEVICE_ATTR(name, S_IRUGO, thinkpad_acpi_pdev_name_show, NULL);
> -
> /* --------------------------------------------------------------------- */
>
> /* /proc support */
> @@ -9782,8 +9772,6 @@ static void thinkpad_acpi_module_exit(void)
> if (tpacpi_hwmon)
> hwmon_device_unregister(tpacpi_hwmon);
>
> - if (tp_features.sensors_pdev_attrs_registered)
> - device_remove_file(&tpacpi_sensors_pdev->dev, &dev_attr_name);
> if (tpacpi_sensors_pdev)
> platform_device_unregister(tpacpi_sensors_pdev);
> if (tpacpi_pdev)
> @@ -9904,14 +9892,10 @@ static int __init thinkpad_acpi_module_init(void)
> thinkpad_acpi_module_exit();
> return ret;
> }
> - ret = device_create_file(&tpacpi_sensors_pdev->dev, &dev_attr_name);
> - if (ret) {
> - pr_err("unable to create sysfs hwmon device attributes\n");
> - thinkpad_acpi_module_exit();
> - return ret;
> - }
> tp_features.sensors_pdev_attrs_registered = 1;
> - tpacpi_hwmon = hwmon_device_register(&tpacpi_sensors_pdev->dev);
> + tpacpi_hwmon = hwmon_device_register_with_groups(
> + &tpacpi_sensors_pdev->dev, TPACPI_NAME, NULL, NULL);
> +
> if (IS_ERR(tpacpi_hwmon)) {
> ret = PTR_ERR(tpacpi_hwmon);
> tpacpi_hwmon = NULL;
--
Henrique Holschuh
