mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
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

      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®