Received-SPF: pass (google.com: domain of linux-kernel+bounces-192987-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240510201242.3886-1-W_Armin@gmx.de> <CAJZ5v0gu=yvFeXH4uwHeAQZ8p1JyCs8uuiKzr6F+yNkD2E1ZAA@mail.gmail.com>
 <92415bd8-4231-4c68-8034-e4f7a0ea5651@gmx.de>
In-Reply-To: <92415bd8-4231-4c68-8034-e4f7a0ea5651@gmx.de>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Tue, 28 May 2024 21:52:38 +0200
Message-ID: <CAJZ5v0iLNZT4jU1QV=HTbAv76s6Ay3tx93_K_QoM0XbvSgZF1w@mail.gmail.com>
Subject: Re: [PATCH v8] ACPI: fan: Add hwmon support
To: Armin Wolf <W_Armin@gmx.de>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, mlj@danelec.com, rafael.j.wysocki@intel.com, 
	lenb@kernel.org, jdelvare@suse.com, andy.shevchenko@gmail.com, 
	linux@roeck-us.net, linux@weissschuh.net, ilpo.jarvinen@linux.intel.com, 
	linux-acpi@vger.kernel.org, linux-hwmon@vger.kernel.org, 
	linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 28, 2024 at 12:31=E2=80=AFAM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 27.05.24 um 19:29 schrieb Rafael J. Wysocki:
>
> > On Fri, May 10, 2024 at 10:13=E2=80=AFPM Armin Wolf <W_Armin@gmx.de> wr=
ote:
> >> 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@kernel.org>
> >> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> >> ---
> >> 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)  +=3D tiny-powe=
r-button.o
> >>   obj-$(CONFIG_ACPI_FAN)         +=3D fan.o
> >>   fan-objs                       :=3D fan_core.o
> >>   fan-objs                       +=3D fan_attr.o
> >> +fan-$(CONFIG_HWMON)            +=3D fan_hwmon.o
> >>
> >>   obj-$(CONFIG_ACPI_VIDEO)       +=3D video.o
> >>   obj-$(CONFIG_ACPI_TAD)         +=3D 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 *devi=
ce) { 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 =3D devm_acpi_fan_create_hwmon(device);
> >> +               if (result)
> >> +                       return result;
> >> +
> >>                  result =3D 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@gmx.de>
> >> + */
> >> +
> >> +#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 =3D 0; i < fan->fps_count; i++) {
> >> +               if (fan->fps[i].control =3D=3D control)
> >> +                       return &fan->fps[i];
> >> +       }
> >> +
> >> +       return NULL;
> >> +}
> >> +
> >> +static umode_t acpi_fan_hwmon_is_visible(const void *drvdata, enum hw=
mon_sensor_types type,
> >> +                                        u32 attr, int channel)
> >> +{
> >> +       const struct acpi_fan *fan =3D 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!