Received-SPF: pass (google.com: domain of linux-kernel+bounces-98288-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: mustafa <mustafa.eskieksi@gmail.com>
To: hdegoede@redhat.com,
	ilpo.jarvinen@linux.intel.com,
	mustafa.eskieksi@gmail.com,
	jdelvare@suse.com,
	linux@roeck-us.net,
	linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org,
	linux-hwmon@vger.kernel.org,
	pavel@ucw.cz,
	lee@kernel.org,
	linux-leds@vger.kernel.org
Cc: rishitbansal0@gmail.com
Subject: [PATCH v3 0/1] platform/x86: Add wmi driver for Casper Excalibur laptops
Date: Sun, 10 Mar 2024 21:14:28 +0300
Message-ID: <20240310181429.59451-1-mustafa.eskieksi@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

From: Mustafa Ekşi <mustafa.eskieksi@gmail.com>

Hi,
I apologize for the delay. I submitted v2 in a hurry and it was
catastrophic (especially the subject), so I didn't want the same thing to
happen in v3.

On 23.02.2024 13:14, Ilpo Järvinen wrote:
> However, I still suspect this is wrong way to do RGB leds and the multi_*
> sysfs interface is the way you should use.
I was skeptical about using multicolor because it said it wasn't a good
fit for rgb drivers in drivers/leds/TODO but after I read the thread about
hp omen backlight driver support, I changed my mind. Hp omen's keyboard
backlight is very similar to Casper's keyboard backlight. And probably
TODO file meant "per key rgb"s and not zoned rgbs.
I think Rishit's' driver didn't get into mainline but I still liked the
API he used so I decided to use the same API.
Driver creates 4 different led_classdev_mc devices:
> casper::kbd_zoned_backlight-right/
> casper::kbd_zoned_backlight-middle/
> casper::kbd_zoned_backlight-left/
> casper::kbd_zoned_backlight-corners/
right, middle, and left leds share the same brightness but the corners
led doesn't.
I found a way to get the brightness level from the hardware, so it shows
the correct brightness when it is changed via keyboard shortcut. But
still, there's most likely no way to get color data from firmware,
Windows driver uses a Windows registry (as a cache) for same reason.

I seek your advice on how to support "led modes". It is very different
from led_pattern. It is not possible to change the interval, or anything.
This version has an enum called casper_led_mode:
> enum casper_led_mode {
>        LED_NORMAL = 0x10,
>        LED_BLINK = 0x20,
>        LED_FADE = 0x30,
>        LED_HEARTBEAT = 0x40,
>        LED_REPEAT = 0x50,
>        LED_RANDOM = 0x60
> };
For example, random mode assigns random colors to leds every second. Fade
mode slowly fades out brightness and then fades in. I thought of creating
an attribute but working with multiple leds seemed uneasy.

And also this multicolor approach doesn't include a way to set all
keyboard leds all at once (like Rishit's driver). This can create long
delays when a userspace program (like OpenRGB) sets all keyboard leds
to user configuration.

On 23.02.2024 03:13, Guenter Roeck wrote:
>> +        return -ENODEV;
>> +    hwmon_dev = devm_hwmon_device_register_with_info(&wdev->dev,
>> +                        "casper_wmi", wdev,
>> +                        &casper_wmi_hwmon_chip_info,
>> +                        NULL);
>> +    return PTR_ERR_OR_ZERO(hwmon_dev);
>
> Why use devm_hwmon_device_register_with_info() but not
> devm_led_classdev_register() ?
I use devm for everything now, and I think it unregisters them if probe
returns -ENODEV, I tested it with hwmon and it didn't crash and
unregistered successfully.

On 23.02.2024 05:54, Kuppuswamy Sathyanarayanan wrote:
>> +static umode_t casper_wmi_hwmon_is_visible(const void *drvdata,
>> +					   enum hwmon_sensor_types type,
>> +					   u32 attr, int channel)
>> +{
>> +	switch (type) {
>> +	case hwmon_fan:
>> +		return 0444;
>
> How about S_IRUSR | S_IRGRP | S_IROTH ?
checkpatch.pl suggests me to not use those macros:
> SYMBOLIC_PERMS: Symbolic permissions 'S_IRUSR | S_IRGRP | S_IROTH'
> are not preferred. Consider using octal permissions '0444'.
So I think it is better to use octal permissions.

On 23.02.2024 13:14, Ilpo Järvinen wrote:
> v1 -> v2 changelog is missing from here!
I added both v2 and v3 changelog in this patch.

On 23.02.2024 13:14, Ilpo Järvinen wrote:
>> +	acpi_status ret = wmidev_block_set(wdev, 0, &input);
> Put the declaration separately into the declarations block:
>
> 	acpi_status ret;
>
> 	ret = wmidev_block_set(wdev, 0, &input);
I followed this except with to_wmi_device and container_of because
you also put to_wmi_device in the declaration block.

Link to Rishit Bansal's thread:
https://lore.kernel.org/lkml/20230131235027.36304-1-rishitbansal0@gmail.com/

Thanks for your reviews and patience,
Mustafa Ekşi

Mustafa Ekşi (1):
  platform/x86: Add wmi driver for Casper Excalibur laptops

 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

-- 
2.44.0