From: Denis Benato <benato.denis96@gmail.com>
To: alex@bogusta.com, Denis Benato <denis.benato@linux.dev>
Cc: "Corentin Chary" <corentin.chary@gmail.com>,
"Luke D. Jones" <luke@ljones.dev>,
"Hans de Goede" <hansg@kernel.org>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Benjamin Tissoires" <bentiss@kernel.org>,
"Antheas Kapenekakis" <lkml@antheas.dev>,
platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] platform/x86: asus-wmi: Clamp keyboard backlight level from hotkeys
Date: Sun, 25 Oct 2026 13:29:00 +0100 [thread overview]
Message-ID: <6e029169-62e5-48da-91cc-8f653b2acbcc@gmail.com> (raw)
In-Reply-To: <Lw2-T3d0vVWwlA_msvm_zjVUms821VDv_EKarQwaugY0E6UtkHDIuZ1a4UQgNeto_RnoB2xxxk43SWSepIWBSKdvKD36G1ss4G-TkuyZv4w=@bogusta.com>
On 10/25/26 06:46, alex@bogusta.com wrote:
> Hi Denis,
> thanks for the review!
>
> On Monday, October 19th, 2026 at 5:52 PM, Denis Benato <denis.benato@linux.dev> wrote:
>
>> Is the change of type really necessary? I think clamp_value does everything required already.
> Hi Denis, thanks for the reply,
>
> clamp_val() without the type change wouldn't fix the problem, as
> at level 0, led_value - 1 is an unsigned type and wraps around. clamp_val() then clamps this to 3, setting the leds to max
> brightness
>
>> Honestly I don't like the change of type... I would prefer something that prevents the
>> bad thing from happening without changing the type.
> My reasoning for int was consistency, as asus_hid_event() and
> do_kbd_led_set() already do this arithmetic as int, and the comment
> on enum led_brightness in leds.h describes it as obsolete.
>
> That said, if you prefer keeping the enum, one could avoid the
> wraparound instead with the call
>
> kbd_led_set_by_kbd(asus, led_value ? led_value - 1 : 0);
>
> and keep the clamp in kbd_led_set_by_kbd() for the upper bound.
I see where this is going: either math done on an enum or an int converted to an enum.
I personally like neither, but any of the two will do to get the issue solved, so... If you happen
to make another version please add a comment somewhere to outline it so nobody gets the idea of
making the code looks better at the expense of functionality. Either with that comment or not:
Reviewed-by: Denis Benato <denis.benato@linux.dev>
> Thanks,
> Alex
Best regards,
Denis Benato
prev parent reply other threads:[~2026-10-25 12:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-10-16 1:51 Alex Bogusta
2026-10-19 21:51 ` Denis Benato
2026-10-25 5:46 ` alex
2026-10-25 12:29 ` Denis Benato [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6e029169-62e5-48da-91cc-8f653b2acbcc@gmail.com \
--to=benato.denis96@gmail.com \
--cc=alex@bogusta.com \
--cc=bentiss@kernel.org \
--cc=corentin.chary@gmail.com \
--cc=denis.benato@linux.dev \
--cc=hansg@kernel.org \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lkml@antheas.dev \
--cc=luke@ljones.dev \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®