IIO: Adds ACPI support for ST gyroscopes

It can be used exactly like L3GD20 but it has a different WhoAmI register value.

Signed-off-by Robert Dolca <[email protected]>

Quoted 3 lines · Robert Dolca

It can be used exactly like L3GD20 but it has a different WhoAmI register value.

Quoted 1 line

Signed-off-by Robert Dolca <[email protected]>

Reviewed-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Signed-off-by Robert Dolca <[email protected]>

---
 drivers/iio/common/st_sensors/st_sensors_i2c.c | 35 ++++++++++++++++++++++++++
 drivers/iio/gyro/st_gyro_i2c.c                 | 29 ++++++++++++++++++++-
 include/linux/iio/common/st_sensors_i2c.h      |  3 +++
 3 files changed, 66 insertions(+), 1 deletion(-)

drivers/iio/common/st_sensors/st_sensors_i2c.c +5 -0

index 98cfee29..2f612ec 100644
@@ -13,6 +13,8 @@
 #include <linux/slab.h>
 #include <linux/iio/iio.h>
 #include <linux/of_device.h>
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
 
 #include <linux/iio/common/st_sensors_i2c.h>
 

@@ -107,6 +109,39 @@ void st_sensors_of_i2c_probe(struct i2c_client *client,
 EXPORT_SYMBOL(st_sensors_of_i2c_probe);
 #endif
 
+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+			       const struct acpi_device_id *match)
+{

drivers/iio/common/st_sensors/st_sensors_i2c.c +5 -0

+
+	return 0;
+}
+EXPORT_SYMBOL(st_sensors_acpi_i2c_probe);
+
 MODULE_AUTHOR("Denis Ciocca <[email protected]>");
 MODULE_DESCRIPTION("STMicroelectronics ST-sensors i2c driver");
 MODULE_LICENSE("GPL v2");

drivers/iio/gyro/st_gyro_i2c.c +28 -1

index 64480b1..712d23f 100644
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
 

@@ -18,6 +19,25 @@
 #include <linux/iio/common/st_sensors_i2c.h>
 #include "st_gyro.h"
 
