[PATCH] IIO: Adds ACPI support for ST gyroscopes

[PATCH] IIO: Adds ACPI support for ST gyroscopes

[Robert Dolca]

Signed-off-by: Robert Dolca <robert.dolca@intel.com>
---
 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(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/iio/common/st_sensors/st_sensors_i2c.c
index 98cfee29..2f612ec 100644
--- a/drivers/iio/common/st_sensors/st_sensors_i2c.c
+++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
@@ -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)
+{
+	const struct acpi_device_id *id;
+	struct gpio_desc *gpiod_irq;
+	int ret;
+
+	id = acpi_match_device(match, &client->dev);
+	if (!id)
+		return -ENODEV;
+
+	/* 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;
+
+	/* 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)));
+
+	return 0;
+}
+EXPORT_SYMBOL(st_sensors_acpi_i2c_probe);
+
 MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
 MODULE_DESCRIPTION("STMicroelectronics ST-sensors i2c driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/iio/gyro/st_gyro_i2c.c b/drivers/iio/gyro/st_gyro_i2c.c
index 64480b1..712d23f 100644
--- a/drivers/iio/gyro/st_gyro_i2c.c
+++ b/drivers/iio/gyro/st_gyro_i2c.c
@@ -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,
diff --git a/include/linux/iio/common/st_sensors_i2c.h b/include/linux/iio/common/st_sensors_i2c.h
index 1796af0..2e90b8f 100644
--- a/include/linux/iio/common/st_sensors_i2c.h
+++ b/include/linux/iio/common/st_sensors_i2c.h
@@ -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 */
-- 
1.9.1

[PATCH] IIO: Add support for L3GD20H gyroscope

[Robert Dolca]

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

Signed-off-by: Robert Dolca <robert.dolca@intel.com>
---
 drivers/iio/gyro/st_gyro_core.c | 83 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index f07a233..21395f2 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -87,6 +87,31 @@
 #define ST_GYRO_2_DRDY_IRQ_INT2_MASK		0x08
 #define ST_GYRO_2_MULTIREAD_BIT			true
 
+/* CUSTOM VALUES FOR SENSOR 3 */
+#define ST_GYRO_3_WAI_EXP			0xd7
+#define ST_GYRO_3_ODR_ADDR			0x20
+#define ST_GYRO_3_ODR_MASK			0xc0
+#define ST_GYRO_3_ODR_AVL_95HZ_VAL		0x00
+#define ST_GYRO_3_ODR_AVL_190HZ_VAL		0x01
+#define ST_GYRO_3_ODR_AVL_380HZ_VAL		0x02
+#define ST_GYRO_3_ODR_AVL_760HZ_VAL		0x03
+#define ST_GYRO_3_PW_ADDR			0x20
+#define ST_GYRO_3_PW_MASK			0x08
+#define ST_GYRO_3_FS_ADDR			0x23
+#define ST_GYRO_3_FS_MASK			0x30
+#define ST_GYRO_3_FS_AVL_250_VAL		0x00
+#define ST_GYRO_3_FS_AVL_500_VAL		0x01
+#define ST_GYRO_3_FS_AVL_2000_VAL		0x02
+#define ST_GYRO_3_FS_AVL_250_GAIN		IIO_DEGREE_TO_RAD(8750)
+#define ST_GYRO_3_FS_AVL_500_GAIN		IIO_DEGREE_TO_RAD(17500)
+#define ST_GYRO_3_FS_AVL_2000_GAIN		IIO_DEGREE_TO_RAD(70000)
+#define ST_GYRO_3_BDU_ADDR			0x23
+#define ST_GYRO_3_BDU_MASK			0x80
+#define ST_GYRO_3_DRDY_IRQ_ADDR			0x22
+#define ST_GYRO_3_DRDY_IRQ_INT2_MASK		0x08
+#define ST_GYRO_3_MULTIREAD_BIT			true
+
+
 static const struct iio_chan_spec st_gyro_16bit_channels[] = {
 	ST_SENSORS_LSM_CHANNELS(IIO_ANGL_VEL,
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
@@ -225,6 +250,64 @@ static const struct st_sensor_settings st_gyro_sensors_settings[] = {
 		.multi_read_bit = ST_GYRO_2_MULTIREAD_BIT,
 		.bootime = 2,
 	},
+	{
+		.wai = ST_GYRO_3_WAI_EXP,
+		.sensors_supported = {
+			[0] = L3GD20_GYRO_DEV_NAME,
+		},
+		.ch = (struct iio_chan_spec *)st_gyro_16bit_channels,
+		.odr = {
+			.addr = ST_GYRO_3_ODR_ADDR,
+			.mask = ST_GYRO_3_ODR_MASK,
+			.odr_avl = {
+				{ 95, ST_GYRO_3_ODR_AVL_95HZ_VAL, },
+				{ 190, ST_GYRO_3_ODR_AVL_190HZ_VAL, },
+				{ 380, ST_GYRO_3_ODR_AVL_380HZ_VAL, },
+				{ 760, ST_GYRO_3_ODR_AVL_760HZ_VAL, },
+			},
+		},
+		.pw = {
+			.addr = ST_GYRO_3_PW_ADDR,
+			.mask = ST_GYRO_3_PW_MASK,
+			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
+			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
+		},
+		.enable_axis = {
+			.addr = ST_SENSORS_DEFAULT_AXIS_ADDR,
+			.mask = ST_SENSORS_DEFAULT_AXIS_MASK,
+		},
+		.fs = {
+			.addr = ST_GYRO_3_FS_ADDR,
+			.mask = ST_GYRO_3_FS_MASK,
+			.fs_avl = {
+				[0] = {
+					.num = ST_GYRO_FS_AVL_250DPS,
+					.value = ST_GYRO_3_FS_AVL_250_VAL,
+					.gain = ST_GYRO_3_FS_AVL_250_GAIN,
+				},
+				[1] = {
+					.num = ST_GYRO_FS_AVL_500DPS,
+					.value = ST_GYRO_3_FS_AVL_500_VAL,
+					.gain = ST_GYRO_3_FS_AVL_500_GAIN,
+				},
+				[2] = {
+					.num = ST_GYRO_FS_AVL_2000DPS,
+					.value = ST_GYRO_3_FS_AVL_2000_VAL,
+					.gain = ST_GYRO_3_FS_AVL_2000_GAIN,
+				},
+			},
+		},
+		.bdu = {
+			.addr = ST_GYRO_3_BDU_ADDR,
+			.mask = ST_GYRO_3_BDU_MASK,
+		},
+		.drdy_irq = {
+			.addr = ST_GYRO_3_DRDY_IRQ_ADDR,
+			.mask_int2 = ST_GYRO_3_DRDY_IRQ_INT2_MASK,
+		},
+		.multi_read_bit = ST_GYRO_3_MULTIREAD_BIT,
+		.bootime = 2,
+	},
 };
 
 static int st_gyro_read_raw(struct iio_dev *indio_dev,
-- 
1.9.1

Re: [PATCH] IIO: Add support for L3GD20H gyroscope

[Linus Walleij]

On Mon, Mar 23, 2015 at 2:40 PM, Robert Dolca <robert.dolca@intel.com> wrote:

> It can be used exactly like L3GD20 but it has a different WhoAmI
> register value.
>
> Signed-off-by: Robert Dolca <robert.dolca@intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] IIO: Add support for L3GD20H gyroscope

[Jonathan Cameron]

On 24/03/15 10:29, Linus Walleij wrote:
> On Mon, Mar 23, 2015 at 2:40 PM, Robert Dolca <robert.dolca@intel.com> wrote:
> 
>> It can be used exactly like L3GD20 but it has a different WhoAmI
>> register value.
>>
>> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij
> 
Applied to the togreg branch of iio.git initially pushed out as testing
for the autobuilders to play.

I'll hold off on the other patch until that discussion is resolved!

Jonathan

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Mon, Mar 23, 2015 at 03:40:24PM +0200, Robert Dolca wrote:
> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/iio/common/st_sensors/st_sensors_i2c.c
> index 98cfee29..2f612ec 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_i2c.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
> @@ -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)
> +{
> +	const struct acpi_device_id *id;
> +	struct gpio_desc *gpiod_irq;
> +	int ret;
> +
> +	id = acpi_match_device(match, &client->dev);
> +	if (!id)
> +		return -ENODEV;
> +
> +	/* Get IRQ GPIO */
> +	gpiod_irq = 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.

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

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

> +	/* Configure IRQ GPIO */
> +	ret = gpiod_direction_input(gpiod_irq);
> +	if (ret)
> +		return ret;
> +
> +	/* 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)));

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?

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(st_sensors_acpi_i2c_probe);
> +
>  MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
>  MODULE_DESCRIPTION("STMicroelectronics ST-sensors i2c driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/gyro/st_gyro_i2c.c b/drivers/iio/gyro/st_gyro_i2c.c
> index 64480b1..712d23f 100644
> --- a/drivers/iio/gyro/st_gyro_i2c.c
> +++ b/drivers/iio/gyro/st_gyro_i2c.c
> @@ -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,
> diff --git a/include/linux/iio/common/st_sensors_i2c.h b/include/linux/iio/common/st_sensors_i2c.h
> index 1796af0..2e90b8f 100644
> --- a/include/linux/iio/common/st_sensors_i2c.h
> +++ b/include/linux/iio/common/st_sensors_i2c.h
> @@ -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 */
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Daniel Baluta]

On Mon, Mar 23, 2015 at 5:18 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Mar 23, 2015 at 03:40:24PM +0200, Robert Dolca wrote:
>> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/iio/common/st_sensors/st_sensors_i2c.c
>> index 98cfee29..2f612ec 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_i2c.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
>> @@ -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)
>> +{
>> +     const struct acpi_device_id *id;
>> +     struct gpio_desc *gpiod_irq;
>> +     int ret;
>> +
>> +     id = acpi_match_device(match, &client->dev);
>> +     if (!id)
>> +             return -ENODEV;
>> +
>> +     /* Get IRQ GPIO */
>> +     gpiod_irq = 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.
>
>> +     if (IS_ERR(gpiod_irq))
>> +             return -ENODEV;
>
> Why not return the original error here? Now you lose things like
> -EPROBE_DEFER.
>
>> +     /* Configure IRQ GPIO */
>> +     ret = gpiod_direction_input(gpiod_irq);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* 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)));
>
> 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?

