Re: [PATCH v7 3/3] platform/x86: dell-pc: Implement platform_profile

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 17 May 2024, Lyndon Sanche wrote:

> Some Dell laptops support configuration of preset fan modes through
> smbios tables.
> 
> If the platform supports these fan modes, set up platform_profile to
> change these modes. If not supported, skip enabling platform_profile.
> 
> Signed-off-by: Lyndon Sanche <lsanche@lyndeno.ca>
> ---
>  MAINTAINERS                                  |   6 +
>  drivers/platform/x86/dell/Kconfig            |  13 +
>  drivers/platform/x86/dell/Makefile           |   1 +
>  drivers/platform/x86/dell/dell-pc.c          | 310 +++++++++++++++++++
>  drivers/platform/x86/dell/dell-smbios-base.c |   1 +
>  drivers/platform/x86/dell/dell-smbios.h      |   1 +
>  6 files changed, 332 insertions(+)
>  create mode 100644 drivers/platform/x86/dell/dell-pc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ebf03f5f0619..69c582b72a08 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5961,6 +5961,12 @@ F:	Documentation/ABI/obsolete/procfs-i8k
>  F:	drivers/hwmon/dell-smm-hwmon.c
>  F:	include/uapi/linux/i8k.h
>  
> +DELL PC DRIVER
> +M:	Lyndon Sanche <lsanche@lyndeno.ca>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/dell/dell-pc.c
> +
>  DELL REMOTE BIOS UPDATE DRIVER
>  M:	Stuart Hayes <stuart.w.hayes@gmail.com>
>  L:	platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index bd9f445974cc..0732850a3dd6 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -91,6 +91,19 @@ config DELL_RBTN
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-rbtn.
>  
> +config DELL_PC
> +	tristate "Dell PC Extras"
> +	default m
> +	depends on ACPI
> +	depends on DMI
> +	depends on DELL_SMBIOS
> +	select ACPI_PLATFORM_PROFILE
> +	help
> +	This driver adds support for controlling the fan modes via platform_profile
> +	on supported Dell systems regardless of formfactor.
> +	Module will simply do nothing if thermal management commands are not
> +	supported.


> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/dmi.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/platform_profile.h>
> +#include <linux/slab.h>
> +#include "dell-smbios.h"

Add empty line between <> and "" includes.

> +static const struct dmi_system_id dell_device_table[] __initconst = {
> +	{
> +		.ident = "Dell Inc.",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +		},
> +	},
> +	{
> +		.ident = "Dell Computer Corporation",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
> +		},
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dmi, dell_device_table);

> +enum thermal_mode_bits {
> +	DELL_BALANCED = BIT(0),
> +	DELL_COOL_BOTTOM = BIT(1),
> +	DELL_QUIET = BIT(2),
> +	DELL_PERFORMANCE = BIT(3),

A few nits still to address.

Can you please align these so that the values align (IIRC, I asked this 
earlier but perhaps my request was too unclear):

	DELL_XX			= BIT(X),
	DELL_YYYYYYYYY		= BIT(Y),

> +static int thermal_get_supported_modes(int *supported_bits)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +
> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	if (ret) {
> +		/* Thermal function not supported */
> +		if (ret == -ENXIO) {
> +			*supported_bits = 0;
> +			return 0;
> +		} else {

Drop else because the previous block ends into return.

> +			return ret;
> +		}
> +	}

Remove the outer if (ret) block and put the inner ones directly on the 
main level as two if () conditions.

> +	*supported_bits = FIELD_GET(DELL_THERMAL_SUPPORTED, buffer.output[1]);
> +	return 0;
> +}
> +
> +static int thermal_get_acc_mode(int *acc_mode)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +
> +	dell_fill_request(&buffer, 0x0, 0, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	if (ret)
> +		return ret;
> +	*acc_mode = FIELD_GET(DELL_ACC_GET_FIELD, buffer.output[3]);
> +	return 0;
> +}
> +
> +static int thermal_set_mode(enum thermal_mode_bits state)
> +{
> +	struct calling_interface_buffer buffer;
> +	int ret;
> +	int acc_mode;
> +
> +	ret = thermal_get_acc_mode(&acc_mode);
> +	if (ret)
> +		return ret;
> +
> +	dell_fill_request(&buffer, 0x1, FIELD_PREP(DELL_ACC_SET_FIELD, acc_mode) | state, 0, 0);
> +	ret = dell_send_request(&buffer, CLASS_INFO, SELECT_THERMAL_MANAGEMENT);
> +	return ret;

Return directly on the previous line.

> +static int thermal_platform_profile_get(struct platform_profile_handler *pprof,
> +					enum platform_profile_option *profile)
> +{
> +	int ret;
> +
> +	ret = thermal_get_mode();
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case DELL_BALANCED:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case DELL_PERFORMANCE:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case DELL_COOL_BOTTOM:
> +		*profile = PLATFORM_PROFILE_COOL;
> +		break;
> +	case DELL_QUIET:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int thermal_init(void)
> +{
> +	int ret;
> +	int supported_modes;
> +
> +	/* If thermal commands not supported, exit without error */

Fix grammar, you're perhaps missing "are".

> +	if (!dell_smbios_class_is_supported(CLASS_INFO))
> +		return 0;
> +
> +	/* If thermal modes not supported, exit without error */

Ditto.

-- 
 i.