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);
>