Hi Mika,

This is similar with device tree  aproach. Looking at st_sensors_of_i2c_probe,
we have:

»       /* The name from the OF match takes precedence if present */
»       strncpy(client->name, of_id->data, sizeof(client->name));
»       client->name[sizeof(client->name) - 1] = '\0';

We want to keep the compatibility with the device tree implementation. Not
sure if the custom name is really required.

Denis, perhaps you can offer more info on this.

Daniel.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Daniel Baluta]

On Mon, Mar 23, 2015 at 5:18 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Mar 23, 2015 at 03:40:24PM +0200, Robert Dolca wrote:
>> Signed-off-by: Robert Dolca <robert.dolca@intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/ii=
o/common/st_sensors/st_sensors_i2c.c
>> index 98cfee29..2f612ec 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_i2c.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
>> @@ -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 *cli=
ent,
>>  EXPORT_SYMBOL(st_sensors_of_i2c_probe);
>>  #endif
>>
>> +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 =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.
>
>> +     if (IS_ERR(gpiod_irq))
>> +             return -ENODEV;
>
> Why not return the original error here? Now you lose things like
> -EPROBE_DEFER.
>
>> +     /* 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?

Hi Mika,

This is similar with device tree  aproach. Looking at st_sensors_of_i2c_pro=
be,
we have:

=C2=BB       /* The name from the OF match takes precedence if present */
=C2=BB       strncpy(client->name, of_id->data, sizeof(client->name));
=C2=BB       client->name[sizeof(client->name) - 1] =3D '\0';

