[PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Hans de Goede]

Hi Dmitry,

This patch is a bit of a kludge, but the problem it fixes has been
encountered on 2 different models now, so it seems that we really
need a workaround for this.

This patch applies on top of these 2 patches:

"Input: soc_button_array - Add active_low setting to soc_button_info"
"Input: soc_button_array - Add support for INT33D3 tablet-mode switch devices"

Which I have posted multiple times upstream already (they are from May!),
but these have not been getting any attention.

The soc_button_array code really is x86 specific glue code to translate
various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
As such I wonder if it would not be better to move this driver to
drivers/platform/x86?

I seem to be doing most if not all of the recent work on soc_button_array,
and soon I will be a co-maintainer of drivers/platform/x86. So having it
there and adding me in MAINTAINERS as maintaining it seems to be best?

If you want I can do a patch moving soc_button_array to drivers/platform/x86
and then add the other 3 patches on top and then we can merge all of this
through drivers/platform/x86?

Regards,

Hans

[PATCH] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Hans de Goede]

Some 2-in-1s which use the soc_button_array driver have this ugly issue in
their DSDT where the _LID method modifies the irq-type settings of the
GPIOs used for the power and home buttons. The intend of this AML code is
to disable these buttons when the lid is closed.

The AML does this by directly poking the GPIO controllers registers. This
is problematic because when re-enabling the irq, which happens whenever
_LID gets called with the lid open (e.g. on boot and on resume), it sets
the irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs
the type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.

