[PATCH 0/3] Input: soc_button_array fixes and question

[PATCH 0/3] Input: soc_button_array fixes and question

[Benjamin Tissoires]

Hi,

This series has been triggered by the Surface 3 I have been given.
The way Microsoft follows its own specs is always intriguing. As
written in drivers/platform/x86/surfacepro3_button.c, the PNP0C40
ACPI device which should follow the specification doesn't have any
GPIO listed (thus the first 2 patches).

Also, the actual ACPI device that has the GPIO described is declared
as an I2C one, even if there is no such device attached to the bus.
This particular device could use the soc_button_array driver after a
little bit of ACPI magic (patches to follow, later), but each GPIO in
this device is declared twice (as Int and Io), so the 3rd patch.

Here is my question mentioned in $subject:

Why are we using gpiod_get_index(dev, KBUILD_MODNAME, acpi_index, GPIOD_ASIS);
in soc_button_lookup_gpio()?

>From what I can test here, it works the first time, but if we rmmod the module
and modprobe it again, it leads to a page fault.

My understanding is to use gpiod_get_index(dev, NULL, acpi_index, GPIOD_ASIS);
instead, which survives a module removal.

However, I do not have the ACPI spec for this and I do not have real hardware
following this spec, so I am just speculating with my Surface 3.

Cheers,
Benjamin

Benjamin Tissoires (3):
  Input - soc_button_array: use gpio_is_valid()
  Input - soc_button_array: bail out earlier if gpiod_count is null
  Input - soc_button_array: make sure one GPIO is not assigned twice

 drivers/input/misc/soc_button_array.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

-- 
2.5.0

[PATCH 1/3] Input - soc_button_array: use gpio_is_valid()

[Benjamin Tissoires]

gpio_keys will later use gpio_is_valid(). To match the actual
behavior, we should use it here too.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/misc/soc_button_array.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index c14b827..9cbe874 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -17,6 +17,7 @@
 #include <linux/acpi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio_keys.h>
+#include <linux/gpio.h>
 #include <linux/platform_device.h>
 
 /*
@@ -92,7 +93,7 @@ soc_button_device_create(struct platform_device *pdev,
 			continue;
 
 		gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index);
-		if (gpio < 0)
+		if (gpio_is_valid(gpio))
 			continue;
 
 		gpio_keys[n_buttons].type = info->event_type;
-- 
2.5.0

Re: [PATCH 1/3] Input - soc_button_array: use gpio_is_valid()

[Fabio Estevam]

On Tue, May 10, 2016 at 12:37 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:

> @@ -92,7 +93,7 @@ soc_button_device_create(struct platform_device *pdev,
>                         continue;
>
>                 gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index);
> -               if (gpio < 0)
> +               if (gpio_is_valid(gpio))

Shouldn't it be "if (!gpio_is_valid(gpio))" ?

Re: [PATCH 1/3] Input - soc_button_array: use gpio_is_valid()

[Benjamin Tissoires]

On May 10 2016 or thereabouts, Fabio Estevam wrote:
> On Tue, May 10, 2016 at 12:37 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> 
> > @@ -92,7 +93,7 @@ soc_button_device_create(struct platform_device *pdev,
> >                         continue;
> >
> >                 gpio = soc_button_lookup_gpio(&pdev->dev, info->acpi_index);
> > -               if (gpio < 0)
> > +               if (gpio_is_valid(gpio))
> 
> Shouldn't it be "if (!gpio_is_valid(gpio))" ?

Oops, indeed. I messed up between the various trees as it's not that
convenient to recompile a full kernel on an atom device. Will fix this
in v2.

Cheers,
Benjamin

[PATCH 2/3] Input - soc_button_array: bail out earlier if gpiod_count is null

[Benjamin Tissoires]

The PNP0C40 device of the Surface 3 doesn't have any GPIO attached to it.
Instead of trying to access the GPIO, request the count beforehand and
bail out if it is null or if an error is returned.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/misc/soc_button_array.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 9cbe874..70002a3 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -165,6 +165,11 @@ static int soc_button_probe(struct platform_device *pdev)
 	if (!id)
 		return -ENODEV;
 
+	if (gpiod_count(&pdev->dev, KBUILD_MODNAME) <= 0) {
+		dev_info(&pdev->dev, "no GPIO attached, ignoring...\n");
+		return -ENODEV;
+	}
+
 	button_info = (struct soc_button_info *)id->driver_data;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-- 
2.5.0

[PATCH 3/3] Input - soc_button_array: make sure one GPIO is not assigned twice

[Benjamin Tissoires]

The Surface 3 declares twice the GPIO as GpioInt and GpioIo in its
ACPI table. Given that we do not keep the gpiod around, but the actual
number associated to, there is a chance while enumerating the GPIOs that
one gets assigned twice. Make sure a previous button has not been mapped
already to the current button to prevent such failure.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/input/misc/soc_button_array.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c
index 70002a3..f2f75cf 100644
--- a/drivers/input/misc/soc_button_array.c
+++ b/drivers/input/misc/soc_button_array.c
@@ -77,7 +77,7 @@ soc_button_device_create(struct platform_device *pdev,
 	struct gpio_keys_platform_data *gpio_keys_pdata;
 	int n_buttons = 0;
 	int gpio;
-	int error;
+	int i, error;
 
 	gpio_keys_pdata = devm_kzalloc(&pdev->dev,
 				       sizeof(*gpio_keys_pdata) +
@@ -96,6 +96,13 @@ soc_button_device_create(struct platform_device *pdev,
 		if (gpio_is_valid(gpio))
 			continue;
 
+		for (i = 0; i < n_buttons; i++) {
+			if (gpio_keys[i].gpio == gpio)
+				break;
+		}
+		if (i < n_buttons)
+			continue; /* the GPIO has already been assigned */
+
 		gpio_keys[n_buttons].type = info->event_type;
 		gpio_keys[n_buttons].code = info->event_code;
 		gpio_keys[n_buttons].gpio = gpio;
-- 
2.5.0

Re: [PATCH 0/3] Input: soc_button_array fixes and question

[Benjamin Tissoires]

On Tue, May 10, 2016 at 5:37 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Hi,
>
> This series has been triggered by the Surface 3 I have been given.
> The way Microsoft follows its own specs is always intriguing. As
> written in drivers/platform/x86/surfacepro3_button.c, the PNP0C40
> ACPI device which should follow the specification doesn't have any
> GPIO listed (thus the first 2 patches).
>
> Also, the actual ACPI device that has the GPIO described is declared
> as an I2C one, even if there is no such device attached to the bus.
> This particular device could use the soc_button_array driver after a
> little bit of ACPI magic (patches to follow, later), but each GPIO in
> this device is declared twice (as Int and Io), so the 3rd patch.
>
> Here is my question mentioned in $subject:
>
> Why are we using gpiod_get_index(dev, KBUILD_MODNAME, acpi_index, GPIOD_ASIS);
> in soc_button_lookup_gpio()?
>

I'll leave the question open and send a v2 with an actual support of
the Surface 3 buttons soon.
I'll remove most of the CC-ed people as it might be considered as spam
for most of you.

Cheers,
Benjamin