We want to keep the compatibility with the device tree implementation. Not
sure if the custom name is really required.

Denis, perhaps you can offer more info on this.

Daniel.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Mon, Mar 23, 2015 at 2:40 PM, Robert Dolca <robert.dolca@intel.com> wrote:

> Signed-off-by: Robert Dolca <robert.dolca@intel.com>

That's a very terse commit message.

> +       /* 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;


Why issue two calls when one is enough. And don't discard
the nice return code. Do this instead:

gpiod_irq = devm_gpiod_get_index(&client->dev, 0, 0, GPIOD_IN);
if (IS_ERR(gpiod_irq))
        return PTR_ERR(gpiod_irq);

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Mon, Mar 23, 2015 at 2:40 PM, Robert Dolca <robert.dolca@intel.com> wrote:

> Signed-off-by: Robert Dolca <robert.dolca@intel.com>

More comments...

> +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";

Look here:
drivers/iio/gyro/st_gyro.h

#define L3G4200D_GYRO_DEV_NAME          "l3g4200d"
#define LSM330D_GYRO_DEV_NAME           "lsm330d_gyro"
#define LSM330DL_GYRO_DEV_NAME          "lsm330dl_gyro"
#define LSM330DLC_GYRO_DEV_NAME         "lsm330dlc_gyro"
#define L3GD20_GYRO_DEV_NAME            "l3gd20"
#define L3G4IS_GYRO_DEV_NAME            "l3g4is_ui"
#define LSM330_GYRO_DEV_NAME            "lsm330_gyro"

Don't redo these definitons, use the defines from the header file.
The DT table uses them.

Atleast do:
static const char L3G4200D_gyro_dev_name[]  = L3G4200D_GYRO_DEV_NAME;

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Mon, Mar 23, 2015 at 2:40 PM, Robert Dolca <robert.dolca@intel.com> wrote:

Oh more comments still...

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

Shouldn't that be devm_gpiod_get_index_optional()?

I think the driver is useable also without that GPIO/IRQ
and should not error out. (Will cause bugs in other
platforms!)

Also: maybe the GPIO/IRQ support should be in a separate
patch so as not to confuse with the ACPI stuff.

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

[...]
> +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;
> +
> +	/* 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.

> +
> +	/* 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.

- Lars

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Robert Dolca]

On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> [...]
>>
>> +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;
>> +
>> +       /* 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.

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.

>
>> +
>> +       /* 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.

Robert

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

On 03/24/2015 02:26 PM, Robert Dolca wrote:
> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> [...]
>>>
>>> +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;
>>> +
>>> +       /* 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.
>
> 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.

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.

> 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.

The GPIO driver is responsible for configuring the pin accordingly so that 
it works in interrupt mode, when the IRQ that belongs to the pin is 
requested. So you don't have to rely on the default being input. No 
additional configuration on the GPIO should be necessary.

>
>>
>>> +
>>> +       /* 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.

Yes.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 03/24/2015 02:26 PM, Robert Dolca wrote:
>> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
>> 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.

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?

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
> On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> > On 03/24/2015 02:26 PM, Robert Dolca wrote:
> >> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
> >> 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.
> 
> 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?

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.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

Add Alexandre and linux-gpio to Cc.

On 03/24/2015 04:06 PM, Mika Westerberg wrote:
> On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
>> On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>>> On 03/24/2015 02:26 PM, Robert Dolca wrote:
>>>> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
>>>> 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.
>>
>> 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?
>
> 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.

>
> 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.

>
> 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.
>

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

Add Alexandre and linux-gpio to Cc.

On 03/24/2015 04:06 PM, Mika Westerberg wrote:
> On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
>> On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>> On 03/24/2015 02:26 PM, Robert Dolca wrote:
>>>> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>> 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.
>>
>> 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?
>
> 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.

>
> 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.

>
> 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.
>

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Daniel Baluta]

On Tue, Mar 24, 2015 at 5:22 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> Add Alexandre and linux-gpio to Cc.
>
>
> On 03/24/2015 04:06 PM, Mika Westerberg wrote:
>>
>> On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
>>>
>>> On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de>
>>> wrote:
>>>>
>>>> On 03/24/2015 02:26 PM, Robert Dolca wrote:
>>>>>
>>>>> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>>> 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.
>>>
>>>
>>> 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?
>>
>>
>> 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.
>
>>
>> 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.

Agree! Let's do it :)

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Tue, Mar 24, 2015 at 04:22:16PM +0100, Lars-Peter Clausen wrote:
> Add Alexandre and linux-gpio to Cc.
> 
> On 03/24/2015 04:06 PM, Mika Westerberg wrote:
> >On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
> >>On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>On 03/24/2015 02:26 PM, Robert Dolca wrote:
> >>>>On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
> >>>>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.
> >>
> >>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?
> >
> >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.

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.

> >
> >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.

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.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

On 03/24/2015 04:55 PM, Mika Westerberg wrote:
> On Tue, Mar 24, 2015 at 04:22:16PM +0100, Lars-Peter Clausen wrote:
>> Add Alexandre and linux-gpio to Cc.
>>
>> On 03/24/2015 04:06 PM, Mika Westerberg wrote:
>>> On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
>>>> On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>> On 03/24/2015 02:26 PM, Robert Dolca wrote:
>>>>>> On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
>>>>>> 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.
>>>>
>>>> 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?
>>>
>>> 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.
>
> 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.
>
>>>
>>> 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.
>
> 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?

- Lars

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Tue, Mar 24, 2015 at 05:43:21PM +0100, Lars-Peter Clausen wrote:
> On 03/24/2015 04:55 PM, Mika Westerberg wrote:
> >On Tue, Mar 24, 2015 at 04:22:16PM +0100, Lars-Peter Clausen wrote:
> >>Add Alexandre and linux-gpio to Cc.
> >>
> >>On 03/24/2015 04:06 PM, Mika Westerberg wrote:
> >>>On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
> >>>>On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
> >>>>>On 03/24/2015 02:26 PM, Robert Dolca wrote:
> >>>>>>On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> >>>>>>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.
> >>>>
> >>>>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?
> >>>
> >>>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.
> >
> >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.
> >
> >>>
> >>>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.
> >
> >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?

Yes, that's the place where we could assign the interrupt number. Given
that we can solve the above problems.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Tue, Mar 24, 2015 at 05:43:21PM +0100, Lars-Peter Clausen wrote:
> On 03/24/2015 04:55 PM, Mika Westerberg wrote:
> >On Tue, Mar 24, 2015 at 04:22:16PM +0100, Lars-Peter Clausen wrote:
> >>Add Alexandre and linux-gpio to Cc.
> >>
> >>On 03/24/2015 04:06 PM, Mika Westerberg wrote:
> >>>On Tue, Mar 24, 2015 at 02:57:49PM +0100, Linus Walleij wrote:
> >>>>On Tue, Mar 24, 2015 at 2:38 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>>>On 03/24/2015 02:26 PM, Robert Dolca wrote:
> >>>>>>On Tue, Mar 24, 2015 at 2:17 PM, Lars-Peter Clausen <lars@metafoo.de>
> >>>>>>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.
> >>>>
> >>>>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?
> >>>
> >>>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.
> >
> >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.
> >
> >>>
> >>>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.
> >
> >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?

Yes, that's the place where we could assign the interrupt number. Given
that we can solve the above problems.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Tue, Mar 24, 2015 at 4:06 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> 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?

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Wed, Mar 25, 2015 at 09:44:34AM +0100, Linus Walleij wrote:
> On Tue, Mar 24, 2015 at 4:06 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > 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>;

OK, I see.

> 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.

> But as long as the DTS file is consistent there is no problem.
> 
> So it seems the ACPI tables are lacking this semantic
> information?

I think the GpioIo/GpioInt separation serves the same purpose. Of course
both refer to GPIO controller instead of interrupt controller.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Wed, Mar 25, 2015 at 11:43:27AM +0200, Mika Westerberg wrote:
> On Wed, Mar 25, 2015 at 09:44:34AM +0100, Linus Walleij wrote:
> > On Tue, Mar 24, 2015 at 4:06 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > 
> > > 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>;
> 
> OK, I see.
> 
> > 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.

Answering my own question: of_irq_get() just translates the IRQ number
to the corresponding Linux IRQ number. Then when a driver calls
request_irq() irqchip callbacks for this GPIO controller (or generic
GPIO irqchip helpers) will lock the GPIO as IRQ.

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.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
> 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.

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.

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c0929d938ced..ed7c40c7b4ab 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -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,
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index edf274cabe81..c12a3f24ada4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 24c7aa8b1d20..826e214ab7d5 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -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 */

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Robert Dolca]

