Message-ID: <8e46e7cc-eb3d-390d-f411-8a15b0d8d22c@linux.intel.com>
Date: Mon, 11 Mar 2024 13:22:35 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: mustafa <mustafa.eskieksi@...il.com>
cc: Hans de Goede <hdegoede@...hat.com>, jdelvare@...e.com, linux@...ck-us.net, 
    LKML <linux-kernel@...r.kernel.org>, platform-driver-x86@...r.kernel.org, 
    linux-hwmon@...r.kernel.org, pavel@....cz, lee@...nel.org, 
    linux-leds@...r.kernel.org, rishitbansal0@...il.com
Subject: Re: [PATCH v3 1/1] platform/x86: Add wmi driver for Casper Excalibur
 laptops

On Sun, 10 Mar 2024, mustafa wrote:

> From: Mustafa Ekşi <mustafa.eskieksi@...il.com>
> 
> This wmi driver supports Casper Excalibur laptops' changing keyboard
> backlight, reading fan speeds and changing power profiles. Multicolor
> led device is used for backlight, platform_profile for power management
> and hwmon for fan speeds. It supports both old (10th gen or older) and
> new (11th gen or newer) laptops. It uses x86_match_cpu to check if the
> laptop is old or new.
> This driver's Multicolor keyboard backlight API is very similar to Rishit
> Bansal's proposed API.
> 
> Signed-off-by: Mustafa Ekşi <mustafa.eskieksi@...il.com>
> ---
> Changes in v3:
>  - Replaced led_control attribute with multicolor led interface.
>  - Added struct led_cache, instead of storing only last color change.
>  - Added dmi list to prevent registering platform_profile driver in models
>    that doesn't have this feature.
>  - Added a x86_cpu_id to differentiate older laptops that are reporting
>    fan speeds in big-endian. Also newer laptops have a different power
>    profile scheme. I'm using x86_cpu_id because they don't have a
>    difference in model names, only in cpu generations (the official driver
>    download page makes you select your cpu's generation too).
>  - Removed hwmon_pwm device in favor of platform_profile driver. It
>    indirectly affects fans' speed but they also affect frequency and
>    power consumption as well.
>  - Replaced handwritten masks with GENMASK equivalents.
>  - Replaced led_classdev_register with
>    devm_led_classdev_multicolor_register. This should solve the bug
>    where led_classdev remains registered even if casper_wmi_probe
>    returns -ENODEV.
>  - Removed select NEW_LEDS and LEDS_CLASS, because it creates recursive
>    dependencies.
>  - And some minor changes.
> Changes in v2:
>  - Added masks for
>  - Changed casper_set and casper_query returns Linux error code rather than
>    acpi_status.
>  - replaced complicated bit operations with FIELD_GET.
>  - Fixed some indentation and spacing.
>  - Broke fan speeds further.
>  - Moved module metadata to the end of the file.
> ---
>  MAINTAINERS                       |   6 +
>  drivers/platform/x86/Kconfig      |  14 +
>  drivers/platform/x86/Makefile     |   1 +
>  drivers/platform/x86/casper-wmi.c | 639 ++++++++++++++++++++++++++++++
>  4 files changed, 660 insertions(+)
>  create mode 100644 drivers/platform/x86/casper-wmi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1aabf1c15bb..e4cb770c990 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4724,6 +4724,12 @@ S:	Maintained
>  W:	https://wireless.wiki.kernel.org/en/users/Drivers/carl9170
>  F:	drivers/net/wireless/ath/carl9170/
>  
> +CASPER EXCALIBUR WMI DRIVER
> +M:	Mustafa Ekşi <mustafa.eskieksi@...il.com>
> +L:	platform-driver-x86@...r.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/casper-wmi.c
> +
>  CAVIUM I2C DRIVER
>  M:	Robert Richter <rric@...nel.org>
>  S:	Odd Fixes
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index bdd302274b9..4f951bcac1a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1127,6 +1127,20 @@ config SEL3350_PLATFORM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called sel3350-platform.
>  
> +config CASPER_WMI
> +	tristate "Casper Excalibur Laptop WMI driver"
> +	depends on ACPI_WMI
> +	depends on HWMON
> +	depends on LEDS_CLASS_MULTICOLOR
> +	select ACPI_PLATFORM_PROFILE
> +	help
> +	  Say Y here if you want to support WMI-based fan speed reporting,
> +	  power management and keyboard backlight support on Casper Excalibur
> +	  Laptops.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called casper-wmi.
> +
>  endif # X86_PLATFORM_DEVICES
>  
>  config P2SB
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 1de432e8861..4b527dd44ad 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MXM_WMI)			+= mxm-wmi.o
>  obj-$(CONFIG_NVIDIA_WMI_EC_BACKLIGHT)	+= nvidia-wmi-ec-backlight.o
>  obj-$(CONFIG_XIAOMI_WMI)		+= xiaomi-wmi.o
>  obj-$(CONFIG_GIGABYTE_WMI)		+= gigabyte-wmi.o
> +obj-$(CONFIG_CASPER_WMI)		+= casper-wmi.o
>  
>  # Acer
>  obj-$(CONFIG_ACERHDF)		+= acerhdf.o
> diff --git a/drivers/platform/x86/casper-wmi.c b/drivers/platform/x86/casper-wmi.c
> new file mode 100644
> index 00000000000..80e1e2b16fb
> --- /dev/null
> +++ b/drivers/platform/x86/casper-wmi.c
> @@ -0,0 +1,639 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/module.h>
> +#include <linux/bits.h>
> +#include <linux/bitops.h>
> +#include <linux/acpi.h>
> +#include <linux/leds.h>
> +#include <linux/slab.h>
> +#include <linux/wmi.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +#include <acpi/acexcep.h>
> +#include <linux/bitfield.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_profile.h>
> +#include <linux/led-class-multicolor.h>
> +
> +#include <linux/dmi.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +
> +#define CASPER_WMI_GUID "644C5791-B7B0-4123-A90B-E93876E0DAAD"
> +
> +#define CASPER_READ 0xfa00
> +#define CASPER_WRITE 0xfb00
> +#define CASPER_GET_HARDWAREINFO 0x0200
> +#define CASPER_SET_LED 0x0100
> +#define CASPER_POWERPLAN 0x0300
> +
> +#define CASPER_KEYBOARD_LED_1 0x03
> +#define CASPER_KEYBOARD_LED_2 0x04
> +#define CASPER_KEYBOARD_LED_3 0x05
> +#define CASPER_ALL_KEYBOARD_LEDS 0x06
> +#define CASPER_CORNER_LEDS 0x07
> +#define CASPER_LED_COUNT 4
> +
> +const char * const zone_names[CASPER_LED_COUNT] = {
> +	"casper::kbd_zoned_backlight-right",
> +	"casper::kbd_zoned_backlight-middle",
> +	"casper::kbd_zoned_backlight-left",
> +	"casper::kbd_zoned_backlight-corners",
> +};
> +
> +#define CASPER_LED_ALPHA GENMASK(31, 24)
> +#define CASPER_LED_RED	 GENMASK(23, 16)
> +#define CASPER_LED_GREEN GENMASK(15, 8)
> +#define CASPER_LED_BLUE  GENMASK(7, 0)
> +#define CASPER_DEFAULT_COLOR (CASPER_LED_RED | CASPER_LED_GREEN | \
> +			      CASPER_LED_BLUE)
> +#define CASPER_FAN_CPU 0
> +#define CASPER_FAN_GPU 1
> +
> +enum casper_power_profile_old {
> +	CASPER_HIGH_PERFORMANCE = 1,
> +	CASPER_GAMING		= 2,
> +	CASPER_TEXT_MODE	= 3,
> +	CASPER_POWERSAVE	= 4
> +};
> +
> +enum casper_power_profile_new {
> +	CASPER_NEW_HIGH_PEROFRMANCE	= 0,

PERFORMANCE


> +	ret = casper_query(wdev, CASPER_GET_HARDWAREINFO, &out);
> +	if (ret)
> +		return ret;
> +
> +	switch (channel) {
> +	case CASPER_FAN_CPU:
> +		if (quirk_applied->big_endian_fans)
> +			*val = be16_to_cpu((u16) out.a4);

So you use be16_to_cpu() but you cast the input to u16, not __be16!?!

Please run sparse with endianness checking enabled and make sure it won't 
complain on these lines.


> +	gen_id = x86_match_cpu(casper_gen);
> +	if (!gen_id)
> +		return -ENODEV;
> +
> +	quirk_applied = (struct casper_quirk_entry *) gen_id->driver_data;

Don't leave space after casts. There could 

> +	dmi_id = dmi_first_match(casper_quirks);
> +	if (!dmi_id)
> +		return -ENODEV;
> +
> +	quirk_applied->no_power_profiles = ((struct casper_quirk_entry *)
> +		dmi_id->driver_data)->no_power_profiles;

Please make a local variable for struct casper_quirk_entry * instead of 
trying to cram this into a single, multi-line statement.

> +	casper_kbd_mcled_info = devm_kzalloc(&wdev->dev,
> +		sizeof(*casper_kbd_mcled_info)*CASPER_LED_COUNT, GFP_KERNEL);

devm_kcalloc()


I'm yet to go through the use of multicolor led stuff (I don't have time 
for that now and I'm not familiar enough with it to check it quickly). 
But hopefully we get one of the leds people to take a look at it before I 
do, their input would be much more valuable than my review.

-- 
 i.

Powered by blists - more mailing lists