Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i5ec1447f:Fastmail
Date:   Sun, 07 Aug 2022 13:54:52 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH 1/5] asus-wmi: Add basic support for TUF laptop keyboard
 RGB
To:     =?iso-8859-2?q?Barnab=E1s_P=F5cze?= <pobrn@protonmail.com>
Cc:     hdegoede@redhat.com, markgross@kernel.org,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Message-Id: <GN28GR.5DRC7JSLXRDT3@ljones.dev>
In-Reply-To: <06B-xdz9pdNnj8DeXMMmdUi9Z7NAmQd3mFSHALyKMARSKZmal3FTLvXYKeF7SX_h78ko8RCpbC3t9wUanK6T2nNz1sYVtrSSGEcsfWAxtXY=@protonmail.com>
References: <20220805081909.10962-1-luke@ljones.dev>
        <20220805081909.10962-2-luke@ljones.dev>
        <06B-xdz9pdNnj8DeXMMmdUi9Z7NAmQd3mFSHALyKMARSKZmal3FTLvXYKeF7SX_h78ko8RCpbC3t9wUanK6T2nNz1sYVtrSSGEcsfWAxtXY=@protonmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-2; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Barnab=E1s,

Thank you for taking the time to review

On Sat, Aug 6 2022 at 17:30:04 +0000, Barnab=E1s P=F5cze=20
<pobrn@protonmail.com> wrote:
> Hi
>=20
>=20
> 2022. augusztus 5., p=E9ntek 10:19 keltez=E9ssel, Luke D. Jones=20
> <luke@ljones.dev> =EDrta:
>=20
>>  Adds support for TUF laptop RGB control via the multicolor LED API.
>>=20
>>  As this is the base essentials for adjusting the RGB, it sets the
>>  default mode of the keyboard to static. This overwrites the booted
>>  state of the keyboard.
>>=20
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c            | 89=20
>> ++++++++++++++++++++++
>>   include/linux/platform_data/x86/asus-wmi.h |  3 +
>>   2 files changed, 92 insertions(+)
>>=20
>>  diff --git a/drivers/platform/x86/asus-wmi.c=20
>> b/drivers/platform/x86/asus-wmi.c
>>  index 0e7fbed8a50d..33384e3321bb 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -25,6 +25,7 @@
>>   #include <linux/input/sparse-keymap.h>
>>   #include <linux/kernel.h>
>>   #include <linux/leds.h>
>>  +#include <linux/led-class-multicolor.h>
>>   #include <linux/module.h>
>>   #include <linux/pci.h>
>>   #include <linux/pci_hotplug.h>
>>  @@ -190,6 +191,11 @@ struct fan_curve_data {
>>   	u8 percents[FAN_CURVE_POINTS];
>>   };
>>=20
>>  +struct keyboard_rgb_led {
>>  +	struct led_classdev_mc dev;
>>  +	struct mc_subled subled_info[3]; /* r g b */
>>  +};
>>  +
>>   struct asus_wmi {
>>   	int dsts_id;
>>   	int spec;
>>  @@ -234,6 +240,9 @@ struct asus_wmi {
>>   	bool dgpu_disable_available;
>>   	bool dgpu_disable;
>>=20
>>  +	bool keyboard_rgb_mode_available;
>=20
> I think this variable could be introduced in the next patch, it is=20
> not used in this one.
>=20

Thank you for spotting this. I must have missed it when I split the=20
patches.

>=20
>>  +	struct keyboard_rgb_led keyboard_rgb_mode;
>>  +
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>>=20
>>  @@ -1028,6 +1037,35 @@ static enum led_brightness=20
>> lightbar_led_get(struct led_classdev *led_cdev)
>>   	return result & ASUS_WMI_DSTS_LIGHTBAR_MASK;
>>   }
>>=20
>>  +static int tuf_rgb_brightness_set(struct led_classdev *cdev,
>>  +	enum led_brightness brightness)
>>  +{
>>  +	u8 r, g, b;
>>  +	int err;
>>  +	u32 ret;
>>  +
>>  +	struct led_classdev_mc *mc_cdev =3D lcdev_to_mccdev(cdev);
>>  +
>>  +	led_mc_calc_color_components(mc_cdev, brightness);
>>  +	r =3D mc_cdev->subled_info[0].brightness;
>>  +	g =3D mc_cdev->subled_info[1].brightness;
>>  +	b =3D mc_cdev->subled_info[2].brightness;
>>  +
>>  +	/* Writing out requires some defaults. This will overwrite boot=20
>> mode */
>>  +	err =3D asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS,=20
>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>>  +			1 | 0 | (r << 16) | (g << 24), (b) | 0, &ret);
>>  +	if (err) {
>>  +		pr_err("Unable to set TUF RGB data?\n");
>>  +		return err;
>>  +	}
>>  +	return 0;
>>  +}
>>  +
>>  +static enum led_brightness tuf_rgb_brightness_get(struct=20
>> led_classdev *cdev)
>>  +{
>>  +	return cdev->brightness;
>>  +}
>=20
> If you can't query the brightness from the hardware, I think you can=20
> leave
> `led_classdev::brightness_get` to be `NULL`. This callback is only=20
> used from
> `led_update_brightness()` as far as I can see.
>=20

I did wonder about this. Thanks, I've nulled it.

