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