This commit adds a workaround for this which (on affected devices) does
not set gpio_keys_button.gpio on these 2-in-1s, instead it gets the irq for
the GPIO, configures it as IRQ_TYPE_LEVEL_LOW (to match how the _LID AML
code configures it) and passes the irq in gpio_keys_button.irq.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/input/misc/soc_button_array.c | 69 +++++++++++++++++++++++----
 1 file changed, 60 insertions(+), 9 deletions(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 837c787e9c4b..cae1a3fae83a 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/acpi.h>
+#include <linux/dmi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio_keys.h>
 #include <linux/gpio.h>
@@ -42,23 +43,66 @@ struct soc_button_data {
 	struct platform_device *children[BUTTON_TYPES];
 };
 
+/*
+ * Some 2-in-1s which use the soc_button_array driver have this ugly issue in
+ * their DSDT where the _LID method modifies the irq-type settings of the GPIOs
+ * used for the power and home buttons. The intend of this AML code is to
+ * disable these buttons when the lid is closed.
+ * The AML does this by directly poking the GPIO controllers registers. This is
+ * problematic because when re-enabling the irq, which happens whenever _LID
+ * gets called with the lid open (e.g. on boot and on resume), it sets the
+ * irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs the
+ * type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.
+ * To work around this we don't set gpio_keys_button.gpio on these 2-in-1s,
+ * instead we get the irq for the GPIO ourselves, configure it as
+ * IRQ_TYPE_LEVEL_LOW (to match how the _LID AML code configures it) and pass
+ * the irq in gpio_keys_button.irq. Below is a list of affected devices.
+ */
+static const struct dmi_system_id dmi_use_low_level_irq[] = {
+	{
+		/*
+		 * Acer Switch 10 SW5-012. _LID method messes with home- and
+		 * power-button GPIO IRQ settings. When (re-)enabling the irq
+		 * it ors in its own flags without clearing the previous set
+		 * ones, leading to an irq-type of IRQ_TYPE_LEVEL_LOW |
+		 * IRQ_TYPE_LEVEL_HIGH causing a continuous interrupt storm.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Aspire SW5-012"),
+		},
+	},
+	{
+		/*
+		 * Acer One S1003. _LID method messes with power-button GPIO
+		 * IRQ settings, leading to a non working power-button.
+		 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "One S1003"),
+		},
+	},
+	{} /* Terminating entry */
+};
+
 /*
  * Get the Nth GPIO number from the ACPI object.
  */
-static int soc_button_lookup_gpio(struct device *dev, int acpi_index)
+static int soc_button_lookup_gpio(struct device *dev, int acpi_index,
+				  int *gpio_ret, int *irq_ret)
 {
 	struct gpio_desc *desc;
-	int gpio;
 
 	desc = gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
 	if (IS_ERR(desc))
 		return PTR_ERR(desc);
 
-	gpio = desc_to_gpio(desc);
+	*gpio_ret = desc_to_gpio(desc);
+	*irq_ret = gpiod_to_irq(desc);
 
 	gpiod_put(desc);
 
-	return gpio;
+	return 0;
 }
 
 static struct platform_device *
@@ -70,9 +114,8 @@ soc_button_device_create(struct platform_device *pdev,
 	struct platform_device *pd;
 	struct gpio_keys_button *gpio_keys;
 	struct gpio_keys_platform_data *gpio_keys_pdata;
+	int error, gpio, irq;
 	int n_buttons = 0;
-	int gpio;
-	int error;
 
 	for (info = button_info; info->name; info++)
 		if (info->autorepeat == autorepeat)
@@ -92,8 +135,8 @@ soc_button_device_create(struct platform_device *pdev,
 		if (info->autorepeat != autorepeat)
 			continue;
 
-		gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index);
-		if (!gpio_is_valid(gpio)) {
+		error = soc_button_lookup_gpio(&pdev->dev, info->acpi_index, &gpio, &irq);
+		if (error || irq < 0) {
 			/*
 			 * Skip GPIO if not present. Note we deliberately
 			 * ignore -EPROBE_DEFER errors here. On some devices
@@ -108,9 +151,17 @@ soc_button_device_create(struct platform_device *pdev,
 			continue;
 		}
 
+		/* See dmi_use_low_level_irq[] comment */
+		if (!autorepeat && dmi_check_system(dmi_use_low_level_irq)) {
+			irq_set_irq_type(irq, IRQ_TYPE_LEVEL_LOW);
+			gpio_keys[n_buttons].irq = irq;
+			gpio_keys[n_buttons].gpio = -ENOENT;
+		} else {
+			gpio_keys[n_buttons].gpio = gpio;
+		}
+
 		gpio_keys[n_buttons].type = info->event_type;
 		gpio_keys[n_buttons].code = info->event_code;
-		gpio_keys[n_buttons].gpio = gpio;
 		gpio_keys[n_buttons].active_low = info->active_low;
 		gpio_keys[n_buttons].desc = info->name;
 		gpio_keys[n_buttons].wakeup = info->wakeup;
-- 
2.28.0

Re: [PATCH] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Dmitry Torokhov]

On Sun, Sep 06, 2020 at 02:20:16PM +0200, Hans de Goede wrote:
> Some 2-in-1s which use the soc_button_array driver have this ugly issue in
> their DSDT where the _LID method modifies the irq-type settings of the
> GPIOs used for the power and home buttons. The intend of this AML code is
> to disable these buttons when the lid is closed.
> 
> The AML does this by directly poking the GPIO controllers registers. This
> is problematic because when re-enabling the irq, which happens whenever
> _LID gets called with the lid open (e.g. on boot and on resume), it sets
> the irq-type to IRQ_TYPE_LEVEL_LOW. Where as the gpio-keys driver programs
> the type to, and expects it to be, IRQ_TYPE_EDGE_BOTH.
> 
> This commit adds a workaround for this which (on affected devices) does
> not set gpio_keys_button.gpio on these 2-in-1s, instead it gets the irq for
> the GPIO, configures it as IRQ_TYPE_LEVEL_LOW (to match how the _LID AML
> code configures it) and passes the irq in gpio_keys_button.irq.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Dmitry Torokhov]

Hi Hans,

On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:
> Hi Dmitry,
> 
> This patch is a bit of a kludge, but the problem it fixes has been
> encountered on 2 different models now, so it seems that we really
> need a workaround for this.
> 
> This patch applies on top of these 2 patches:
> 
> "Input: soc_button_array - Add active_low setting to soc_button_info"
> "Input: soc_button_array - Add support for INT33D3 tablet-mode switch devices"
> 
> Which I have posted multiple times upstream already (they are from May!),
> but these have not been getting any attention.

Sorry about that, I merged them just now.
> 
> The soc_button_array code really is x86 specific glue code to translate
> various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
> As such I wonder if it would not be better to move this driver to
> drivers/platform/x86?
> 
> I seem to be doing most if not all of the recent work on soc_button_array,
> and soon I will be a co-maintainer of drivers/platform/x86. So having it
> there and adding me in MAINTAINERS as maintaining it seems to be best?
> 
> If you want I can do a patch moving soc_button_array to drivers/platform/x86
> and then add the other 3 patches on top and then we can merge all of this
> through drivers/platform/x86?

Sorry, misread this first time through, so already merged the 3 patches,
but I to not mind at all moving the driver to platform tree. If you send
me such a patch I will apply it.

Thanks.

-- 
Dmitry

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Hans de Goede]

Hi,

On 9/14/20 8:12 AM, Dmitry Torokhov wrote:
> Hi Hans,
> 
> On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:
>> Hi Dmitry,
>>
>> This patch is a bit of a kludge, but the problem it fixes has been
>> encountered on 2 different models now, so it seems that we really
>> need a workaround for this.
>>
>> This patch applies on top of these 2 patches:
>>
>> "Input: soc_button_array - Add active_low setting to soc_button_info"
>> "Input: soc_button_array - Add support for INT33D3 tablet-mode switch devices"
>>
>> Which I have posted multiple times upstream already (they are from May!),
>> but these have not been getting any attention.
> 
> Sorry about that, I merged them just now.

No problem, and thank you.

>> The soc_button_array code really is x86 specific glue code to translate
>> various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
>> As such I wonder if it would not be better to move this driver to
>> drivers/platform/x86?
>>
>> I seem to be doing most if not all of the recent work on soc_button_array,
>> and soon I will be a co-maintainer of drivers/platform/x86. So having it
>> there and adding me in MAINTAINERS as maintaining it seems to be best?
>>
>> If you want I can do a patch moving soc_button_array to drivers/platform/x86
>> and then add the other 3 patches on top and then we can merge all of this
>> through drivers/platform/x86?
> 
> Sorry, misread this first time through, so already merged the 3 patches,
> but I to not mind at all moving the driver to platform tree. If you send
> me such a patch I will apply it.

Ok.

Andy are you ok with moving the driver to the pdx86 tree too?

Regards,

Hans

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Andy Shevchenko]

On Mon, Sep 14, 2020 at 10:45 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/14/20 8:12 AM, Dmitry Torokhov wrote:
> > On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:

...

> >> The soc_button_array code really is x86 specific glue code to translate
> >> various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
> >> As such I wonder if it would not be better to move this driver to
> >> drivers/platform/x86?

AFAIU the above is a justification why PDx86 suits better to host it.

> >> I seem to be doing most if not all of the recent work on soc_button_array,
> >> and soon I will be a co-maintainer of drivers/platform/x86. So having it
> >> there and adding me in MAINTAINERS as maintaining it seems to be best?
> >>
> >> If you want I can do a patch moving soc_button_array to drivers/platform/x86
> >> and then add the other 3 patches on top and then we can merge all of this
> >> through drivers/platform/x86?
> >
> > Sorry, misread this first time through, so already merged the 3 patches,
> > but I to not mind at all moving the driver to platform tree. If you send
> > me such a patch I will apply it.
>
> Ok.
>
> Andy are you ok with moving the driver to the pdx86 tree too?

Taking into consideration the above, if I read it correctly, I agree.
Feel free to add my Ack.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Hans de Goede]

