Re: [PATCH 2/2] [RFC] platform/x86: toshiba_acpi: Add fan RPM reading (hwmon interface)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2022-09-01 17:28, Guenter Roeck wrote:
> On 9/1/22 07:49, Arvid Norlander wrote:
>> This expands on the previous commit, exporting the fan RPM via hwmon.
>>
>> This will look something like the following when using the "sensors"
>> command from lm_sensors:
>>
>> toshiba_acpi_sensors-acpi-0
>> Adapter: ACPI interface
>> fan1:           0 RPM
>>
>> Signed-off-by: Arvid Norlander <lkml@xxxxxxxxx>
>> ---
>>   drivers/platform/x86/Kconfig        |  1 +
>>   drivers/platform/x86/toshiba_acpi.c | 49 +++++++++++++++++++++++++++++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..9a98974ab8bf 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -799,6 +799,7 @@ config ACPI_TOSHIBA
>>       depends on ACPI_VIDEO || ACPI_VIDEO = n
>>       depends on RFKILL || RFKILL = n
>>       depends on IIO
>> +    select HWMON
> 
> This is wrong. I know other drivers in this directory do it, but it is
> still wrong. It should be something like
> 
>     depends on HWMON || HWMON=n
> 
> and the code should deal with the conditionals.

Thanks, I will do that. Perhaps ping the thinkpad_acpi maintainers about
this as well, that is where I copied it from.

> 
>>       select INPUT_SPARSEKMAP
>>       help
>>         This driver adds support for access to certain system settings
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 02e3522f4eeb..2b71dac34cf0 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -39,6 +39,7 @@
>>   #include <linux/i8042.h>
>>   #include <linux/acpi.h>
>>   #include <linux/dmi.h>
>> +#include <linux/hwmon.h>
>>   #include <linux/uaccess.h>
>>   #include <linux/miscdevice.h>
>>   #include <linux/rfkill.h>
>> @@ -171,6 +172,7 @@ struct toshiba_acpi_dev {
>>       struct miscdevice miscdev;
>>       struct rfkill *wwan_rfk;
>>       struct iio_dev *indio_dev;
>> +    struct device *hwmon_device;
>>         int force_fan;
>>       int last_key_event;
>> @@ -2941,6 +2943,38 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>       return 0;
>>   }
>>   +
>> +/* HWMON support for fan */
>> +static ssize_t fan_fan1_input_show(struct device *dev,
>> +                   struct device_attribute *attr,
>> +                   char *buf)
>> +{
>> +    u32 value;
>> +    int ret;
>> +
>> +    ret = get_fan_rpm(toshiba_acpi, &value);
>> +    if (ret)
>> +        return ret;
>> +
>> +    return sysfs_emit(buf, "%u\n", value);
>> +}
>> +
>> +static DEVICE_ATTR(fan1_input, S_IRUGO, fan_fan1_input_show, NULL);
>> +
>> +static struct attribute *fan_attributes[] = {
>> +    &dev_attr_fan1_input.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group fan_attr_group = {
>> +    .attrs = fan_attributes,
>> +};
>> +
>> +static const struct attribute_group *toshiba_acpi_hwmon_groups[] = {
>> +    &fan_attr_group,
>> +    NULL,
>> +};
>> +
>>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>>   {
>>       pr_info("Supported laptop features:");
>> @@ -2995,6 +3029,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>>         remove_toshiba_proc_entries(dev);
>>   +    if (dev->hwmon_device)
>> +        hwmon_device_unregister(dev->hwmon_device);
>> +
>>       if (dev->accelerometer_supported && dev->indio_dev) {
>>           iio_device_unregister(dev->indio_dev);
>>           iio_device_free(dev->indio_dev);
>> @@ -3187,6 +3224,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>       ret = get_fan_rpm(dev, &dummy);
>>       dev->fan_rpm_supported = !ret;
>>   +    if (dev->fan_rpm_supported) {
>> +        dev->hwmon_device = hwmon_device_register_with_groups(
> 
> New drivers should register using [devm_]hwmon_device_register_with_info().

Again, copied thikpad_acpi, but I'll look into that function for version 2.

> 
>> +            &dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
>> +            toshiba_acpi_hwmon_groups);
>> +        if (IS_ERR(dev->hwmon_device)) {
>> +            ret = PTR_ERR(dev->hwmon_device);
>> +            dev->hwmon_device = NULL;
>> +            pr_err("unable to register hwmon device\n");
>> +            goto error;
> 
> The driver works just fine without hwmon, and should not fail to probe
> if hwmon registration fails. It did not fail before this patch was applied
> either, and hwmon is not essential functionality for this driver.

Good point. Again, this was based on thinkpad_acpi which it appears then
has some legacy behaviour.

> 
> Guenter
> 
>> +        }
>> +    }
>> +
>>       toshiba_wwan_available(dev);
>>       if (dev->wwan_supported)
>>           toshiba_acpi_setup_wwan_rfkill(dev);
> 

Best regards,
Arvid Norlander



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux