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

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

 



Hi Guenter, Arvid,

On 9/2/22 00:27, Guenter Roeck wrote:
> On 9/1/22 14:58, 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 | 72 +++++++++++++++++++++++++++++
>>   2 files changed, 73 insertions(+)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index f2f98e942cf2..4d0d2676939a 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -797,6 +797,7 @@ config ACPI_TOSHIBA
>>       depends on INPUT
>>       depends on SERIO_I8042 || SERIO_I8042 = n
>>       depends on ACPI_VIDEO || ACPI_VIDEO = n
>> +    depends on HWMON || HWMON = n
>>       depends on RFKILL || RFKILL = n
>>       depends on IIO
>>       select INPUT_SPARSEKMAP
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index 02e3522f4eeb..a976dfb97a5e 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -46,6 +46,10 @@
>>   #include <linux/toshiba.h>
>>   #include <acpi/video.h>
>>   +#ifdef CONFIG_HWMON
>> +#include <linux/hwmon.h>
>> +#endif
> 
> ifdef not needed here.

Ack.

> 
>> +
>>   MODULE_AUTHOR("John Belmonte");
>>   MODULE_DESCRIPTION("Toshiba Laptop ACPI Extras Driver");
>>   MODULE_LICENSE("GPL");
>> @@ -171,6 +175,9 @@ struct toshiba_acpi_dev {
>>       struct miscdevice miscdev;
>>       struct rfkill *wwan_rfk;
>>       struct iio_dev *indio_dev;
>> +#ifdef CONFIG_HWMON
>> +    struct device *hwmon_device;
>> +#endif
>>         int force_fan;
>>       int last_key_event;
>> @@ -2941,6 +2948,54 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>>       return 0;
>>   }
>>   +/* HWMON support for fan */
>> +#ifdef CONFIG_HWMON
> 
> This should be #if IS_REACHABLE(CONFIG_HWMON)

Actually that should be IS_ENABLED since you suggested that
Arvid should use:

	depends on HWMON || HWMON = n

In the Kconfig bit there is no need for IS_REACHABLE,
note IS_REACHABLE will work too but I generally prefer
to avoid it because cases which actually need it lead
to weirdness where e.g. both HWMON and TOSHIBA_ACPI are
enabled yet TOSHIBA_ACPI will still not have HWMON
support.

Arvid, sorry about the "noise" here, let me try to
explain.

First of all lets explain this bit of magic:

	depends on HWMON || HWMON = n

What this says is that if HWMON is enabled it must
be able to satisfy dependencies on it in toshiba_acpi.c
(or it may also be fully disabled).

This magic is necessary to avoid a case where
toshiba_acpi gets build into the kernel, but the
hwmon code is a module. In that case linking errors
(unresolved hwmon symbols) will happen when building
the main vmlinuz kernel image.

So basically what this does is if HWMON is configured
as a module, it limits the choices for TOSHIBA_ACPI
to either n or m and disallows y.

I hope that so far I'm making sense...

So now to the #ifdef-ery. Since HWMON can be a module
when enabled the #define's from Kconfig will either
contain:

#define CONFIG_HWMON 1   // when builtin, HWMON=y

or:

#define CONFIG_HWMON_MODULE 1   // when a module, HWMON=m

So you would need to write:

#if defined CONFIG_HWMON || defined CONFIG_HWMON_MODULE

as a condition

#if IS_ENABLED(CONFIG_HWMON)

is a shorthand (macro) for this.

###

Now back to:

#if IS_REACHABLE(CONFIG_HWMON)

This is a special macro for when your Kconfig bit would just be:

	depends on HWMON

in that case TOSHIBA_ACPI might be set to builtin (y)
while the HWMON core/class code is a module. As mentioned
above that would lead to undefined hwmon symbols when
using "#if IS_ENABLED(CONFIG_HWMON)" as test. IS_REACHABLE
is special in that it will disable (evaluate to false)
in the case where the code being build is builtin and
the dependency is a module.

But that cannot happen here because your Kconfig bit is:

	depends on HWMON || HWMON = n

So "#if IS_ENABLED(CONFIG_HWMON)" is sufficient.

TL;DR: please use "#if IS_ENABLED(CONFIG_HWMON)" to test
if the hwmon code should be build.

Regards,

Hans











> 
>> +umode_t toshiba_acpi_hwmon_is_visible(const void *drvdata,
>> +                      enum hwmon_sensor_types type,
>> +                      u32 attr, int channel)
>> +{
>> +    return 0444;
>> +}
>> +
>> +int toshiba_acpi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +                u32 attr, int channel, long *val)
>> +{
>> +    /*
>> +     * There is only a single channel and single attribute (for the
>> +     * fan) at this point.
>> +     * This can be replaced with more advanced logic in the future,
>> +     * should the need arise.
>> +     */
>> +    if (type == hwmon_fan && channel == 0 && attr == hwmon_fan_input) {
>> +        u32 value;
>> +        int ret;
>> +
>> +        ret = get_fan_rpm(toshiba_acpi, &value);
>> +        if (ret)
>> +            return ret;
>> +
>> +        *val = value;
>> +        return 0;
>> +    }
>> +    return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_channel_info *toshiba_acpi_hwmon_info[] = {
>> +    HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_ops toshiba_acpi_hwmon_ops = {
>> +    .is_visible = toshiba_acpi_hwmon_is_visible,
>> +    .read = toshiba_acpi_hwmon_read,
>> +};
>> +
>> +static const struct hwmon_chip_info toshiba_acpi_hwmon_chip_info = {
>> +    .ops = &toshiba_acpi_hwmon_ops,
>> +    .info = toshiba_acpi_hwmon_info,
>> +};
>> +#endif
>> +
>>   static void print_supported_features(struct toshiba_acpi_dev *dev)
>>   {
>>       pr_info("Supported laptop features:");
>> @@ -2995,6 +3050,11 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>>         remove_toshiba_proc_entries(dev);
>>   +#ifdef CONFIG_HWMON
> 
> #if IS_REACHABLE()
> 
>> +    if (dev->hwmon_device)
>> +        hwmon_device_unregister(dev->hwmon_device);
>> +#endif
>> +
>>       if (dev->accelerometer_supported && dev->indio_dev) {
>>           iio_device_unregister(dev->indio_dev);
>>           iio_device_free(dev->indio_dev);
>> @@ -3187,6 +3247,18 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>>       ret = get_fan_rpm(dev, &dummy);
>>       dev->fan_rpm_supported = !ret;
>>   +#ifdef CONFIG_HWMON
> 
> 
> ... and again.
> 
>> +    if (dev->fan_rpm_supported) {
>> +        dev->hwmon_device = hwmon_device_register_with_info(
>> +            &dev->acpi_dev->dev, "toshiba_acpi_sensors", NULL,
>> +            &toshiba_acpi_hwmon_chip_info, NULL);
>> +        if (IS_ERR(dev->hwmon_device)) {
>> +            dev->hwmon_device = NULL;
>> +            pr_warn("unable to register hwmon device, skipping\n");
>> +        }
>> +    }
>> +#endif
>> +
>>       toshiba_wwan_available(dev);
>>       if (dev->wwan_supported)
>>           toshiba_acpi_setup_wwan_rfkill(dev);
> 




[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