+static const char L3G4200D_gyro_dev_name[]  = "l3g4200d";
+static const char LSM330D_gyro_dev_name[]   = "lsm330d_gyro";
+static const char LSM330DL_gyro_dev_name[]  = "lsm330dl_gyro";
+static const char LSM330DLC_gyro_dev_name[] = "lsm330dlc_gyro";
+static const char L3GD20_gyro_dev_name[]    = "l3gd20";
+static const char L3G4IS_gyro_dev_name[]    = "l3g4is_ui";
+static const char LSM330_gyro_dev_name[]    = "lsm330_gyro";
+
+static const struct acpi_device_id st_gyro_acpi_match[] = {
+	{"L3G4200D", (kernel_ulong_t) L3G4200D_gyro_dev_name},
+	{"LSM330D",  (kernel_ulong_t) LSM330D_gyro_dev_name},
+	{"LSM330D2", (kernel_ulong_t) LSM330DL_gyro_dev_name},
+	{"LSM330D3", (kernel_ulong_t) LSM330DLC_gyro_dev_name},
+	{"L3GD2000", (kernel_ulong_t) L3GD20_gyro_dev_name},
+	{"L3G40000", (kernel_ulong_t) L3G4IS_gyro_dev_name},
+	{"LSM3300",  (kernel_ulong_t) LSM330_gyro_dev_name},
+	{}
+};
+
 #ifdef CONFIG_OF
 static const struct of_device_id st_gyro_of_match[] = {
 	{

@@ -67,7 +87,13 @@ static int st_gyro_i2c_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	gdata = iio_priv(indio_dev);
-	st_sensors_of_i2c_probe(client, st_gyro_of_match);
+
+	if (ACPI_HANDLE(&client->dev)) {
+		err = st_sensors_acpi_i2c_probe(client, st_gyro_acpi_match);
+		if (err < 0)
+			return err;
+	} else
+		st_sensors_of_i2c_probe(client, st_gyro_of_match);
 
 	st_sensors_i2c_configure(indio_dev, client, gdata);
 

@@ -102,6 +128,7 @@ static struct i2c_driver st_gyro_driver = {
 		.owner = THIS_MODULE,
 		.name = "st-gyro-i2c",
 		.of_match_table = of_match_ptr(st_gyro_of_match),
+		.acpi_match_table = ACPI_PTR(st_gyro_acpi_match),
 	},
 	.probe = st_gyro_i2c_probe,
 	.remove = st_gyro_i2c_remove,

include/linux/iio/common/st_sensors_i2c.h +3 -0

index 1796af0..2e90b8f 100644
@@ -28,4 +28,7 @@ static inline void st_sensors_of_i2c_probe(struct i2c_client *client,
 }
 #endif
 
+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+			       const struct acpi_device_id *match);
+
 #endif /* ST_SENSORS_I2C_H */

Signature · 7 lines

1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 3 lines · Robert Dolca

Signed-off-by Robert Dolca <[email protected]>

---
drivers/iio/common/st_sensors/st_sensors_i2c.c | 35 +++++++++++++++++++=

Quoted 1 line

drivers/iio/gyro/st_gyro_i2c.c | 29 +++++++++++++++++++=

drivers/ii=

Quoted 2 lines

drivers/ii= renamed from drivers/iio/common/st_sensors/st_sensors_i2c.c +0 -0

Quoted 10 lines

index 98cfee29..2f612ec 100644

drivers/iio/common/st_sensors/st_sensors_i2c.c +2 -0

@@ -13,6 +13,8 @@
 #include <linux/slab.h>
 #include <linux/iio/iio.h>
 #include <linux/of_device.h>
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
 

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *cli=

Quoted 3 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +0 -0

 #include <linux/iio/common/st_sensors_i2c.h>
 

@@ -107,6 +109,39 @@ void st_sensors_of_i2c_probe(struct i2c_client *cli=

Quoted 6 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +3 -0

 EXPORT_SYMBOL(st_sensors_of_i2c_probe);
 #endif
 
+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+                            const struct acpi_device_id *match)
+{

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *client,

Quoted 10 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+     const struct acpi_device_id *id;
+     struct gpio_desc *gpiod_irq;
+     int ret;
+
+     id =3D acpi_match_device(match, &client->dev);
+     if (!id)
+             return -ENODEV;
+
+     /* Get IRQ GPIO */
+     gpiod_irq =3D devm_gpiod_get_index(&client->dev, 0, 0);

Please use plain devm_gpiod_get(&client->dev, NULL). That should work with DT and ACPI _DSD as well.

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *client, · line 126

Quoted 2 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +2 -0

+     if (IS_ERR(gpiod_irq))
+             return -ENODEV;

Why not return the original error here? Now you lose things like -EPROBE_DEFER.

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *client,

Quoted 12 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +12 -0

+     /* Configure IRQ GPIO */
+     ret =3D gpiod_direction_input(gpiod_irq);
+     if (ret)
+             return ret;
+
+     /* Map the pin to an IRQ */
+     client->irq =3D gpiod_to_irq(gpiod_irq);
+
+     /* The name from the ACPI match takes precedence if present */
+     memset(client->name, 0, sizeof(client->name));
+     strncpy(client->name, (char *) id->driver_data,
+             min(sizeof(client->name), strlen((char *) id->driver_data)=

Hmm, the above should not be required at all. If the device has an ACPI companion the I2C core will match first with that.

Also you now modify the original i2c_client structure. Should you at least restore the original values when the driver is unbound?

Signed-off-by Robert Dolca <[email protected]>

Signed-off-by Robert Dolca <[email protected]>

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+			       const struct acpi_device_id *match)
+{
+	const struct acpi_device_id *id;
+	struct gpio_desc *gpiod_irq;
+	int ret;
+
+	id = acpi_match_device(match, &client->dev);
+	if (!id)
+		return -ENODEV;

[...]

Quoted 11 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+                              const struct acpi_device_id *match)
+{
+       const struct acpi_device_id *id;
+       struct gpio_desc *gpiod_irq;
+       int ret;
+
+       id = acpi_match_device(match, &client->dev);
+       if (!id)
+               return -ENODEV;

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *client, · line 131

Quoted 10 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+
+       /* Get IRQ GPIO */
+       gpiod_irq = devm_gpiod_get_index(&client->dev, 0, 0);
+       if (IS_ERR(gpiod_irq))
+               return -ENODEV;
+
+       /* Configure IRQ GPIO */
+       ret = gpiod_direction_input(gpiod_irq);
+       if (ret)
+               return ret;

How exactly does this whole GPIO IRQ thing work with ACPI. Does the ACPI description just specify the GPIOs and the driver needs to know which GPIO is the is used for the IRQ or does the description indicate that a certain GPIO should be used as a IRQ. The reason why I'm asking is that same code pops up in pretty much every ACPI I2C sensor driver now. Which suggests that this should be factored out into common infrastructure.

And especially the requesting and the setting of the direction of the GPIO should not be necessary if the GPIO controller implements interrupt handling correctly as this is something that is, as far as I understand, taken care of by the GPIO framework when the IRQ is requested.

Quoted 10 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +8 -0

+
+       /* Map the pin to an IRQ */
+       client->irq = gpiod_to_irq(gpiod_irq);
+
+       /* The name from the ACPI match takes precedence if present */
+       memset(client->name, 0, sizeof(client->name));
+       strncpy(client->name, (char *) id->driver_data,
+               min(sizeof(client->name), strlen((char *)

id->driver_data)));

Both client->irq and client->name should not be modified by the driver, these are only supposed to be set by the I2C framework. Modifying them in the driver can result in undefined behavior.

Quoted 31 lines · Lars-Peter Clausen

[...]

Quoted 11 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+int st_sensors_acpi_i2c_probe(struct i2c_client *client,
+                              const struct acpi_device_id *match)
+{
+       const struct acpi_device_id *id;
+       struct gpio_desc *gpiod_irq;
+       int ret;
+
+       id = acpi_match_device(match, &client->dev);
+       if (!id)
+               return -ENODEV;

drivers/iio/common/st_sensors/st_sensors_i2c.c void st_sensors_of_i2c_probe(struct i2c_client *client, · line 131

Quoted 10 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +10 -0

+
+       /* Get IRQ GPIO */
+       gpiod_irq = devm_gpiod_get_index(&client->dev, 0, 0);
+       if (IS_ERR(gpiod_irq))
+               return -ENODEV;
+
+       /* Configure IRQ GPIO */
+       ret = gpiod_direction_input(gpiod_irq);
+       if (ret)
+               return ret;

How exactly does this whole GPIO IRQ thing work with ACPI. Does the ACPI description just specify the GPIOs and the driver needs to know which GPIO is the is used for the IRQ or does the description indicate that a certain GPIO should be used as a IRQ. The reason why I'm asking is that same code pops up in pretty much every ACPI I2C sensor driver now. Which suggests that this should be factored out into common infrastructure.

Quoted 4 lines

And especially the requesting and the setting of the direction of the GPIO should not be necessary if the GPIO controller implements interrupt handling correctly as this is something that is, as far as I understand, taken care of by the GPIO framework when the IRQ is requested.

Hi Lars,

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

As far as I know you can not specify the direction in the ACPI description and I am not sure if we can rely on the fact that the GPIO pins are in input mode by default.

Quoted 16 lines

Quoted 11 lines

drivers/iio/common/st_sensors/st_sensors_i2c.c +8 -0

+
+       /* Map the pin to an IRQ */
+       client->irq = gpiod_to_irq(gpiod_irq);
+
+       /* The name from the ACPI match takes precedence if present */
+       memset(client->name, 0, sizeof(client->name));
+       strncpy(client->name, (char *) id->driver_data,
+               min(sizeof(client->name), strlen((char *)

id->driver_data)));

Both client->irq and client->name should not be modified by the driver, these are only supposed to be set by the I2C framework. Modifying them in the driver can result in undefined behavior.

I understand that modifying the name is not a good approach. Doing it for device tree and not for ACPI can also result in a unknown behavior. Seems like the best approach here is to remove that name overwriting from both device tree and acpi probe after becomes clear why was it there in the 1st place.

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

Quoted 9 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 4 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

 if (!client->irq && dev->of_node) {
                int irq = of_irq_get(dev->of_node, 0);

                if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
        }

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

Quoted 41 lines · Linus Walleij

Quoted 9 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars-Qo5EllUWu/[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 4 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

  if (!client->irq && dev->of_node) {
                 int irq = of_irq_get(dev->of_node, 0);

                 if (irq == -EPROBE_DEFER)
                         return irq;
                 if (irq < 0)
                         irq = 0;

                 client->irq = irq;
         }

Quoted 5 lines

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

In ACPI we have two kind of GPIOs: GpioIo and GpioInt. The latter is used to describe GPIOs that can be used as interrupts.

In order to translate a GpioInt to an interrupt number we would need to request the GPIO first here (in the I2C core), call gpiod_to_irq() to it and assign that to the client->irq.

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.
3) We may have multiple GpioInts for devices like GPIO button array. Which one we should pick, or should we let the driver to handle this separetely?

I recently did similar change to drivers/hid/i2c-hid/i2c-hid.c and would be happy if we can get this factored to some generic code.

Add Alexandre and linux-gpio to Cc.

Quoted 58 lines · Mika Westerberg

Quoted 49 lines · Linus Walleij

Quoted 13 lines · Lars-Peter Clausen

Quoted 8 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 5 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it
is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

  if (!client->irq && dev->of_node) {
                 int irq = of_irq_get(dev->of_node, 0);

                 if (irq == -EPROBE_DEFER)
                         return irq;
                 if (irq < 0)
                         irq = 0;

                 client->irq = irq;
         }

Quoted 5 lines

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

Quoted 6 lines

In ACPI we have two kind of GPIOs: GpioIo and GpioInt. The latter is used to describe GPIOs that can be used as interrupts.

In order to translate a GpioInt to an interrupt number we would need to request the GPIO first here (in the I2C core), call gpiod_to_irq() to it and assign that to the client->irq.

Maybe the API can be extended to support to translate a GPIO to a IRQ without actually requesting the GPIO first.

Quoted 10 lines

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.
3) We may have multiple GpioInts for devices like GPIO button array. Which one we should pick, or should we let the driver to handle this separetely?

Well, we have the same issue with devicetree already. I'd say use the first IRQ for client->irq and ignore the other ones for now.

Add Alexandre and linux-gpio to Cc.

Quoted 51 lines · Mika Westerberg

Quoted 41 lines · Linus Walleij

Quoted 9 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 4 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

 if (!client->irq && dev->of_node) {
                int irq = of_irq_get(dev->of_node, 0);

                if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
        }

Quoted 5 lines

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

In ACPI we have two kind of GPIOs: GpioIo and GpioInt. The latter is used to describe GPIOs that can be used as interrupts.

Quoted 3 lines

In order to translate a GpioInt to an interrupt number we would need to request the GPIO first here (in the I2C core), call gpiod_to_irq() to it and assign that to the client->irq.

Maybe the API can be extended to support to translate a GPIO to a IRQ without actually requesting the GPIO first.

Quoted 10 lines

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.
3) We may have multiple GpioInts for devices like GPIO button array. Which one we should pick, or should we let the driver to handle this separetely?

Well, we have the same issue with devicetree already. I'd say use the first IRQ for client->irq and ignore the other ones for now.

Quoted 54 lines · Lars-Peter Clausen

Add Alexandre and linux-gpio to Cc.

Quoted 51 lines · Mika Westerberg

Quoted 41 lines · Linus Walleij

Quoted 9 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 4 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

  if (!client->irq && dev->of_node) {
                 int irq = of_irq_get(dev->of_node, 0);

                 if (irq == -EPROBE_DEFER)
                         return irq;
                 if (irq < 0)
                         irq = 0;

                 client->irq = irq;
         }

Quoted 5 lines

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

In ACPI we have two kind of GPIOs: GpioIo and GpioInt. The latter is used to describe GPIOs that can be used as interrupts.

Quoted 6 lines

Quoted 3 lines

In order to translate a GpioInt to an interrupt number we would need to request the GPIO first here (in the I2C core), call gpiod_to_irq() to it and assign that to the client->irq.

Maybe the API can be extended to support to translate a GPIO to a IRQ without actually requesting the GPIO first.

We still need to take care the the GPIO is properly requested and locked as IRQ. Otherwise something else (userspace for example) can mess this up.

Quoted 11 lines

Quoted 10 lines

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.
3) We may have multiple GpioInts for devices like GPIO button array. Which one we should pick, or should we let the driver to handle this separetely?

Quoted 2 lines

Well, we have the same issue with devicetree already. I'd say use the first IRQ for client->irq and ignore the other ones for now.

For devices like the button array above doing that leaves the driver wondering where the heck is one of my GPIOs :-) Perhaps we could to automatic translation if we find out that there is only one GpioInt for this device.

Quoted 80 lines · Mika Westerberg

Quoted 54 lines · Lars-Peter Clausen

Add Alexandre and linux-gpio to Cc.

Quoted 51 lines · Mika Westerberg

Quoted 41 lines · Linus Walleij

Quoted 9 lines · Lars-Peter Clausen

Quoted 6 lines · Robert Dolca

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars-Qo5EllUWu/[email protected]> wrote:

In the ACPI description you specify one or more IRQ GPIO pins. In the driver you request the GPIO pin using the index. In the ACPI 5.1 specification you can use named GPIOs instead of index.

Quoted 4 lines

But is there a way to distinguish between IRQ GPIOs and non IRQ GPIOs? If it is clear that a certain GPIO is the IRQ for the device the I2C framework should take care of assigning the client->irq field, instead of doing it manually in each and every device driver.

In the device tree case we have a mechanism where each GPIO chip implements two API:s, one gpio_chip API and one irqchip API.

Then in the tree both the GPIO and IRQs can be assigned as resources to clients, orthogonally. Usually this will only work if there is a 1-to-1 correspondence between the GPIO lines and available IRQ line triggers on the GPIO chip, but that is indeed the most common. They will then usually also have the same line offset numbers. In some odd cases I guess it won't work this way.

The I2C subsystem does this for the device tree case in i2c_device_probe() like this:

 if (!client->irq && dev->of_node) {
                int irq = of_irq_get(dev->of_node, 0);

                if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
        }

Quoted 5 lines

This is why the code does not contain any OF/DT
IRQ assignment code.

However in the ACPI probe path I guess it doesn't
happen then?

In ACPI we have two kind of GPIOs: GpioIo and GpioInt. The latter is used to describe GPIOs that can be used as interrupts.

Quoted 6 lines

Quoted 3 lines

In order to translate a GpioInt to an interrupt number we would need to request the GPIO first here (in the I2C core), call gpiod_to_irq() to it and assign that to the client->irq.

Maybe the API can be extended to support to translate a GPIO to a IRQ without actually requesting the GPIO first.

We still need to take care the the GPIO is properly requested and locked as IRQ. Otherwise something else (userspace for example) can mess this up.

Quoted 13 lines

Quoted 10 lines

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.
3) We may have multiple GpioInts for devices like GPIO button array. Which one we should pick, or should we let the driver to handle this separetely?

Well, we have the same issue with devicetree already. I'd say use the first IRQ for client->irq and ignore the other ones for now.

Quoted 4 lines

For devices like the button array above doing that leaves the driver wondering where the heck is one of my GPIOs :-) Perhaps we could to automatic translation if we find out that there is only one GpioInt for this device.

