| Message ID | 20230419083343.505780-7-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Thermal zone device structure encapsulation | expand |
On Wed, Apr 19, 2023 at 10:33 AM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > The Menlon thermal driver creates auxiliary trip points in the thermal > zone sysfs directory. It is specific to Menlon. Actually these trip > points could be generalized with the generic trip points in the future. > > Let's make the code optional and disable it by default so we have a > consistency with the attributes in the thermal zone sysfs > directories. If that hurts we will enable by default this option > instead of disabling it. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/intel/Kconfig | 11 +++++++++++ > drivers/thermal/intel/intel_menlow.c | 9 +++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig > index cb7e7697cf1e..ef7ffe6b56a0 100644 > --- a/drivers/thermal/intel/Kconfig > +++ b/drivers/thermal/intel/Kconfig > @@ -112,6 +112,17 @@ config INTEL_MENLOW > > If unsure, say N. > > +config INTEL_MENLOW_SYSFS_ADDON > + bool "Enable extra sysfs attributes in the thermal zone" > + depends on INTEL_MENLOW > + def_bool n > + help > + Create auxiliary trip points in the thermal zone sysfs > + directory. This is specific to this driver. By default those > + are disabled and are candidate for removal, if you need these > + information anyway, enable the option or upgrade the > + userspace program using them. > + > config INTEL_HFI_THERMAL > bool "Intel Hardware Feedback Interface" > depends on NET > diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c > index d720add918ff..605983be516c 100644 > --- a/drivers/thermal/intel/intel_menlow.c > +++ b/drivers/thermal/intel/intel_menlow.c > @@ -367,6 +367,7 @@ static ssize_t bios_enabled_show(struct device *dev, > return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled"); > } > > +#ifdef CONFIG_INTEL_MENLOW_SYSFS_ADDON > static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, > void *store, struct device *dev, > acpi_handle handle) > @@ -398,6 +399,14 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, > > return 0; > } > +#else > +static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, > + void *store, struct device *dev, > + acpi_handle handle) > +{ > + return 0; > +} After looking at it once more, I think that this driver isn't really functional without the additional sysfs attributes, so if CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes dead code. That's rather unfortunate and I'm not sure how to deal with it ATM. I can queue up the rest of the series for 6.4-rc1 (in which case it will be pushed in the second half of the merge window), but this particular patch requires more thought IMV. > +#endif > > static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl, > void *context, void **rv) > --
On 20/04/2023 19:24, Rafael J. Wysocki wrote: [ ... ] > After looking at it once more, I think that this driver isn't really > functional without the additional sysfs attributes, so if > CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes > dead code. > > That's rather unfortunate and I'm not sure how to deal with it ATM. > > I can queue up the rest of the series for 6.4-rc1 (in which case it > will be pushed in the second half of the merge window), but this > particular patch requires more thought IMV. I'm fine if you drop this one from the series. Thanks for looking at it more deeply
On Thu, Apr 20, 2023 at 11:40 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > > On 20/04/2023 19:24, Rafael J. Wysocki wrote: > > [ ... ] > > > After looking at it once more, I think that this driver isn't really > > functional without the additional sysfs attributes, so if > > CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes > > dead code. > > > > That's rather unfortunate and I'm not sure how to deal with it ATM. > > > > I can queue up the rest of the series for 6.4-rc1 (in which case it > > will be pushed in the second half of the merge window), but this > > particular patch requires more thought IMV. > > I'm fine if you drop this one from the series. > > Thanks for looking at it more deeply No problem. After some private conversation with Srinivas regarding this driver, I've just posted a patch to get rid of it entirely and so I'm going to skip all of the patches touching the Menlow driver in this series.
diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig index cb7e7697cf1e..ef7ffe6b56a0 100644 --- a/drivers/thermal/intel/Kconfig +++ b/drivers/thermal/intel/Kconfig @@ -112,6 +112,17 @@ config INTEL_MENLOW If unsure, say N. +config INTEL_MENLOW_SYSFS_ADDON + bool "Enable extra sysfs attributes in the thermal zone" + depends on INTEL_MENLOW + def_bool n + help + Create auxiliary trip points in the thermal zone sysfs + directory. This is specific to this driver. By default those + are disabled and are candidate for removal, if you need these + information anyway, enable the option or upgrade the + userspace program using them. + config INTEL_HFI_THERMAL bool "Intel Hardware Feedback Interface" depends on NET diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c index d720add918ff..605983be516c 100644 --- a/drivers/thermal/intel/intel_menlow.c +++ b/drivers/thermal/intel/intel_menlow.c @@ -367,6 +367,7 @@ static ssize_t bios_enabled_show(struct device *dev, return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled"); } +#ifdef CONFIG_INTEL_MENLOW_SYSFS_ADDON static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, void *store, struct device *dev, acpi_handle handle) @@ -398,6 +399,14 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, return 0; } +#else +static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show, + void *store, struct device *dev, + acpi_handle handle) +{ + return 0; +} +#endif static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl, void *context, void **rv)
The Menlon thermal driver creates auxiliary trip points in the thermal zone sysfs directory. It is specific to Menlon. Actually these trip points could be generalized with the generic trip points in the future. Let's make the code optional and disable it by default so we have a consistency with the attributes in the thermal zone sysfs directories. If that hurts we will enable by default this option instead of disabling it. Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- drivers/thermal/intel/Kconfig | 11 +++++++++++ drivers/thermal/intel/intel_menlow.c | 9 +++++++++ 2 files changed, 20 insertions(+)