On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> 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.
>
> 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.
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c0929d938ced..ed7c40c7b4ab 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -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,
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index edf274cabe81..c12a3f24ada4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 24c7aa8b1d20..826e214ab7d5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -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 */

Hi Mika,

I will try it and come back with more info.

Robert

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[sathyanarayanan kuppuswamy]

Hi,


On 03/25/2015 06:21 AM, Mika Westerberg wrote:
> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> 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.
> 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.
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c0929d938ced..ed7c40c7b4ab 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -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,
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index edf274cabe81..c12a3f24ada4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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 ?
> +		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;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 24c7aa8b1d20..826e214ab7d5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[sathyanarayanan kuppuswamy]

Hi,


On 03/25/2015 06:21 AM, Mika Westerberg wrote:
> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> 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.
> 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.
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c0929d938ced..ed7c40c7b4ab 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -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,
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index edf274cabe81..c12a3f24ada4 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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 ?
> +		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;
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 24c7aa8b1d20..826e214ab7d5 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

On 03/25/2015 07:05 PM, sathyanarayanan kuppuswamy wrote:
>>   static acpi_status
>>   acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>>                   u32 bits, u64 *value, void *handler_context,
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index edf274cabe81..c12a3f24ada4 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -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 ?

0 is a invalid interrupt number these days, and is typically used to 
indicate no IRQ.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> 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.
>
> 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?

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Robert Dolca]