Btw. in the ACPI case client->irq is already initialized by acpi_dev_resource_interrupt() in the I2C core. Should the GpioInts just map onto this API as well?

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.

Quoted 6 lines · Mika Westerberg

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.

In the DT usecase we actually specify that in the DTS file so we don't have the problem. Either the consumer accesses the irqchip API with:

interrupts = <nn nn>;

or it accesses the GPIO API with:

gpios = <nn nn>;

so in that sense it is clear what is requested. Then the core of course uses gpiochip_lock/unlock_as_irq() to handle the case where bugs make a collision (like if both were specified and both APIs tries to access the same resource).

But as long as the DTS file is consistent there is no problem.

So it seems the ACPI tables are lacking this semantic information?

Quoted 14 lines · Linus Walleij

Quoted 6 lines · Mika Westerberg

This has few problems that I have not yet figured out. Maybe someone here can suggest what to do:

1) Who is responsible in releasing the GPIO?
2) What if the driver wants to use that pin as a GPIO instead? The GPIO is already requested by the I2C core.

In the DT usecase we actually specify that in the DTS file so we don't have the problem. Either the consumer accesses the irqchip API with:

Quoted 5 lines

interrupts = <nn nn>;

or it accesses the GPIO API with:

gpios = <nn nn>;

OK, I see.

