Hidden dependency of i2c-hid on GPIOLIB

Hidden dependency of i2c-hid on GPIOLIB

[Gabriele Mazzotta]

Hi,

my touchpad stopped working because of a485923efbb8 ("HID: i2c-hid:
Add support for ACPI GPIO interrupts"). It turned out that I need
CONFIG_GPIOLIB. I think this dependency should be made explicit or the
driver should not depend on it.

Regards,
Gabriele

Re: Hidden dependency of i2c-hid on GPIOLIB

[Mika Westerberg]

On Tue, Apr 14, 2015 at 12:55:20AM +0200, Gabriele Mazzotta wrote:
> Hi,
> 
> my touchpad stopped working because of a485923efbb8 ("HID: i2c-hid:
> Add support for ACPI GPIO interrupts"). It turned out that I need
> CONFIG_GPIOLIB. I think this dependency should be made explicit or the
> driver should not depend on it.

Using GPIOs should be optional. Gpiolib stubs functions out whenever it
is not enabled.

However, I made a mistake when GPIOs are fed to the driver. If gpiolib
is not enabled acpi_dev_add_driver_gpios() returns -ENXIO and prevents
your touchpad from working.

I think it is alright to ignore the error here. Can you try if below
patch helps?

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ab4dd952b6ba..b427c11d4cd3 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -877,7 +877,8 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	pdata->hid_descriptor_address = obj->integer.value;
 	ACPI_FREE(obj);
 
-	return acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
+	acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
+	return 0;
 }
 
 static const struct acpi_device_id i2c_hid_acpi_match[] = {

Re: Hidden dependency of i2c-hid on GPIOLIB

[Gabriele Mazzotta]

On Tuesday 14 April 2015 12:59:24 Mika Westerberg wrote:
> On Tue, Apr 14, 2015 at 12:55:20AM +0200, Gabriele Mazzotta wrote:
> > Hi,
> > 
> > my touchpad stopped working because of a485923efbb8 ("HID: i2c-hid:
> > Add support for ACPI GPIO interrupts"). It turned out that I need
> > CONFIG_GPIOLIB. I think this dependency should be made explicit or the
> > driver should not depend on it.
> 
> Using GPIOs should be optional. Gpiolib stubs functions out whenever it
> is not enabled.
> 
> However, I made a mistake when GPIOs are fed to the driver. If gpiolib
> is not enabled acpi_dev_add_driver_gpios() returns -ENXIO and prevents
> your touchpad from working.
> 
> I think it is alright to ignore the error here. Can you try if below
> patch helps?

Sorry for not being more specific. Yes, the problem is that
acpi_dev_add_driver_gpios() returns -ENXIO if CONFIG_GPIOLIB is not set.

> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index ab4dd952b6ba..b427c11d4cd3 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -877,7 +877,8 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
>  	pdata->hid_descriptor_address = obj->integer.value;
>  	ACPI_FREE(obj);
>  
> -	return acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
> +	acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
> +	return 0;
>  }
>  
>  static const struct acpi_device_id i2c_hid_acpi_match[] = {

[PATCH] HID: i2c-hid: Do not fail probing if gpiolib is not enabled

[Mika Westerberg]

Using GPIOs and gpiolib is optional. If the kernel is compiled without GPIO
support the driver should not fail if it finds the interrupt using normal
methods.

However, commit a485923efbb8 ("HID: i2c-hid: Add support for ACPI GPIO
interrupts") did not take into account that acpi_dev_add_driver_gpios()
returns -ENXIO when !CONFIG_GPIOLIB.

Fix this by checking the return value against -ENXIO and 0 and only in that
case fail the probe.

Reported-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ab4dd952b6ba..92d6cdf02460 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -862,6 +862,7 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	union acpi_object *obj;
 	struct acpi_device *adev;
 	acpi_handle handle;
+	int ret;
 
 	handle = ACPI_HANDLE(&client->dev);
 	if (!handle || acpi_bus_get_device(handle, &adev))
@@ -877,7 +878,9 @@ static int i2c_hid_acpi_pdata(struct i2c_client *client,
 	pdata->hid_descriptor_address = obj->integer.value;
 	ACPI_FREE(obj);
 
-	return acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
+	/* GPIOs are optional */
+	ret = acpi_dev_add_driver_gpios(adev, i2c_hid_acpi_gpios);
+	return ret < 0 && ret != -ENXIO ? ret : 0;
 }
 
 static const struct acpi_device_id i2c_hid_acpi_match[] = {
-- 
2.1.4

Re: [PATCH] HID: i2c-hid: Do not fail probing if gpiolib is not enabled

[Jiri Kosina]

On Tue, 14 Apr 2015, Mika Westerberg wrote:

> Using GPIOs and gpiolib is optional. If the kernel is compiled without GPIO
> support the driver should not fail if it finds the interrupt using normal
> methods.
> 
> However, commit a485923efbb8 ("HID: i2c-hid: Add support for ACPI GPIO
> interrupts") did not take into account that acpi_dev_add_driver_gpios()
> returns -ENXIO when !CONFIG_GPIOLIB.
> 
> Fix this by checking the return value against -ENXIO and 0 and only in that
> case fail the probe.
> 
> Reported-by: Gabriele Mazzotta <gabriele.mzt@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I have applied this to for-4.1/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs