Received-SPF: pass (google.com: domain of linux-kernel+bounces-71349-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Mon, 19 Feb 2024 14:51:48 +0200 (EET)
To: Gergo Koteles <soyer@irl.hu>, Hans de Goede <hdegoede@redhat.com>
cc: Ike Panhc <ike.pan@canonical.com>, platform-driver-x86@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] platform/x86: ideapad-laptop: support Fn+R dual-function
 key
In-Reply-To: <531e85d73c1926161eb15f8900ea77aade394b6f.camel@irl.hu>
Message-ID: <29ed8a8e-09f5-ec43-11bd-7ddd12ce08f8@linux.intel.com>
References: <0cdbc0e6eb65e160384ae0ed152e7de3ded1d9d5.1707604991.git.soyer@irl.hu>  <3b6a7bba-47a5-469c-aac1-5574ad78dadf@canonical.com> <531e85d73c1926161eb15f8900ea77aade394b6f.camel@irl.hu>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

On Mon, 19 Feb 2024, Gergo Koteles wrote:
> On Mon, 2024-02-19 at 10:39 +0800, Ike Panhc wrote:
> > On 2/11/24 06:51, Gergo Koteles wrote:
> > > According to the manual, Fn+R adjusts the display refresh rate.
> > > Map Fn+R to KEY_DISPLAYTOGGLE.
> > > 
> > > Signed-off-by: Gergo Koteles <soyer@irl.hu>

> > > +++ b/drivers/platform/x86/ideapad-laptop.c
> > > @@ -1091,6 +1091,8 @@ static const struct key_entry ideapad_keymap[] = {
> > >  	{ KE_KEY,	0x07 | IDEAPAD_WMI_KEY, { KEY_HELP } },
> > >  	{ KE_KEY,	0x0e | IDEAPAD_WMI_KEY, { KEY_PICKUP_PHONE } },
> > >  	{ KE_KEY,	0x0f | IDEAPAD_WMI_KEY, { KEY_HANGUP_PHONE } },
> > > +	/* Refresh Rate Toggle (Fn+R) */
> > > +	{ KE_KEY,	0x10 | IDEAPAD_WMI_KEY, { KEY_DISPLAYTOGGLE } },
> > >  	/* Dark mode toggle */
> > >  	{ KE_KEY,	0x13 | IDEAPAD_WMI_KEY, { KEY_PROG1 } },
> > >  	/* Sound profile switch */
> > 
> > Acked-by: Ike Panhc <ike.pan@canonical.com>
> > 
> > BTW on which ideapad we need this patch?
> > 
> 
> Oh, I somehow missed that. I found it on a Yoga 7 14ARB7.
> 
> Newer Yogas and Legions with 60Hz/90Hz displays have this refresh rate
> toggle feature.
> 
> I'm wondering if this would be worth a new KEY_REFRESH_RATE_TOGGLE
> event code? 
> 
> KEY_DISPLAYTOGGLE is used to toggle the LCD on/off in other drivers.
> 
> What do you think?

At least to me it felt like an abuse of KEY_DISPLAYTOGGLE because it's 
obviously different. But since there was existing, similar use for it 
already, I didn't push back on it but took your patch.

-- 
 i.