On Wed, Mar 25, 2015 at 11:12 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>>> 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.
>>
>> 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?

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.

Robert

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Thu, Mar 26, 2015 at 12:06:45PM +0200, Robert Dolca wrote:
> On Wed, Mar 25, 2015 at 11:12 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
> > On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> >> On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
> >>> 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.
> >>
> >> 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?
> 
> 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.

Thanks for testing. I noticed that acpi_gpio_irq_get() has few bugs and
I'll fix them before submitting upstream.

What comes to enabling GPIO and turning it to input mode, sure we can do
that but we need to figure out where we are supposed to do that.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
> >> 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.
> >
> > 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?

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.

Linus?

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
>> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> >> 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.
>> >
>> > 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?
>
> 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:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 568aa2b..9865627 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -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;
+
+       ret = gpiod_request(&chip->desc[d->hwirq], "IRQ");
+       if (ret) {
+               chip_err(chip, "unable to request %lu for IRQ\n", d->hwirq);
+               return ret;
+       }
+
+       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
> >> >> 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.
> >> >
> >> > 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?
> >
> > 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:
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 568aa2b..9865627 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -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;
> +
> +       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?

> +
> +       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
>> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
>> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>> >> <mika.westerberg@linux.intel.com> wrote:
>> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> >> >> 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.
>> >> >
>> >> > 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?
>> >
>> > 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:
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 568aa2b..9865627 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -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;
>> +
>> +       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...