Quoted 4 lines

so in that sense it is clear what is requested. Then the core of course uses gpiochip_lock/unlock_as_irq() to handle the case where bugs make a collision (like if both were specified and both APIs tries to access the same resource).

Where in the core code gpiochip_lock/unlock_as_irq() is called for these? At least of_irq_get() doesn't seem to be doing that. Maybe I'm looking at the wrong place.

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

	if (client->irq <= 0) {
		int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

		if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
	}

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Quoted 20 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
      }

Quoted 3 lines

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

Robert, can you try it out?

If people think this is the right way to go forward, I'll make formal patches and submit them for review.

drivers/gpio/gpiolib-acpi.c +30 -0

index c0929d938ced..ed7c40c7b4ab 100644
@@ -510,6 +510,36 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
        return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
 }
 
+/**
+ * acpi_dev_gpio_irq_get() - Find GpioInt and translate it to Linux IRQ number
+ * @adev: pointer to a ACPI device to get IRQ from
+ * @index: index of GpioInt resource (starting from %0)
+ *
+ * If the device has one or more GpioInt resources, this function can be
+ * used to translate from the GPIO offset in the resource to the Linux IRQ
+ * number.
+ *
+ * Return: Linux IRQ number (>%0) on success, negative errno on failure.
+ */
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
+{
+       struct gpio_desc *desc;
+       int i = 0;
+
+       do {
+               struct acpi_gpio_info info;
+
+               desc = acpi_get_gpiod_by_index(adev, NULL, i, &info);
+               if (!IS_ERR(desc)) {
+                       if (info.gpioint && i++ == index)
+                               return gpiod_to_irq(desc);
+               }
+       } while (!IS_ERR(desc));
+
+       return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_get);
+
 static acpi_status
 acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
                            u32 bits, u64 *value, void *handler_context,

