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:   Fri, 26 Aug 2022 22:02:43 +1200
From:   Luke Jones <luke@ljones.dev>
Subject: Re: [PATCH v4 1/2] asus-wmi: Implement TUF laptop keyboard LED modes
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     markgross@kernel.org, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org, andy.shevchenko@gmail.com,
        pobrn@protonmail.com, pavel@ucw.cz
Message-Id: <JWV7HR.9II7K7ZK8G9M1@ljones.dev>
In-Reply-To: <4b196309-fb73-bc77-0096-81915ac9a97d@redhat.com>
References: <20220825232251.345893-1-luke@ljones.dev>
        <20220825232251.345893-2-luke@ljones.dev>
        <0a45a991-4123-a76c-174f-435523f6d348@redhat.com>
        <XMV7HR.FTECLY4PXEQX@ljones.dev>
        <4b196309-fb73-bc77-0096-81915ac9a97d@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Precedence: bulk


On Fri, Aug 26 2022 at 11:59:55 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/26/22 11:56, Luke Jones wrote:
>> 
>> 
>>  On Fri, Aug 26 2022 at 11:47:37 +0200, Hans de Goede 
>> <hdegoede@redhat.com> wrote:
>>>  Hi Luke,
>>> 
>>>  On 8/26/22 01:22, Luke D. Jones wrote:
>>>>   Adds support for changing the laptop keyboard LED mode and 
>>>> colour.
>>>> 
>>>>   The modes are visible effects such as static, rainbow, pulsing,
>>>>   colour cycles.
>>>> 
>>>>   These sysfs attributes are added to asus::kbd_backlight:
>>>>   - kbd_rgb_mode
>>>>   - kbd_rgb_mode_index
>>>> 
>>>>   Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>>   ---
>>>>    drivers/platform/x86/asus-wmi.c            | 76 
>>>> +++++++++++++++++++++-
>>>>    include/linux/platform_data/x86/asus-wmi.h |  3 +
>>>>    2 files changed, 78 insertions(+), 1 deletion(-)
>>>> 
>>>>   diff --git a/drivers/platform/x86/asus-wmi.c 
>>>> b/drivers/platform/x86/asus-wmi.c
>>>>   index 0f9f79f249c7..92f16bb9b4ef 100644
>>>>   --- a/drivers/platform/x86/asus-wmi.c
>>>>   +++ b/drivers/platform/x86/asus-wmi.c
>>>>   @@ -237,6 +237,8 @@ struct asus_wmi {
>>>>        bool dgpu_disable_available;
>>>>        bool gpu_mux_mode_available;
>>>> 
>>>>   +    bool kbd_rgb_mode_available;
>>>>   +
>>>>        bool throttle_thermal_policy_available;
>>>>        u8 throttle_thermal_policy_mode;
>>>> 
>>>>   @@ -720,6 +722,69 @@ static ssize_t gpu_mux_mode_store(struct 
>>>> device *dev,
>>>>    }
>>>>    static DEVICE_ATTR_RW(gpu_mux_mode);
>>>> 
>>>>   +/* TUF Laptop Keyboard RGB Modes 
>>>> **********************************************/
>>>>   +static ssize_t kbd_rgb_mode_store(struct device *dev,
>>>>   +                 struct device_attribute *attr,
>>>>   +                 const char *buf, size_t count)
>>>>   +{
>>>>   +    u32 cmd, mode, r, g,  b,  speed;
>>>>   +    int err;
>>>>   +
>>>>   +    if (sscanf(buf, "%d %d %d %d %d %d", &cmd, &mode, &r, &g, 
>>>> &b, &speed) != 6)
>>>>   +        return -EINVAL;
>>>>   +
>>>>   +    cmd = !!cmd;
>>>>   +
>>>>   +    /* These are the known usable modes across all TUF/ROG */
>>>>   +    if (mode >= 12 || mode == 9)
>>>>   +        mode = 10;
>>>>   +
>>>>   +    switch (speed) {
>>>>   +    case 0:
>>>>   +        speed = 0xe1;
>>>>   +        break;
>>>>   +    case 1:
>>>>   +        speed = 0xeb;
>>>>   +        break;
>>>>   +    case 2:
>>>>   +        speed = 0xf5;
>>>>   +        break;
>>>>   +    default:
>>>>   +        speed = 0xeb;
>>>>   +    }
>>>>   +
>>>>   +    err = asus_wmi_evaluate_method3(ASUS_WMI_METHODID_DEVS, 
>>>> ASUS_WMI_DEVID_TUF_RGB_MODE,
>>>>   +            cmd | (mode << 8) | (r << 16) | (g << 24), b | 
>>>> (speed << 8), NULL);
>>>>   +    if (err)
>>>>   +        return err;
>>>>   +
>>>>   +    return count;
>>>>   +}
>>>>   +static DEVICE_ATTR_WO(kbd_rgb_mode);
>>>>   +
>>>>   +static ssize_t kbd_rgb_mode_index_show(struct device *device,
>>>>   +                         struct device_attribute *attr,
>>>>   +                         char *buf)
>>>>   +{
>>>>   +    return sysfs_emit(buf, "%s\n", "cmd mode red green blue 
>>>> speed");
>>>>   +}
>>>>   +static DEVICE_ATTR_RO(kbd_rgb_mode_index);
>>>>   +
>>>>   +static struct attribute *kbd_rgb_mode_attrs[] = {
>>>>   +    &dev_attr_kbd_rgb_mode.attr,
>>>>   +    &dev_attr_kbd_rgb_mode_index.attr,
>>>>   +    NULL,
>>>>   +};
>>>>   +
>>>>   +static const struct attribute_group kbd_rgb_mode_group = {
>>>>   +    .attrs = kbd_rgb_mode_attrs,
>>>>   +};
>>>>   +
>>>>   +const struct attribute_group *kbd_rgb_mode_groups[] = {
>>>>   +    NULL,
>>>>   +    NULL,
>>>>   +};
>>>>   +
>>>>    /* Battery 
>>>> ********************************************************************/
>>>> 
>>>>    /* The battery maximum charging percentage */
>>>>   @@ -1038,7 +1103,10 @@ static void asus_wmi_led_exit(struct 
>>>> asus_wmi *asus)
>>>> 
>>>>    static int asus_wmi_led_init(struct asus_wmi *asus)
>>>>    {
>>>>   -    int rv = 0, led_val;
>>>>   +    int rv = 0, num_rgb_groups = 0, led_val;
>>>>   +
>>>>   +    if (asus->kbd_rgb_mode_available)
>>>>   +        kbd_rgb_mode_groups[num_rgb_groups++] = 
>>>> &kbd_rgb_mode_group;
>>>> 
>>>>        asus->led_workqueue = 
>>>> create_singlethread_workqueue("led_workqueue");
>>>>        if (!asus->led_workqueue)
>>>>   @@ -1066,6 +1134,9 @@ static int asus_wmi_led_init(struct 
>>>> asus_wmi *asus)
>>>>            asus->kbd_led.brightness_get = kbd_led_get;
>>>>            asus->kbd_led.max_brightness = 3;
>>>> 
>>>>   +        if (num_rgb_groups != 0)
>>>>   +            asus->kbd_led.groups = kbd_rgb_mode_groups;
>>>>   +
>>>>            rv = led_classdev_register(&asus->platform_device->dev,
>>>>                           &asus->kbd_led);
>>>>            if (rv)
>>>>   @@ -3253,6 +3324,8 @@ static umode_t 
>>>> asus_sysfs_is_visible(struct kobject *kobj,
>>>>            ok = asus->egpu_enable_available;
>>>>        else if (attr == &dev_attr_dgpu_disable.attr)
>>>>            ok = asus->dgpu_disable_available;
>>>>   +    else if (attr == &dev_attr_dgpu_disable.attr)
>>>>   +        ok = asus->dgpu_disable_available;
>>>>        else if (attr == &dev_attr_gpu_mux_mode.attr)
>>>>            ok = asus->gpu_mux_mode_available;
>>>>        else if (attr == &dev_attr_fan_boost_mode.attr)
>>> 
>>>  This patch-hunk looks like it is a (mangled) leftover from 
>>> previous versions
>>>  of the patch.
>> 
>>  Do you mean:
>> 
>>  +    else if (attr == &dev_attr_dgpu_disable.attr)
>>  +        ok = asus->dgpu_disable_available;
> 
> Yes, patch hunks or chunks are the parts of a patch between
> a "@@ ... @@" line and the next "@@ ... @@" line.

Thanks. Oh gosh i see now... dgpu_disable. I'm unsure how I missed 
that. My apologies..

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
>>>>   @@ -3519,6 +3592,7 @@ static int asus_wmi_add(struct 
>>>> platform_device *pdev)
>>>>        asus->egpu_enable_available = asus_wmi_dev_is_present(asus, 
>>>> ASUS_WMI_DEVID_EGPU);
>>>>        asus->dgpu_disable_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_DGPU);
>>>>        asus->gpu_mux_mode_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_GPU_MUX);
>>>>   +    asus->kbd_rgb_mode_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_TUF_RGB_MODE);
>>>>        asus->panel_overdrive_available = 
>>>> asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
>>>> 
>>>>        err = fan_boost_mode_check_present(asus);
>>>>   diff --git a/include/linux/platform_data/x86/asus-wmi.h 
>>>> b/include/linux/platform_data/x86/asus-wmi.h
>>>>   index 6e8a95c10d17..3d861477cb20 100644
>>>>   --- a/include/linux/platform_data/x86/asus-wmi.h
>>>>   +++ b/include/linux/platform_data/x86/asus-wmi.h
>>>>   @@ -103,6 +103,9 @@
>>>>    /* gpu mux switch, 0 = dGPU, 1 = Optimus */
>>>>    #define ASUS_WMI_DEVID_GPU_MUX        0x00090016
>>>> 
>>>>   +/* TUF laptop RGB modes/colours */
>>>>   +#define ASUS_WMI_DEVID_TUF_RGB_MODE    0x00100056
>>>>   +
>>>>    /* DSTS masks */
>>>>    #define ASUS_WMI_DSTS_STATUS_BIT    0x00000001
>>>>    #define ASUS_WMI_DSTS_UNKNOWN_BIT    0x00000002
>>> 
>> 
>> 
>