>> +
>> +       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Thu, Mar 26, 2015 at 04:37:39PM +0200, Octavian Purdila wrote:
> On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
> >> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
> >> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
> >> >> <mika.westerberg@linux.intel.com> wrote:
> >> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
> >> >> >> 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.
> >> >> >
> >> >> > 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?
> >> >
> >> > 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:
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 568aa2b..9865627 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -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;
> >> +
> >> +       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.

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.

> 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.

> 
> >> +
> >> +       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Thu, Mar 26, 2015 at 4:47 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Mar 26, 2015 at 04:37:39PM +0200, Octavian Purdila wrote:
>> On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
>> >> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
>> >> <mika.westerberg@linux.intel.com> wrote:
>> >> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
>> >> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>> >> >> <mika.westerberg@linux.intel.com> wrote:
>> >> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>> >> >> >> 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.
>> >> >> >
>> >> >> > 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?
>> >> >
>> >> > 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:
>> >>
>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> >> index 568aa2b..9865627 100644
>> >> --- a/drivers/gpio/gpiolib.c
>> >> +++ b/drivers/gpio/gpiolib.c
>> >> @@ -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;
>> >> +
>> >> +       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.
>
> 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.
>

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

>> 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.

>>
>> >> +
>> >> +       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Thu, Mar 26, 2015 at 5:00 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Thu, Mar 26, 2015 at 4:47 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> On Thu, Mar 26, 2015 at 04:37:39PM +0200, Octavian Purdila wrote:
>>> On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
>>> <mika.westerberg@linux.intel.com> wrote:
>>> > On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
>>> >> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
>>> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila wrote:
>>> >> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>>> >> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg wrote:
>>> >> >> >> 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.
>>> >> >> >
>>> >> >> > 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?
>>> >> >
>>> >> > 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:
>>> >>
>>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> >> index 568aa2b..9865627 100644
>>> >> --- a/drivers/gpio/gpiolib.c
>>> >> +++ b/drivers/gpio/gpiolib.c
>>> >> @@ -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;
>>> >> +
>>> >> +       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.
>>
>> 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.
>>
>
> 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.