drivers/i2c/i2c-core.c +7 -2

index edf274cabe81..c12a3f24ada4 100644
@@ -629,8 +629,13 @@ static int i2c_device_probe(struct device *dev)
        if (!client)
                return 0;
 
-       if (!client->irq && dev->of_node) {
-               int irq = of_irq_get(dev->of_node, 0);
+       if (client->irq <= 0) {
+               int irq = -ENOENT;
+
+               if (dev->of_node)
+                       irq = of_irq_get(dev->of_node, 0);
+               else if (ACPI_COMPANION(dev))
+                       irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
 
                if (irq == -EPROBE_DEFER)
                        return irq;

include/linux/acpi.h +2 -0

index 24c7aa8b1d20..826e214ab7d5 100644
@@ -704,6 +704,8 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
        if (adev)
                adev->driver_gpios = NULL;
 }
+
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
 #else
 static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
                              const struct acpi_gpio_mapping *gpios)

@@ -711,6 +713,11 @@ static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
        return -ENXIO;

Quoted 20 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

	if (client->irq <= 0) {
		int irq = -ENOENT;

                 if (dev->of_node)
                         irq = of_irq_get(dev->of_node, 0);
                 else if (ACPI_COMPANION(dev))
                         irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

		if (irq == -EPROBE_DEFER)
                         return irq;
                 if (irq < 0)
                         irq = 0;

                 client->irq = irq;
	}

Quoted 3 lines

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

Robert, can you try it out?

If people think this is the right way to go forward, I'll make formal patches and submit them for review.

drivers/gpio/gpiolib-acpi.c +30 -0

index c0929d938ced..ed7c40c7b4ab 100644
@@ -510,6 +510,36 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
  	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
  }
 
+/**
+ * acpi_dev_gpio_irq_get() - Find GpioInt and translate it to Linux IRQ number
+ * @adev: pointer to a ACPI device to get IRQ from
+ * @index: index of GpioInt resource (starting from %0)
+ *
+ * If the device has one or more GpioInt resources, this function can be
+ * used to translate from the GPIO offset in the resource to the Linux IRQ
+ * number.
+ *
+ * Return: Linux IRQ number (>%0) on success, negative errno on failure.
+ */
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
+{
+	struct gpio_desc *desc;
+	int i = 0;
+
+	do {
+		struct acpi_gpio_info info;
+
+		desc = acpi_get_gpiod_by_index(adev, NULL, i, &info);
+		if (!IS_ERR(desc)) {
+			if (info.gpioint && i++ == index)
+				return gpiod_to_irq(desc);
+		}
+	} while (!IS_ERR(desc));
+
+	return -ENOENT;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_get);
+
  static acpi_status
  acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
  			    u32 bits, u64 *value, void *handler_context,

drivers/i2c/i2c-core.c +0 -0

index edf274cabe81..c12a3f24ada4 100644

drivers/i2c/i2c-core.c +6 -0

+		int irq = -ENOENT;
+
+		if (dev->of_node)
+			irq = of_irq_get(dev->of_node, 0);
+		else if (ACPI_COMPANION(dev))
+			irq = acpi_dev_gpio_irq_get(ACPI_COMPANION(dev), 0);
 
  		if (irq == -EPROBE_DEFER)
  			return irq;

include/linux/acpi.h +7 -1

index 24c7aa8b1d20..826e214ab7d5 100644
@@ -704,6 +704,8 @@ static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev)
  	if (adev)
  		adev->driver_gpios = NULL;
  }
+
+int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index);
  #else
  static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
  			      const struct acpi_gpio_mapping *gpios)

@@ -711,6 +713,11 @@ static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
  	return -ENXIO;
  }
  static inline void acpi_dev_remove_driver_gpios(struct acpi_device *adev) {}
+
+static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
+{
+	return -ENXIO;
+}
  #endif
 
  /* Device properties */
--

To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines

static acpi_status
acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
u32 bits, u64 *value, void *handler_context,

drivers/i2c/i2c-core.c +0 -1

index edf274cabe81..c12a3f24ada4 100644
--- a/drivers/i2c/i2c-core.c

drivers/i2c/i2c-core.c static int i2c_device_probe(struct device *dev) · line 631

Quoted 7 lines

+++ b/drivers/i2c/i2c-core.c

drivers/i2c/i2c-core.c +1 -2

@@ -629,8 +629,13 @@ static int i2c_device_probe(struct device *dev)
      if (!client)
          return 0;