>=20
>>  +
>>   static void asus_wmi_led_exit(struct asus_wmi *asus)
>>   {
>>   	led_classdev_unregister(&asus->kbd_led);
>>  @@ -1105,6 +1143,57 @@ static int asus_wmi_led_init(struct asus_wmi=20
>> *asus)
>>   					   &asus->lightbar_led);
>>   	}
>>=20
>>  +	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE)) {
>>  +		struct led_classdev_mc *mc_cdev;
>>  +		struct mc_subled *mc_led_info;
>>  +		u8 brightness =3D 127;
>>  +
>>  +		mc_cdev =3D &asus->keyboard_rgb_mode.dev;
>>  +
>>  +		/*
>>  +		 * asus::kbd_backlight still controls a base 3-level backlight=20
>> and when
>>  +		 * it is on 0, the RGB is not visible at all. RGB should be=20
>> treated as
>>  +		 * an additional step.
>>  +		 */
>>  +		mc_cdev->led_cdev.name =3D "asus::multicolour::kbd_backlight";
>>  +		mc_cdev->led_cdev.flags =3D LED_CORE_SUSPENDRESUME |=20
>> LED_RETAIN_AT_SHUTDOWN;
>>  +		mc_cdev->led_cdev.brightness_set_blocking =3D=20
>> tuf_rgb_brightness_set;
>>  +		mc_cdev->led_cdev.brightness_get =3D tuf_rgb_brightness_get;
>>  +
>>  +		/* Let the multicolour LED own the info */
>>  +		mc_led_info =3D devm_kmalloc_array(
>>  +			&asus->platform_device->dev,
>>  +			3,
>>  +			sizeof(*mc_led_info),
>>  +			GFP_KERNEL | __GFP_ZERO);
>>  +
>=20
> I am a bit confused as to why dynamic allocation is needed here.=20
> Haven't you
> already "allocated" the storage in `keyboard_rgb_led::subled_info`?

Honestly, I write rust 90% of the time which is quite clear on these=20
things, so here I wasn't very sure about what to do - I read the=20
hid-playstation.c and it had something similar so I used it and=20
completely forgot about the struct allocation.

I've updated to `struct mc_subled *mc_led_info =3D=20
asus->keyboard_rgb_mode.subled_info;` and removed the dynamic alloc.

>=20
>=20
>>  +		if (!mc_led_info)
>>  +			return -ENOMEM;
>>  +
>>  +		mc_led_info[0].color_index =3D LED_COLOR_ID_RED;
>>  +		mc_led_info[1].color_index =3D LED_COLOR_ID_GREEN;
>>  +		mc_led_info[2].color_index =3D LED_COLOR_ID_BLUE;
>>  +
>>  +		/*
>>  +		 * It's not possible to get last set data from device so set=20
>> defaults
>>  +		 * to make it safe for a user to change either RGB or modes. We=20
>> don't
>>  +		 * write these defaults to the device because they will=20
>> overwrite a
>>  +		 * users last saved boot setting (in NVRAM).
>>  +		 */
>>  +		mc_cdev->led_cdev.brightness =3D brightness;
>>  +		mc_cdev->led_cdev.max_brightness =3D brightness;
>>  +		mc_led_info[0].intensity =3D brightness;
>>  +		mc_led_info[0].brightness =3D mc_cdev->led_cdev.brightness;
>>  +		mc_led_info[1].brightness =3D mc_cdev->led_cdev.brightness;
>>  +		mc_led_info[2].brightness =3D mc_cdev->led_cdev.brightness;
>>  +		led_mc_calc_color_components(mc_cdev, brightness);
>>  +
>>  +		mc_cdev->subled_info =3D mc_led_info;
>>  +		mc_cdev->num_colors =3D 3;
>=20
> `led_mc_calc_color_components()` uses `led_classdev_mc::num_colors`,=20
> so I think
> it needs to be set before calling it. But that function sets the=20
> subled brightness
> based on the intensity, so it will overwrite the brightness values=20
> that have just
> been set.

Ah thanks, I don't think I understood that before. I've removed=20
mc_led_info[i].brightness setting now.

>=20
>=20
>>  +
>>  +		rv =3D=20
>> led_classdev_multicolor_register(&asus->platform_device->dev,=20
>> mc_cdev);
>>  +	}
>>  +
>>   error:
>>   	if (rv)
>>   		asus_wmi_led_exit(asus);
>>  diff --git a/include/linux/platform_data/x86/asus-wmi.h=20
>> b/include/linux/platform_data/x86/asus-wmi.h
>>  index a571b47ff362..d63c9945a17d 100644
>>  --- a/include/linux/platform_data/x86/asus-wmi.h
>>  +++ b/include/linux/platform_data/x86/asus-wmi.h
>>  @@ -98,6 +98,9 @@
>>   /* dgpu on/off */
>>   #define ASUS_WMI_DEVID_DGPU		0x00090020
>>=20
>>  +/* TUF laptop RGB control */
>>  +#define ASUS_WMI_DEVID_TUF_RGB_MODE	0x00100056
>>  +
>>   /* DSTS masks */
>>   #define ASUS_WMI_DSTS_STATUS_BIT	0x00000001
>>   #define ASUS_WMI_DSTS_UNKNOWN_BIT	0x00000002
>>  --
>>  2.37.1
>>=20
>>=20
>=20
>=20
> Regards,
> Barnab=E1s P=F5cze