Hi,

On 9/14/20 10:00 AM, Andy Shevchenko wrote:
> On Mon, Sep 14, 2020 at 10:45 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 9/14/20 8:12 AM, Dmitry Torokhov wrote:
>>> On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:
> 
> ...
> 
>>>> The soc_button_array code really is x86 specific glue code to translate
>>>> various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
>>>> As such I wonder if it would not be better to move this driver to
>>>> drivers/platform/x86?
> 
> AFAIU the above is a justification why PDx86 suits better to host it.

Correct.

>>>> I seem to be doing most if not all of the recent work on soc_button_array,
>>>> and soon I will be a co-maintainer of drivers/platform/x86. So having it
>>>> there and adding me in MAINTAINERS as maintaining it seems to be best?
>>>>
>>>> If you want I can do a patch moving soc_button_array to drivers/platform/x86
>>>> and then add the other 3 patches on top and then we can merge all of this
>>>> through drivers/platform/x86?
>>>
>>> Sorry, misread this first time through, so already merged the 3 patches,
>>> but I to not mind at all moving the driver to platform tree. If you send
>>> me such a patch I will apply it.
>>
>> Ok.
>>
>> Andy are you ok with moving the driver to the pdx86 tree too?
> 
> Taking into consideration the above, if I read it correctly, I agree.
> Feel free to add my Ack.

Ok, since Dmitry's tree currently has some changes to soc_button_array.c,
the plan is to merge the patch through Dmitry's tree.

I will prepare a patch with your Acked-by and submit it.

Regards,

Hans

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Hans de Goede]

Hi,