-    if (!client->irq && dev->of_node) {
-        int irq = of_irq_get(dev->of_node, 0);
+    if (client->irq <= 0) {

Isn't irq 0 a valid interrupt number ? Shouldn't it be client->irq < 0 ?

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

I've tested the patch and it works if the GPIO pin is enabled in the controller (which isn't as Octavian said). I believe that the patch should enable the pin and set the direction before it is used as IRQ.

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

Quoted 41 lines · Mika Westerberg

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 9 lines

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below and we can now directly use client->irq:

drivers/gpio/gpiolib.c +0 -0

index 568aa2b..9865627 100644

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as input\n", d->hwirq);
+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,

Quoted 65 lines · Octavian Purdila

Quoted 41 lines · Mika Westerberg

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 9 lines

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below and we can now directly use client->irq:

drivers/gpio/gpiolib.c +1 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = { · line 520

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }

What if the driver has already requested the GPIO?

Quoted 9 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as input\n", d->hwirq);
+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,

Quoted 66 lines · Mika Westerberg

Quoted 65 lines · Octavian Purdila

Quoted 41 lines · Mika Westerberg

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 9 lines

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below and we can now directly use client->irq:

drivers/gpio/gpiolib.c +1 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;

Quoted 9 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = { · line 520

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }

What if the driver has already requested the GPIO?

Initially I implemented the above to take that into account, e.g. if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) ...

But than I thought that we can't mess up with the GPIO anyway while the interrupt is in use.

One case I missed was if the user wants to read the GPIO while using it as an interrupt which seems to be possible...

Quoted 10 lines

Quoted 10 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as input\n", d->hwirq);
+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,

Quoted 78 lines · Octavian Purdila

Quoted 66 lines · Mika Westerberg

Quoted 65 lines · Octavian Purdila

Quoted 41 lines · Mika Westerberg

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 9 lines

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below and we can now directly use client->irq:

drivers/gpio/gpiolib.c +1 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;

Quoted 9 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = { · line 520

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }

What if the driver has already requested the GPIO?

Quoted 5 lines

Initially I implemented the above to take that into account, e.g. if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) ...

But than I thought that we can't mess up with the GPIO anyway while the interrupt is in use.

That's right but then the above will fail also normal cases. For example if the driver gets the irq like:

        desc = devm_gpiod_get(dev, ..);
        gpiod_direction_input(desc);
        irq = gpiod_to_irq(desc);

        ret = request_irq(irq, ...)

at this point we end up calling gpiochip_irq_reqres() which cannot request the GPIO again and fails.

Quoted 2 lines

One case I missed was if the user wants to read the GPIO while using it as an interrupt which seems to be possible...

While the GPIO is locked as IRQ it cannot be done as far as I can tell but you can work it around by calling free_irq() first.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as input\n", d->hwirq);
+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,

Quoted 92 lines · Mika Westerberg

Quoted 78 lines · Octavian Purdila

Quoted 66 lines · Mika Westerberg

Quoted 65 lines · Octavian Purdila

Quoted 41 lines · Mika Westerberg

Quoted 37 lines · Octavian Purdila

Quoted 18 lines · Mika Westerberg

Quoted 17 lines · Mika Westerberg

I think we can do the same for ACPI GpioInts so that we introduce acpi_gpio_irq_get() that translates from GpioInt to Linux IRQ numberspace. Then we can do something like below in I2C core:

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq = acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

Quoted 9 lines

Quoted 6 lines

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be available to the driver anymore (as a GPIO since it is locked) but if DT already does the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set input directions operations are not called anymore. Some gpio controller drivers (dln2, adnp, lynx_point from quickly browsing the code) do not explicitly enable the GPIO pin nor set direction to input when the interrupt is enabled. Depending on hardware this may be an issue - it is on dln2 for example.

Quoted 2 lines

Should the gpio controllers enable and set to input in irq_enable, irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Quoted 9 lines

Good question.

In general I think that it is assumed that the boot firmware configures the pin upfront. However, we have seen too many times that it actually doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below and we can now directly use client->irq:

drivers/gpio/gpiolib.c +1 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;

Quoted 9 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = { · line 520

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }

What if the driver has already requested the GPIO?

Quoted 5 lines

Initially I implemented the above to take that into account, e.g. if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) ...

But than I thought that we can't mess up with the GPIO anyway while the interrupt is in use.

That's right but then the above will fail also normal cases. For example if the driver gets the irq like:

        desc = devm_gpiod_get(dev, ..);
        gpiod_direction_input(desc);
        irq = gpiod_to_irq(desc);

Quoted 5 lines

        ret = request_irq(irq, ...)

at this point we end up calling gpiochip_irq_reqres() which cannot request the GPIO again and fails.

Good point, let me add back that check then :)

Quoted 6 lines

Quoted 2 lines

One case I missed was if the user wants to read the GPIO while using it as an interrupt which seems to be possible...

While the GPIO is locked as IRQ it cannot be done as far as I can tell but you can work it around by calling free_irq() first.

AFAICS you can not set the direction to output but you can still read values while the interrupt is active.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as input\n", d->hwirq);
+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,

Quoted 38 lines

Quoted 37 lines

Quoted 19 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpio/gpiolib.c +1 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;

Quoted 9 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = { · line 520

Quoted 6 lines

drivers/gpio/gpiolib.c +6 -0

+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }

What if the driver has already requested the GPIO?

Quoted 5 lines

Initially I implemented the above to take that into account, e.g. if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) ...

But than I thought that we can't mess up with the GPIO anyway while the interrupt is in use.

That's right but then the above will fail also normal cases. For example if the driver gets the irq like:

        desc = devm_gpiod_get(dev, ..);
        gpiod_direction_input(desc);
        irq = gpiod_to_irq(desc);

        ret = request_irq(irq, ...)

at this point we end up calling gpiochip_irq_reqres() which cannot request the GPIO again and fails.

Quoted 2 lines

Good point, let me add back that check then :)