>>> 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.
>
>>>
>>> >> +
>>> >> +       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,

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
> >>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >>> >> index 568aa2b..9865627 100644
> >>> >> --- a/drivers/gpio/gpiolib.c
> >>> >> +++ b/drivers/gpio/gpiolib.c
> >>> >> @@ -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;
> >>> >> +
> >>> >> +       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.
> >>
> >> 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.
> >>
> >
> > 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.

Good point.

> 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?

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:

>> 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?

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...)

Sometimes the power-down/up path includes driving pins to
GND using the generic pin config option PIN_CONFIG_OUTPUT to
drive the logic.

For details on this mess where HW designers think that low-power
sleep mode is "GPIO-something" see Documentation/pinctrl.txt
section named "GPIO mode pitfalls". I..e the question is not what
registers are involved and what these are named, but the actual
usecase.

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
> 
> >> 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?
> 
> 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.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Mon, Mar 30, 2015 at 12:52 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
>> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
>>
>> >> 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?
>>
>> 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).
>

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.

> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Mon, Mar 30, 2015 at 03:55:14PM +0300, Octavian Purdila wrote:
> On Mon, Mar 30, 2015 at 12:52 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
> >> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
> >>
> >> >> 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?
> >>
> >> 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).
> >
> 
> 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 :-(

> 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.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Octavian Purdila]

On Mon, Mar 30, 2015 at 4:33 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Mar 30, 2015 at 03:55:14PM +0300, Octavian Purdila wrote:
>> On Mon, Mar 30, 2015 at 12:52 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
>> >> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
>> >> <mika.westerberg@linux.intel.com> wrote:
>> >> > On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
>> >>
>> >> >> 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?
>> >>
>> >> 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).
>> >
>>
>> 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 :-(
>
>> 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.


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.)

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Mika Westerberg]

On Mon, Mar 30, 2015 at 04:52:49PM +0300, Octavian Purdila wrote:
> 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.)

The pinctrl subsystem can do this already but currently there is no ACPI
support. Using information from GpioInt to fill pinctrl pin
configuration sounds good to me.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Linus Walleij]

On Mon, Mar 30, 2015 at 3:52 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Mon, Mar 30, 2015 at 4:33 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
>> On Mon, Mar 30, 2015 at 03:55:14PM +0300, Octavian Purdila wrote:
>>> On Mon, Mar 30, 2015 at 12:52 PM, Mika Westerberg
>>> <mika.westerberg@linux.intel.com> wrote:
>>> > On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
>>> >> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
>>> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> > On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
>>> >>
>>> >> >> 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?
>>> >>
>>> >> 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).
>>> >
>>>
>>> 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 :-(
>>
>>> 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.
>
>
> 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.)

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.

spinlocks to avoid collision with gpiolib usecases if necessary,
make GPIO side of the driver not assume it has sole control
over the registers or their contents, use gpiolib_irqchip or other
mechanism that will mark the line as used for IRQs.

Yours,
Linus Walleij

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Lars-Peter Clausen]

On 04/07/2015 11:35 AM, Linus Walleij wrote:
> On Mon, Mar 30, 2015 at 3:52 PM, Octavian Purdila
> <octavian.purdila@intel.com> wrote:
>> On Mon, Mar 30, 2015 at 4:33 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>>> On Mon, Mar 30, 2015 at 03:55:14PM +0300, Octavian Purdila wrote:
>>>> On Mon, Mar 30, 2015 at 12:52 PM, Mika Westerberg
>>>> <mika.westerberg@linux.intel.com> wrote:
>>>>> On Fri, Mar 27, 2015 at 11:36:25AM +0100, Linus Walleij wrote:
>>>>>> On Fri, Mar 27, 2015 at 11:06 AM, Mika Westerberg
>>>>>> <mika.westerberg@linux.intel.com> wrote:
>>>>>>> On Thu, Mar 26, 2015 at 06:28:19PM +0200, Octavian Purdila wrote:
>>>>>>
>>>>>>>> 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?
>>>>>>
>>>>>> 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).
>>>>>
>>>>
>>>> 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 :-(
>>>
>>>> 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.
>>
>>
>> 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.)
>
> 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.