On 9/14/20 3:52 PM, Hans de Goede wrote:
> Hi,
> 
> On 9/14/20 10:00 AM, Andy Shevchenko wrote:
>> On Mon, Sep 14, 2020 at 10:45 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 9/14/20 8:12 AM, Dmitry Torokhov wrote:
>>>> On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:
>>
>> ...
>>
>>>>> The soc_button_array code really is x86 specific glue code to translate
>>>>> various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
>>>>> As such I wonder if it would not be better to move this driver to
>>>>> drivers/platform/x86?
>>
>> AFAIU the above is a justification why PDx86 suits better to host it.
> 
> Correct.
> 
>>>>> I seem to be doing most if not all of the recent work on soc_button_array,
>>>>> and soon I will be a co-maintainer of drivers/platform/x86. So having it
>>>>> there and adding me in MAINTAINERS as maintaining it seems to be best?
>>>>>
>>>>> If you want I can do a patch moving soc_button_array to drivers/platform/x86
>>>>> and then add the other 3 patches on top and then we can merge all of this
>>>>> through drivers/platform/x86?
>>>>
>>>> Sorry, misread this first time through, so already merged the 3 patches,
>>>> but I to not mind at all moving the driver to platform tree. If you send
>>>> me such a patch I will apply it.
>>>
>>> Ok.
>>>
>>> Andy are you ok with moving the driver to the pdx86 tree too?
>>
>> Taking into consideration the above, if I read it correctly, I agree.
>> Feel free to add my Ack.
> 
> Ok, since Dmitry's tree currently has some changes to soc_button_array.c,
> the plan is to merge the patch through Dmitry's tree.
> 
> I will prepare a patch with your Acked-by and submit it.

So to make sure that there won't be any merge issues,
I was comparing bases for
{drivers/input/misc,drivers/platform/x86}/{Makefile,Kconfig}
looking at the versions in:
https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/for-next (which atm is just 5.9-rc1)

And the latter has a couple of commits to
drivers/platform/x86/Kconfig which the input tree is missing;
and these commits touch part of the file which moving the driver
over will also be touching.

Dmitry, it seems that your for next-tree is based on 5.7 + 2
large merges and as such does not have all the commits from
5.9-rc1 ?

Anyways this is not urgent, given the conflict I think it
might be best if I send out the patch after 5.10-rc1, using
5.10-rc1 as a base for it.

Regards,

Hans

Re: [PATCH 0/1] Input: soc_button_array - Work around DSDTs which modify the irqflags

[Dmitry Torokhov]

On Mon, Sep 14, 2020 at 04:08:09PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/14/20 3:52 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 9/14/20 10:00 AM, Andy Shevchenko wrote:
> > > On Mon, Sep 14, 2020 at 10:45 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > > > On 9/14/20 8:12 AM, Dmitry Torokhov wrote:
> > > > > On Sun, Sep 06, 2020 at 02:20:15PM +0200, Hans de Goede wrote:
> > > 
> > > ...
> > > 
> > > > > > The soc_button_array code really is x86 specific glue code to translate
> > > > > > various incarnations of gpio-keys in ACPI tables to gpio_keys_platform_data.
> > > > > > As such I wonder if it would not be better to move this driver to
> > > > > > drivers/platform/x86?
> > > 
> > > AFAIU the above is a justification why PDx86 suits better to host it.
> > 
> > Correct.
> > 
> > > > > > I seem to be doing most if not all of the recent work on soc_button_array,
> > > > > > and soon I will be a co-maintainer of drivers/platform/x86. So having it
> > > > > > there and adding me in MAINTAINERS as maintaining it seems to be best?
> > > > > > 
> > > > > > If you want I can do a patch moving soc_button_array to drivers/platform/x86
> > > > > > and then add the other 3 patches on top and then we can merge all of this
> > > > > > through drivers/platform/x86?
> > > > > 
> > > > > Sorry, misread this first time through, so already merged the 3 patches,
> > > > > but I to not mind at all moving the driver to platform tree. If you send
> > > > > me such a patch I will apply it.
> > > > 
> > > > Ok.
> > > > 
> > > > Andy are you ok with moving the driver to the pdx86 tree too?
> > > 
> > > Taking into consideration the above, if I read it correctly, I agree.
> > > Feel free to add my Ack.
> > 
> > Ok, since Dmitry's tree currently has some changes to soc_button_array.c,
> > the plan is to merge the patch through Dmitry's tree.
> > 
> > I will prepare a patch with your Acked-by and submit it.
> 
> So to make sure that there won't be any merge issues,
> I was comparing bases for
> {drivers/input/misc,drivers/platform/x86}/{Makefile,Kconfig}
> looking at the versions in:
> https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/?h=next
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/for-next (which atm is just 5.9-rc1)
> 
> And the latter has a couple of commits to
> drivers/platform/x86/Kconfig which the input tree is missing;
> and these commits touch part of the file which moving the driver
> over will also be touching.
> 
> Dmitry, it seems that your for next-tree is based on 5.7 + 2
> large merges and as such does not have all the commits from
> 5.9-rc1 ?

Yeah, I typically merge with mainline if I need new APIs or to sync up
with shared stuff.

Thanks.

-- 
Dmitry