I just realized that there is another issue: gpiochip_irq_reqres() is called under a spinlock, so we can call gpiod_request() only if the gpio controller does not sleep.

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 8 lines

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

OK, thanks.

In case of ACPI (where firmware does lot more) it is supposed to configure pins based on what is connected, if the firmware knows that. Due to bugs in the boot firmware that obviously does not happen in all cases (like this one).

If we want to support requesting IRQ directly through irqchip interface on ACPI systems there needs to be some way the GPIO/pinctrl core can use to turn the pin as input, GPIO mode.

Octavian's patch does that but not sure if it is good idea for non-ACPI systems as they already work. We could also do the same in GPIO/irqchip drivers but there is no guarantee for the caller that it is done. IMHO safest option is to explictly request the GPIO, turn it input and then convert the GPIO to interrupt. That way we know the pin is configured just like we expect.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · Mika Westerberg

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 8 lines

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

Quoted 7 lines

OK, thanks.

In case of ACPI (where firmware does lot more) it is supposed to configure pins based on what is connected, if the firmware knows that. Due to bugs in the boot firmware that obviously does not happen in all cases (like this one).

Ah, interesting, I was not aware that the firmware was supposed to do the pin configuration. In this case I think your patch can be merged as it is Mika, mine doesn't make sense anymore.

This particular case is special since we did not performed the tests on a full system that has the component integrated. We instead used and I2C to USB bridge to which we connected the component and we loaded the ACPI table dynamically.

Quoted 27 lines · Octavian Purdila

Quoted 17 lines · Mika Westerberg

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 8 lines

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

Quoted 7 lines

OK, thanks.

In case of ACPI (where firmware does lot more) it is supposed to configure pins based on what is connected, if the firmware knows that. Due to bugs in the boot firmware that obviously does not happen in all cases (like this one).

Quoted 3 lines

Ah, interesting, I was not aware that the firmware was supposed to do the pin configuration. In this case I think your patch can be merged as it is Mika, mine doesn't make sense anymore.

Unfortunately because of bugs we can't rely that the firmware (BIOS) gets those always right :-(

Quoted 4 lines

This particular case is special since we did not performed the tests on a full system that has the component integrated. We instead used and I2C to USB bridge to which we connected the component and we loaded the ACPI table dynamically.

Regardless of how did the device appear this always works:

1) request the GPIO (with GPIOD_IN)
2) convert it to IRQ using gpiod_to_irq()

Since we cannot be sure that the firmware has configured the pin properly, rather than adding automatic IRQ <-> GPIO translation for ACPI GpioInt I think we are better off if drivers explictly request their GPIOs and configure them as needed.

What if we can do the pin configuration in gpiolib right after the GPIO controller is initialized. I am thinking of searching the ACPI namespace and looking for resources that have GpioInt entries for that particular GPIO controller and use them to set the pins to input (and ideally also set the other properties like pullups, pulldowns, etc.)

Quoted 42 lines · Mika Westerberg

Quoted 27 lines · Octavian Purdila

Quoted 17 lines · Mika Westerberg

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 8 lines

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

Quoted 7 lines

OK, thanks.

In case of ACPI (where firmware does lot more) it is supposed to configure pins based on what is connected, if the firmware knows that. Due to bugs in the boot firmware that obviously does not happen in all cases (like this one).

Quoted 3 lines

Ah, interesting, I was not aware that the firmware was supposed to do the pin configuration. In this case I think your patch can be merged as it is Mika, mine doesn't make sense anymore.

Unfortunately because of bugs we can't rely that the firmware (BIOS) gets those always right :-(

Quoted 4 lines

This particular case is special since we did not performed the tests on a full system that has the component integrated. We instead used and I2C to USB bridge to which we connected the component and we loaded the ACPI table dynamically.

Regardless of how did the device appear this always works:

Quoted 7 lines

1) request the GPIO (with GPIOD_IN)
2) convert it to IRQ using gpiod_to_irq()

Since we cannot be sure that the firmware has configured the pin properly, rather than adding automatic IRQ <-> GPIO translation for ACPI GpioInt I think we are better off if drivers explictly request their GPIOs and configure them as needed.

Yes, this is what we are doing now and it works well.

But I am not yet ready to give-up on the automatic IRQ <-> GPIO translation :)

What if we can do the pin configuration in gpiolib right after the GPIO controller is initialized. I am thinking of searching the ACPI namespace and looking for resources that have GpioInt entries for that particular GPIO controller and use them to set the pins to input (and ideally also set the other properties like pullups, pulldowns, etc.)

Quoted 55 lines · Octavian Purdila

Quoted 42 lines · Mika Westerberg

Quoted 27 lines · Octavian Purdila

Quoted 17 lines · Mika Westerberg

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Mika Westerberg

Quoted 2 lines · Octavian Purdila

For the sleep case I think the GPIO controller needs to do the pin enable and set input direction operation in it's irq_bus_sync_unlock.

Quoted 8 lines

Quoted 2 lines

I wonder how DT handles all this? Is it the boot firmware that sets up the pins accordingly or is there something we are missing?

DT systems mostly do not have firmware for power usecases, they handle it all using pin control. I would more say that is a feature of all-SW systems without power-firmware ideas, without ACPI and without PSCI (well PSCI systems do not care about much more than CPU power down in firmware anyway...)

Quoted 7 lines

OK, thanks.

In case of ACPI (where firmware does lot more) it is supposed to configure pins based on what is connected, if the firmware knows that. Due to bugs in the boot firmware that obviously does not happen in all cases (like this one).

Quoted 3 lines