Yes, especially considering that for some controllers they actually are 
orthogonal. I.e. different bits need to be set depending on whether the pin 
should be used as a GPIO input or as a interrupt pin.

It may make sense to offer some helper functions or flags for those chips 
were the pin needs to be configured as input so the core can take care of it 
and not every driver needs to do it on its own.

- Lars

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Jonathan Cameron]

On 26 March 2015 15:00:43 GMT+00:00, Octavian Purdila <octavian.purdila@intel.com> wrote:
>On Thu, Mar 26, 2015 at 4:47 PM, Mika Westerberg
><mika.westerberg@linux.intel.com> wrote:
>> On Thu, Mar 26, 2015 at 04:37:39PM +0200, Octavian Purdila wrote:
>>> On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
>>> <mika.westerberg@linux.intel.com> wrote:
>>> > On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
>>> >> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
>>> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila
>wrote:
>>> >> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>>> >> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg
>wrote:
>>> >> >> >> 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.
>>> >> >> >
>>> >> >> > 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?
>>> >> >
>>> >> > 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:
>>> >>
>>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> >> index 568aa2b..9865627 100644
>>> >> --- a/drivers/gpio/gpiolib.c
>>> >> +++ b/drivers/gpio/gpiolib.c
>>> >> @@ -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;
>>> >> +
>>> >> +       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.
>>
>> 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.
>>
>
>Good point, let me add back that check then :)
>
>>> 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.

That's right. We have recently run into fun with this with some of the weirder buses.
On those we care about tight timing so need interrupts but the pin is used to
 send as well so we go through an elaborate dance of disabling interrupts,
 flipping direction setting the output and then flipping back again.
We used to get away with setting a pin to output with the interrupt enabled :) no longer...
>
>>>
>>> >> +
>>> >> +       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,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] IIO: Adds ACPI support for ST gyroscopes

[Jonathan Cameron]

On 26 March 2015 15:00:43 GMT+00:00, Octavian Purdila <octavian.purdila@intel.com> wrote:
>On Thu, Mar 26, 2015 at 4:47 PM, Mika Westerberg
><mika.westerberg@linux.intel.com> wrote:
>> On Thu, Mar 26, 2015 at 04:37:39PM +0200, Octavian Purdila wrote:
>>> On Thu, Mar 26, 2015 at 4:04 PM, Mika Westerberg
>>> <mika.westerberg@linux.intel.com> wrote:
>>> > On Thu, Mar 26, 2015 at 02:04:35PM +0200, Octavian Purdila wrote:
>>> >> On Thu, Mar 26, 2015 at 12:16 PM, Mika Westerberg
>>> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> > On Wed, Mar 25, 2015 at 11:12:16PM +0200, Octavian Purdila
>wrote:
>>> >> >> On Wed, Mar 25, 2015 at 3:21 PM, Mika Westerberg
>>> >> >> <mika.westerberg@linux.intel.com> wrote:
>>> >> >> > On Wed, Mar 25, 2015 at 02:25:05PM +0200, Mika Westerberg
>wrote:
>>> >> >> >> 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.
>>> >> >> >
>>> >> >> > 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?
>>> >> >
>>> >> > 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:
>>> >>
>>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> >> index 568aa2b..9865627 100644
>>> >> --- a/drivers/gpio/gpiolib.c
>>> >> +++ b/drivers/gpio/gpiolib.c
>>> >> @@ -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;
>>> >> +
>>> >> +       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.
>>
>> 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.
>>
>
>Good point, let me add back that check then :)
>
>>> 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.

That's right. We have recently run into fun with this with some of the weirder buses.
On those we care about tight timing so need interrupts but the pin is used to
 send as well so we go through an elaborate dance of disabling interrupts,
 flipping direction setting the output and then flipping back again.
We used to get away with setting a pin to output with the interrupt enabled :) no longer...
>
>>>
>>> >> +
>>> >> +       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,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.