Re: [PATCH v8] ACPI: fan: Add hwmon support

[Rafael J. Wysocki]

On Tue, May 28, 2024 at 12:31 AM Armin Wolf <W_Armin@xxxxxx> wrote:
>
> Am 27.05.24 um 19:29 schrieb Rafael J. Wysocki:
>
> > On Fri, May 10, 2024 at 10:13 PM Armin Wolf <W_Armin@xxxxxx> wrote:
> >> Currently, the driver does only support a custom sysfs
> >> interface to allow userspace to read the fan speed.
> >> Add support for the standard hwmon interface so users
> >> can read the fan speed with standard tools like "sensors".
> >>
> >> Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
> >> Signed-off-by: Armin Wolf <W_Armin@xxxxxx>
> >> ---
> >> Tested witha custom ACPI SSDT, available here:
> >> https://github.com/Wer-Wolf/acpi-fan-ssdt
> >>
> >> Changes since v7:
> >> - add Reviewed-by tag
> >> - spelling fixes
> >> - add missing types.h include
> >>
> >> Changes since v6:
> >> - add "hwmon" to the names of functions and variables
> >> related to hwmon
> >> - replace -ENODATA with -EIO/-ENODEV
> >>
> >> Changes since v5:
> >> - fix coding style issues
> >> - replace double break with return
> >> - add missing includes
> >>
> >> Changes since v4:
> >> - fix spelling issues
> >> - check power values for overflow condition too
> >>
> >> Changes since v3:
> >> - drop fault attrs
> >> - rework initialization
> >>
> >> Changes since v2:
> >> - add support for fanX_target and power attrs
> >>
> >> Changes since v1:
> >> - fix undefined reference error
> >> - fix fan speed validation
> >> - coding style fixes
> >> - clarify that the changes are compile-tested only
> >> - add hwmon maintainers to cc list
> >> ---
> >>   drivers/acpi/Makefile    |   1 +
> >>   drivers/acpi/fan.h       |   9 +++
> >>   drivers/acpi/fan_core.c  |   4 +
> >>   drivers/acpi/fan_hwmon.c | 170 +++++++++++++++++++++++++++++++++++++++
> >>   4 files changed, 184 insertions(+)
> >>   create mode 100644 drivers/acpi/fan_hwmon.c
> >>
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 39ea5cfa8326..61ca4afe83dc 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -77,6 +77,7 @@ obj-$(CONFIG_ACPI_TINY_POWER_BUTTON)  += tiny-power-button.o
> >>   obj-$(CONFIG_ACPI_FAN)         += fan.o
> >>   fan-objs                       := fan_core.o
> >>   fan-objs                       += fan_attr.o
> >> +fan-$(CONFIG_HWMON)            += fan_hwmon.o
> >>
> >>   obj-$(CONFIG_ACPI_VIDEO)       += video.o
> >>   obj-$(CONFIG_ACPI_TAD)         += acpi_tad.o
> >> diff --git a/drivers/acpi/fan.h b/drivers/acpi/fan.h
> >> index f89d19c922dc..db25a3898af7 100644
> >> --- a/drivers/acpi/fan.h
> >> +++ b/drivers/acpi/fan.h
> >> @@ -10,6 +10,8 @@
> >>   #ifndef _ACPI_FAN_H_
> >>   #define _ACPI_FAN_H_
> >>
> >> +#include <linux/kconfig.h>
> >> +
> >>   #define ACPI_FAN_DEVICE_IDS    \
> >>          {"INT3404", }, /* Fan */ \
> >>          {"INTC1044", }, /* Fan for Tiger Lake generation */ \
> >> @@ -57,4 +59,11 @@ struct acpi_fan {
> >>   int acpi_fan_get_fst(struct acpi_device *device, struct acpi_fan_fst *fst);
> >>   int acpi_fan_create_attributes(struct acpi_device *device);
> >>   void acpi_fan_delete_attributes(struct acpi_device *device);
> >> +
> >> +#if IS_REACHABLE(CONFIG_HWMON)
> >> +int devm_acpi_fan_create_hwmon(struct acpi_device *device);
> >> +#else
> >> +static inline int devm_acpi_fan_create_hwmon(struct acpi_device *device) { return 0; };
> >> +#endif
> >> +
> >>   #endif
> >> diff --git a/drivers/acpi/fan_core.c b/drivers/acpi/fan_core.c
> >> index ff72e4ef8738..7cea4495f19b 100644
> >> --- a/drivers/acpi/fan_core.c
> >> +++ b/drivers/acpi/fan_core.c
> >> @@ -336,6 +336,10 @@ static int acpi_fan_probe(struct platform_device *pdev)
> >>                  if (result)
> >>                          return result;
> >>
> >> +               result = devm_acpi_fan_create_hwmon(device);
> >> +               if (result)
> >> +                       return result;
> >> +
> >>                  result = acpi_fan_create_attributes(device);
> >>                  if (result)
> >>                          return result;
> >> diff --git a/drivers/acpi/fan_hwmon.c b/drivers/acpi/fan_hwmon.c
> >> new file mode 100644
> >> index 000000000000..bd0d31a398fa
> >> --- /dev/null
> >> +++ b/drivers/acpi/fan_hwmon.c
> >> @@ -0,0 +1,170 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * hwmon interface for the ACPI Fan driver.
> >> + *
> >> + * Copyright (C) 2024 Armin Wolf <W_Armin@xxxxxx>
> >> + */
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/device.h>
> >> +#include <linux/err.h>
> >> +#include <linux/hwmon.h>
> >> +#include <linux/limits.h>
> >> +#include <linux/types.h>
> >> +#include <linux/units.h>
> >> +
> >> +#include "fan.h"
> >> +
> >> +/* Returned when the ACPI fan does not support speed reporting */
> >> +#define FAN_SPEED_UNAVAILABLE  U32_MAX
> >> +#define FAN_POWER_UNAVAILABLE  U32_MAX
> >> +
> >> +static struct acpi_fan_fps *acpi_fan_get_current_fps(struct acpi_fan *fan, u64 control)
> >> +{
> >> +       unsigned int i;
> >> +
> >> +       for (i = 0; i < fan->fps_count; i++) {
> >> +               if (fan->fps[i].control == control)
> >> +                       return &fan->fps[i];
> >> +       }
> >> +
> >> +       return NULL;
> >> +}
> >> +
> >> +static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> >> +                                        u32 attr, int channel)
> >> +{
> >> +       const struct acpi_fan *fan = drvdata;
> >> +       unsigned int i;
> > AFAICS, the code below can be rewritten as follows:
> >
> > if (fan->fif.fine_grain_ctrl)
> >           return 0;
>
> Hi,
>
> this would break hwmon_fan_input.

Ah, I overlooked the first branch.  Fair enough.

Applied as 6.11 material, thanks!