Ah, interesting, I was not aware that the firmware was supposed to do the pin configuration. In this case I think your patch can be merged as it is Mika, mine doesn't make sense anymore.

Unfortunately because of bugs we can't rely that the firmware (BIOS) gets those always right :-(

Quoted 4 lines

This particular case is special since we did not performed the tests on a full system that has the component integrated. We instead used and I2C to USB bridge to which we connected the component and we loaded the ACPI table dynamically.

Regardless of how did the device appear this always works:

Quoted 7 lines

1) request the GPIO (with GPIOD_IN)
2) convert it to IRQ using gpiod_to_irq()

Since we cannot be sure that the firmware has configured the pin properly, rather than adding automatic IRQ <-> GPIO translation for ACPI GpioInt I think we are better off if drivers explictly request their GPIOs and configure them as needed.

Yes, this is what we are doing now and it works well.

Quoted 7 lines

But I am not yet ready to give-up on the automatic IRQ <-> GPIO translation :)

What if we can do the pin configuration in gpiolib right after the GPIO controller is initialized. I am thinking of searching the ACPI namespace and looking for resources that have GpioInt entries for that particular GPIO controller and use them to set the pins to input (and ideally also set the other properties like pullups, pulldowns, etc.)

I would rather say that since the gpiolib and irqchip APIs are supposed to be orthogonal, the irqchip request/unmask or whatever functions are enabling IRQs should just go and poke the necessary bits in the GPIO registers to turn the pin into an input.

Quoted 7 lines · Mika Westerberg

Quoted 6 lines · Octavian Purdila

Quoted 4 lines · Mika Westerberg

Quoted 3 lines · Octavian Purdila

Quoted 1 line · Mika Westerberg

On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila

Quoted 3 lines · wrote:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 1 line · Mika Westerberg

On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg

Quoted 1 line · wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

I think we can do the same for ACPI GpioInts so that we

introduce

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

acpi_gpio_irq_get() that translates from GpioInt to Linux

IRQ

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

numberspace. Then we can do something like below in I2C

core:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

      if (client->irq <= 0) {
              int irq = -ENOENT;

                if (dev->of_node)
                        irq = of_irq_get(dev->of_node, 0);
                else if (ACPI_COMPANION(dev))
                        irq =

acpi_gpio_irq_get(ACPI_COMPANION(dev), 0);

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

              if (irq == -EPROBE_DEFER)
                        return irq;
                if (irq < 0)
                        irq = 0;

                client->irq = irq;
      }

Now it has the drawback that the first GpioInt will not be

available to

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the driver anymore (as a GPIO since it is locked) but if DT

already does

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

the same we should be fine.

Below patch should take care of this.

One issue we noticed is that now the gpio request and set

input

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

directions operations are not called anymore. Some gpio

controller

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

drivers (dln2, adnp, lynx_point from quickly browsing the

code) do not

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

explicitly enable the GPIO pin nor set direction to input when

the

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

interrupt is enabled. Depending on hardware this may be an

issue - it

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

is on dln2 for example.

Should the gpio controllers enable and set to input in

irq_enable,

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

irq_bus_sync_unlock, etc.? Or should this be done in gpiolib?

Good question.

In general I think that it is assumed that the boot firmware

configures

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the pin upfront. However, we have seen too many times that it

actually

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

doesn't happen or it is configured wrong.

Perhaps we could do this in GPIO core, for example in gpiochip_irq_reqres/gpiochip_irq_map or so.

That sounds good to me. We tested your patch with the patch below

and

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops

Quoted 7 lines

we can now directly use client->irq:

drivers/gpio/gpiolib.c +0 -0

index 568aa2b..9865627 100644
@@ -511,6 +511,19 @@ static const struct irq_domain_ops

gpiochip_domain_ops = {

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = {

Quoted 8 lines

drivers/gpio/gpiolib.c +5 -0

 static int gpiochip_irq_reqres(struct irq_data *d)
 {
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+       int ret;
+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n",

d->hwirq);

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops · line 520

Quoted 2 lines

drivers/gpio/gpiolib.c +2 -0

+               return ret;
+       }

What if the driver has already requested the GPIO?

Quoted 7 lines

Quoted 5 lines

Initially I implemented the above to take that into account, e.g. if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) ...

But than I thought that we can't mess up with the GPIO anyway while the interrupt is in use.

That's right but then the above will fail also normal cases. For

example

Quoted 11 lines

if the driver gets the irq like:

        desc = devm_gpiod_get(dev, ..);
        gpiod_direction_input(desc);
        irq = gpiod_to_irq(desc);

        ret = request_irq(irq, ...)

at this point we end up calling gpiochip_irq_reqres() which cannot request the GPIO again and fails.

Good point, let me add back that check then :)

Quoted 3 lines

Quoted 2 lines

One case I missed was if the user wants to read the GPIO while using it as an interrupt which seems to be possible...

Quoted 1 line

While the GPIO is locked as IRQ it cannot be done as far as I can

tell

Quoted 2 lines

but you can work it around by calling free_irq() first.

AFAICS you can not set the direction to output but you can still read values while the interrupt is active.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/gpio/gpiolib.c static const struct irq_domain_ops gpiochip_domain_ops = {

Quoted 6 lines

drivers/gpio/gpiolib.c +4 -0

+
+       ret = gpiod_direction_input(&chip->desc[d->hwirq]);
+       if (ret) {
+               chip_err(chip, "unable to set HW IRQ %lu as

input\n", d->hwirq);

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpio/gpiolib.c +2 -0

+               return ret;
+       }
 
        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,