From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 579E925DCFD; Thu, 26 Jun 2025 18:58:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750964296; cv=none; b=O4TweabDThDBIs4VjoSPqU3WJB0ODdG7F3cEUg8vcGqqX/BwQgfzlDsDvI7r1UPlc+squ4JTsGRvdMJH290lLNNh/zkRLWSiWcOLZCAgdNuokdvrFu4doUvdUnYg1q4oI/oe/RIE66ldbluYbAmATVY3f88zB3QEzryGE+pBTxA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750964296; c=relaxed/simple; bh=VuGdCO3UIva2y3pUx7i14W0KejS0P5aGUDN2EygFrdU=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=d6bWqfZbXPffNp81VTYViEaBm6A2qTUyoV7YwCIjiAzxASXyE97Ny+1z60aAuTlxmDcuAZj1CHc6xkSswsW8+QLY9Coty9/Z3KrCBoX8iJ9oiq99Mafy93m9Hd5STATSYSlloT21RyFKKAb5E+daX6nhloWb7m17jbxigH/8HRI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=U5fzGcgT; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="U5fzGcgT" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E48D7C4CEEB; Thu, 26 Jun 2025 18:58:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1750964296; bh=VuGdCO3UIva2y3pUx7i14W0KejS0P5aGUDN2EygFrdU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=U5fzGcgToOg1k6OP+Kk7KkJRXM1qcE0oFZaU7eOl7gfCRVqMAZHYvgwTMBU+5+CMY 1FpBjjP+ZmPzO465fbgJtv7Yy2kvPRLMsjVReQ9VaeQxSyfggthbkNQW0+sLXz2liq xqC26eaLDzPGH5NJWY3kfJ2QqNyjjJ2/J+sipJU2Sgz1Pg9b8hMiJXbX+JXYQ7CYG1 HS3k86UNlXONgQpm35x1FIDbqPtwuCnt8SzH70Ew9a9dca+00EBVQzAwnlLo8uUR/K nFdA/K/SQ3qVl1Qcbg4apnZuBD9KSa50OlfZbpOZTlxmWkz3vaaQsXdoVkEYV3czvZ 0fFP0Enr+O1YQ== Message-ID: <6b51c18a-c8e9-40c4-8a6b-07f2af1cb531@kernel.org> Date: Thu, 26 Jun 2025 13:58:14 -0500 Precedence: bulk X-Mailing-List: linux-input@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 3/3] Input: soc_button_array: Only debounce cherryview and baytrail systems To: Dmitry Torokhov Cc: Hans de Goede , Mika Westerberg , Andy Shevchenko , Linus Walleij , Bartosz Golaszewski , "open list:GPIO ACPI SUPPORT" , "open list:GPIO ACPI SUPPORT" , open list , "open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)..." , Mario Limonciello References: <20250625181342.3175969-1-superm1@kernel.org> <20250625181342.3175969-4-superm1@kernel.org> <57e9b1d5-faf1-4c7a-87fc-047e0dc102f9@kernel.org> <8fc9051f-bef3-43fc-83a1-172a0eb599dc@kernel.org> <55b4cd56-1812-4048-bf16-4b5b94a842d7@kernel.org> Content-Language: en-US From: Mario Limonciello In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 6/26/2025 1:53 PM, Dmitry Torokhov wrote: > On Thu, Jun 26, 2025 at 01:30:15PM -0500, Mario Limonciello wrote: >> On 6/26/2025 1:27 PM, Dmitry Torokhov wrote: >>> On Wed, Jun 25, 2025 at 03:34:07PM -0500, Mario Limonciello wrote: >>>> On 6/25/25 2:42 PM, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 25-Jun-25 9:23 PM, Mario Limonciello wrote: >>>>>> On 6/25/25 2:03 PM, Hans de Goede wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 25-Jun-25 8:13 PM, Mario Limonciello wrote: >>>>>>>> From: Mario Limonciello >>>>>>>> >>>>>>>> commit 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons") >>>>>>>> hardcoded all soc-button-array devices to use a 50ms debounce timeout >>>>>>>> but this doesn't work on all hardware.  The hardware I have on hand >>>>>>>> actually prescribes in the ASL that the timeout should be 0: >>>>>>>> >>>>>>>> GpioInt (Edge, ActiveBoth, Exclusive, PullUp, 0x0000, >>>>>>>>           "\\_SB.GPIO", 0x00, ResourceConsumer, ,) >>>>>>>> {   // Pin list >>>>>>>>      0x0000 >>>>>>>> } >>>>>>>> >>>>>>>> Many cherryview and baytrail systems don't have accurate values in the >>>>>>>> ASL for debouncing and thus use software debouncing in gpio_keys. The >>>>>>>> value to use is programmed in soc_button_array.  Detect Cherry View >>>>>>>> and Baytrail using ACPI HID IDs used for those GPIO controllers and apply >>>>>>>> the 50ms only for those systems. >>>>>>>> >>>>>>>> Cc: Hans de Goede >>>>>>>> Fixes: 5c4fa2a6da7fb ("Input: soc_button_array - debounce the buttons") >>>>>>>> Signed-off-by: Mario Limonciello >>>>>>> >>>>>>> I'm not a fan of this approach, I believe that we need to always debounce >>>>>>> when dealing with mechanical buttons otherwise we will get unreliable / >>>>>>> spurious input events. >>>>>>> >>>>>>> My suggestion to deal with the issue where setting up debouncing at >>>>>>> the GPIO controller level is causing issues is to always use software >>>>>>> debouncing (which I suspect is what Windows does). >>>>>>> >>>>>>> Let me copy and pasting my reply from the v1 thread with >>>>>>> a bit more detail on my proposal: >>>>>>> >>>>>>> My proposal is to add a "no_hw_debounce" flag to >>>>>>> struct gpio_keys_platform_data and make the soc_button_array >>>>>>> driver set that regardless of which platform it is running on. >>>>>>> >>>>>>> And then in gpio_keys.c do something like this: >>>>>>> >>>>>>> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c >>>>>>> index f9db86da0818..2788d1e5782c 100644 >>>>>>> --- a/drivers/input/keyboard/gpio_keys.c >>>>>>> +++ b/drivers/input/keyboard/gpio_keys.c >>>>>>> @@ -552,8 +552,11 @@ static int gpio_keys_setup_key(struct platform_device *pdev, >>>>>>>           bool active_low = gpiod_is_active_low(bdata->gpiod); >>>>>>>             if (button->debounce_interval) { >>>>>>> -            error = gpiod_set_debounce(bdata->gpiod, >>>>>>> -                    button->debounce_interval * 1000); >>>>>>> +            if (ddata->pdata->no_hw_debounce) >>>>>>> +                error = -EINVAL; >>>>>>> +            else >>>>>>> +                error = gpiod_set_debounce(bdata->gpiod, >>>>>>> +                        button->debounce_interval * 1000); >>>>>>>               /* use timer if gpiolib doesn't provide debounce */ >>>>>>>               if (error < 0) >>>>>>>                   bdata->software_debounce = >>>>>>> >>>>>>> So keep debouncing, as that will always be necessary when dealing with >>>>>>> mechanical buttons, but always use software debouncing to avoid issues >>>>>>> like the issue you are seeing. >>>>>>> >>>>>>> My mention of the BYT/CHT behavior in my previous email was to point >>>>>>> out that those already always use software debouncing for the 50 ms >>>>>>> debounce-period. It was *not* my intention to suggest to solve this >>>>>>> with platform specific quirks/behavior. >>>>>>> >>>>>>> Regards, >>>>>>> >>>>>>> Hans >>>>>> >>>>>> I mentioned on the v1 too, but let's shift conversation here. >>>>> >>>>> Ack. >>>>> >>>>>> So essentially all platforms using soc_button_array would always turn on software debouncing of 50ms? >>>>> >>>>> Yes that is what my proposal entails. >>>>> >>>>>> In that case what happens if the hardware debounce was ALSO set from the ASL?  You end up with double debouncing I would expect. >>>>> >>>>> A hardware debounce of say 25 ms would still report the button down >>>>> immediately, it just won't report any state changes for 25 ms >>>>> after that, at least that is how I would expect this to work. >>>>> >>>>> So the 50 ms ignore-button-releases for the sw debounce will start >>>>> at the same time as the hw ignore-button-release window and basically >>>>> the longest window will win. So having both active should not really >>>>> cause any problems. >>>>> >>>>> Still only using one or the other as you propose below would >>>>> be better. >>>>> >>>>>> Shouldn't you only turn on software debouncing when it's required? >>>>>> >>>>>> I'm wondering if considering the first two patches we should have gpio-keys look up if hardware can support debounce, and then "only if it can't" we program the value from soc button array. >>>>>> >>>>>> It can be done by having gpio_keys do a "get()" on debounce.  Iff the driver returns -ENOTSUPP /then/ program the software debounce. >>>>> >>>>> Any special handling here should be done in soc_button_array since >>>>> this is specific to how with ACPI we have the GPIO resource >>>>> descriptors setting up the hw-debounce and then the need to do >>>>> software debounce when that was not setup. >>>>> >>>>> As for checking for -ENOTSUPP I would make soc_button_array >>>>> do something like this. >>>>> >>>>> ret = debounce_get() >>>>> if (ret <= 0) >>>>> use-sw-debounce; >>>>> >>>>> If hw-debounce is supported but not setup, either because >>>>> the exact debounce value being requested is not supported >>>>> or because the DSDT specified 0, then sw-debouncing should >>>>> also be used. >>>>> >>>>> Note this will still require the use of a new no_hw_debounce >>>>> flag so that we don't end up enabling hw-debounce in >>>>> the hw-debounce is supported but not setup case. >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>> >>>> I did some experiments with your proposal (letting SW debounce get >>>> programmed) and everything seems to work fine*. I think you're right that >>>> setting a double debounce would be worst one wins. >>> >>> I am confused, can you explain why do we need this new no_hw_debounce >>> flag? If AMD gpio driver is unable to program 50 ms debounce for a given >>> pin but does not return an error (or returns an error but leaves system >>> in a bad state) that is the issue in that driver and needs to be fixed >>> there? Why do we need to change soc_button_driver at all? >>> >>> Thanks. >>> >> >> The requested 50ms HW debounce gets programmed to the hardware register >> successfully. It is within bound that the GPIO controller can support. >> >> The problem is the power button does not function with a 50ms debounce. >> The firmware asserted that 0ms should have been programmed (by the _CRS >> value in GpioInt). > > I do not understand how debounce that is within the controller's > supported range can not work. The button is a switch that reports on and > off, there is nothing more to it, is there? > > I feel there is a deeper problem that we simply trying to paper over. > I never said the power button is a simple mechanical switch connected to the GPIO. A lot of designs I've seen have the EC connected to the power button GPIO and the EC does it's own debouncing of a physical switch to decide to assert GPIO 0. My point here is if the firmware is asserting that there shouldn't be a debounce programmed, programming one can lead to problems.