Re: [PATCH v3 6/6] thermal/drivers/intel_menlow: Make additionnal sysfs information optional

[Rafael J. Wysocki]

On Thu, Apr 13, 2023 at 1:47 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
>
> The Menlon thermal driver creates specific files in the thermal zone
> sysfs class. It is specific to Menlon and these entries look debug
> code. It is probable these are not needed.
>
> Let's make the code optional and disable it by default.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
>  drivers/thermal/intel/Kconfig        | 11 +++++++++++
>  drivers/thermal/intel/intel_menlow.c |  3 +++
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index cb7e7697cf1e..1405d9cd2bab 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 thermal sysfs addon"
> +       depends on INTEL_MENLOW
> +       def_bool n
> +       help
> +        Enable sysfs extra information added in the thermal

This should be something like "Enable extra sysfs attributes in the
thermal zone."

> +        zone. 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.

The above part of the help text doesn't add much value IMV.  It would
be far better to say what the extra attributes are for instead.

> +
>  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..a169d7e4f537 100644
> --- a/drivers/thermal/intel/intel_menlow.c
> +++ b/drivers/thermal/intel/intel_menlow.c
> @@ -374,6 +374,9 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
>         struct intel_menlow_attribute *attr;
>         int result;
>
> +       if (!IS_ENABLED(CONFIG_INTEL_MENLOW_SYSFS_ADDON))
> +               return 0;

IMV it would be cleaner to put all of the relevant code under the
Kconfig option, so it is clear that it goes away when that option is
not set.

I'm not sure what the compiler will do with it in the current form.

> +
>         attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
>         if (!attr)
>                 return -ENOMEM;
> --