[RFC PATCH] gpio: support for GPIO forwarding

[RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

This makes it possible to assign GPIOs at runtime. The
motivation for it is because of need to forward GPIOs from
one device to an other. That feature may be useful for
example with some mfd devices, but initially it is needed
because on some Intel Braswell based ACPI platforms the GPIO
resources controlling signals of the USB PHY are given to
the controller device object for whatever reason, so the
driver of that controller needs be able to pass them to the
PHY device somehow.

So basically this is meant to allow patching of bad (bad
from Linux kernels point of view) hardware descriptions
provided by system fw in the drivers.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---

Hi,

I'm sending this first as a RFC in case you guys have some better
idea how to solve our problem. I actually already have couple
platforms where the GPIO resources are given to the "wrong" device
objects now.

Originally we were thinking about simply handling our problem with
hacks to the PHY drivers, basically also checking if the parent has
GPIOs. That would only work if the controller is always the parent,
which it's not, but even if it was, it would be too risky. The PHY
drivers don't know which controller they are attached to, so what is
to say that the GPIOs aren't really attached to the controller.

So the safest way to handle our problem is to deal with it in quirks
in the controller drivers. Solving it by bouncing the GPIOs did not
feel ideal there doesn't seem to be any other way. The API is copied
from clkdev (clk_register_clkdev). In the end it's really only one
function for adding the lookups and one for removing them.

The way it's implemented is by modifying the current style of handling
the lookups a little bit. The per device lookup table is basically
reverted back to the single linked-list format since it seems that
these lookups may have to be assigned from several sources.

Thanks,

 Documentation/gpio/board.txt                 |  17 ++---
 Documentation/gpio/consumer.txt              |  15 ++++
 arch/arm/mach-integrator/impd1.c             |  22 +++---
 arch/arm/mach-omap2/board-rx51-peripherals.c |  11 +--
 arch/arm/mach-tegra/board-paz00.c            |  13 ++--
 arch/mips/jz4740/board-qi_lb60.c             |  13 ++--
 drivers/gpio/gpiolib.c                       | 110 ++++++++++++++++-----------
 include/linux/gpio/consumer.h                |  22 ++++++
 include/linux/gpio/machine.h                 |  20 ++---
 9 files changed, 146 insertions(+), 97 deletions(-)

diff --git a/Documentation/gpio/board.txt b/Documentation/gpio/board.txt
index 4452786..58a0eaf 100644
--- a/Documentation/gpio/board.txt
+++ b/Documentation/gpio/board.txt
@@ -90,20 +90,17 @@ Note that GPIO_LOOKUP() is just a shortcut to GPIO_LOOKUP_IDX() where idx = 0.
 A lookup table can then be defined as follows, with an empty entry defining its
 end:
 
-struct gpiod_lookup_table gpios_table = {
-	.dev_id = "foo.0",
-	.table = {
-		GPIO_LOOKUP_IDX("gpio.0", 15, "led", 0, GPIO_ACTIVE_HIGH),
-		GPIO_LOOKUP_IDX("gpio.0", 16, "led", 1, GPIO_ACTIVE_HIGH),
-		GPIO_LOOKUP_IDX("gpio.0", 17, "led", 2, GPIO_ACTIVE_HIGH),
-		GPIO_LOOKUP("gpio.0", 1, "power", GPIO_ACTIVE_LOW),
-		{ },
-	},
+struct gpiod_lookup gpios_table[] = {
+	GPIO_LOOKUP_IDX("gpio.0", 15, "foo.0", "led", 0, GPIO_ACTIVE_HIGH),
+	GPIO_LOOKUP_IDX("gpio.0", 16, "foo.0", "led", 1, GPIO_ACTIVE_HIGH),
+	GPIO_LOOKUP_IDX("gpio.0", 17, "foo.0", "led", 2, GPIO_ACTIVE_HIGH),
+	GPIO_LOOKUP("gpio.0", 1, "foo.0", "power", GPIO_ACTIVE_LOW),
+	{ },
 };
 
 And the table can be added by the board code as follows:
 
-	gpiod_add_lookup_table(&gpios_table);
+	gpiod_add_lookup_table(gpios_table);
 
 The driver controlling "foo.0" will then be able to obtain its GPIOs as follows:
 
diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
index d85fbae..21b2eee 100644
--- a/Documentation/gpio/consumer.txt
+++ b/Documentation/gpio/consumer.txt
@@ -281,3 +281,18 @@ descriptor is only possible after the GPIO number has been released.
 
 Freeing a GPIO obtained by one API with the other API is forbidden and an
 unchecked error.
+
+Runtime GPIO Mappings
+=====================
+If a GPIO needs to be assigning to consumer when the system is already up and
+running, a lookup can be created and later removed with the following
+functions:
+
+	int gpiod_create_lookup(struct gpio_desc *desc, const char *con_id,
+				const char *dev_id)
+	void gpiod_remove_lookup(struct gpio_desc *desc, const char *con_id,
+				 const char *dev_id)
+
+Where "dev_id" is the device name of the consumer and "con_id" the connection
+identifier that can be used to identify the GPIO when it's requested by that
+consumer.
diff --git a/arch/arm/mach-integrator/impd1.c b/arch/arm/mach-integrator/impd1.c
index 38b0da3..70ff0b5 100644
--- a/arch/arm/mach-integrator/impd1.c
+++ b/arch/arm/mach-integrator/impd1.c
@@ -386,16 +386,14 @@ static int __init_refok impd1_probe(struct lm_device *dev)
 
 		/* Add GPIO descriptor lookup table for the PL061 block */
 		if (idev->offset == 0x00400000) {
-			struct gpiod_lookup_table *lookup;
+			struct gpiod_lookup *lookup;
 			char *chipname;
 			char *mmciname;
 
-			lookup = devm_kzalloc(&dev->dev,
-					      sizeof(*lookup) + 3 * sizeof(struct gpiod_lookup),
+			lookup = devm_kzalloc(&dev->dev, sizeof(*lookup) * 3,
 					      GFP_KERNEL);
 			chipname = devm_kstrdup(&dev->dev, devname, GFP_KERNEL);
 			mmciname = kasprintf(GFP_KERNEL, "lm%x:00700", dev->id);
-			lookup->dev_id = mmciname;
 			/*
 			 * Offsets on GPIO block 1:
 			 * 3 = MMC WP (write protect)
@@ -410,13 +408,15 @@ static int __init_refok impd1_probe(struct lm_device *dev)
 			 * 5 = Key lower right
 			 */
 			/* We need the two MMCI GPIO entries */
-			lookup->table[0].chip_label = chipname;
-			lookup->table[0].chip_hwnum = 3;
-			lookup->table[0].con_id = "wp";
-			lookup->table[1].chip_label = chipname;
-			lookup->table[1].chip_hwnum = 4;
-			lookup->table[1].con_id = "cd";
-			lookup->table[1].flags = GPIO_ACTIVE_LOW;
+			lookup[0].chip_label = chipname;
+			lookup[0].chip_hwnum = 3;
+			lookup[0].dev_id = mmciname;
+			lookup[0].con_id = "wp";
+			lookup[1].chip_label = chipname;
+			lookup[1].chip_hwnum = 4;
+			lookup[1].dev_id = mmciname;
+			lookup[1].con_id = "cd";
+			lookup[1].flags = GPIO_ACTIVE_LOW;
 			gpiod_add_lookup_table(lookup);
 		}
 
diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
index 14edcd7..606ba16 100644
--- a/arch/arm/mach-omap2/board-rx51-peripherals.c
+++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
@@ -756,17 +756,14 @@ static struct regulator_init_data rx51_vintdig = {
 	},
 };
 
-static struct gpiod_lookup_table rx51_fmtx_gpios_table = {
-	.dev_id = "2-0063",
-	.table = {
-		GPIO_LOOKUP("gpio.6", 3, "reset", GPIO_ACTIVE_HIGH), /* 163 */
-		{ },
-	},
+static struct gpiod_lookup rx51_fmtx_gpios_table[] = {
+	GPIO_LOOKUP("gpio.6", 3, "2-0063", "reset", GPIO_ACTIVE_HIGH), /* 163 */
+	{ },
 };
 
 static __init void rx51_gpio_init(void)
 {
-	gpiod_add_lookup_table(&rx51_fmtx_gpios_table);
+	gpiod_add_lookup_table(rx51_fmtx_gpios_table);
 }
 
 static int rx51_twlgpio_setup(struct device *dev, unsigned gpio, unsigned n)
diff --git a/arch/arm/mach-tegra/board-paz00.c b/arch/arm/mach-tegra/board-paz00.c
index fbe74c6..fedd7d5 100644
--- a/arch/arm/mach-tegra/board-paz00.c
+++ b/arch/arm/mach-tegra/board-paz00.c
@@ -36,17 +36,14 @@ static struct platform_device wifi_rfkill_device = {
 	},
 };
 
-static struct gpiod_lookup_table wifi_gpio_lookup = {
-	.dev_id = "rfkill_gpio",
-	.table = {
-		GPIO_LOOKUP_IDX("tegra-gpio", 25, NULL, 0, 0),
-		GPIO_LOOKUP_IDX("tegra-gpio", 85, NULL, 1, 0),
-		{ },
-	},
+static struct gpiod_lookup wifi_gpio_lookup[] = {
+	GPIO_LOOKUP_IDX("tegra-gpio", 25, "rfkill_gpio", NULL, 0, 0),
+	GPIO_LOOKUP_IDX("tegra-gpio", 85, "rfkill_gpio", NULL, 1, 0),
+	{ },
 };
 
 void __init tegra_paz00_wifikill_init(void)
 {
-	gpiod_add_lookup_table(&wifi_gpio_lookup);
+	gpiod_add_lookup_table(wifi_gpio_lookup);
 	platform_device_register(&wifi_rfkill_device);
 }
diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c
index c454525..7b94feb 100644
--- a/arch/mips/jz4740/board-qi_lb60.c
+++ b/arch/mips/jz4740/board-qi_lb60.c
@@ -426,13 +426,10 @@ static struct platform_device qi_lb60_audio_device = {
 	.id = -1,
 };
 
-static struct gpiod_lookup_table qi_lb60_audio_gpio_table = {
-	.dev_id = "qi-lb60-audio",
-	.table = {
-		GPIO_LOOKUP("Bank B", 29, "snd", 0),
-		GPIO_LOOKUP("Bank D", 4, "amp", 0),
-		{ },
-	},
+static struct gpiod_lookup qi_lb60_audio_gpio_table[] = {
+	GPIO_LOOKUP("Bank B", 29, "qi-lb60-audio", "snd", 0),
+	GPIO_LOOKUP("Bank D", 4, "qi-lb60-audio", "amp", 0),
+	{ },
 };
 
 static struct platform_device *jz_platform_devices[] __initdata = {
@@ -471,7 +468,7 @@ static int __init qi_lb60_init_platform_devices(void)
 	jz4740_adc_device.dev.platform_data = &qi_lb60_battery_pdata;
 	jz4740_mmc_device.dev.platform_data = &qi_lb60_mmc_pdata;
 
-	gpiod_add_lookup_table(&qi_lb60_audio_gpio_table);
+	gpiod_add_lookup_table(qi_lb60_audio_gpio_table);
 
 	jz4740_serial_device_register();
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 487afe6..36dc2a9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1644,14 +1644,62 @@ EXPORT_SYMBOL_GPL(gpiod_set_array_cansleep);
  * gpiod_add_lookup_table() - register GPIO device consumers
  * @table: table of consumers to register
  */
-void gpiod_add_lookup_table(struct gpiod_lookup_table *table)
+void gpiod_add_lookup_table(struct gpiod_lookup *table)
 {
 	mutex_lock(&gpio_lookup_lock);
+	for ( ; table->chip_label; table++)
+		list_add_tail(&table->list, &gpio_lookup_list);
+	mutex_unlock(&gpio_lookup_lock);
+}
 
-	list_add_tail(&table->list, &gpio_lookup_list);
+/**
+ * gpiod_create_lookup() - register consumer for a GPIO
+ * @desc: the GPIO
+ * @con_id: name of the GPIO from the device's point of view
+ * @dev_id: device name of the consumer for this GPIO
+ */
+int gpiod_create_lookup(struct gpio_desc *desc, const char *con_id,
+			const char *dev_id)
+{
+	struct gpiod_lookup *gl;
+
+	gl = kzalloc(sizeof(*gl), GFP_KERNEL);
+	if (!gl)
+		return -ENOMEM;
 
+	gl->con_id = con_id;
+	gl->dev_id = dev_id;
+	gl->desc = desc;
+
+	mutex_lock(&gpio_lookup_lock);
+	list_add_tail(&gl->list, &gpio_lookup_list);
 	mutex_unlock(&gpio_lookup_lock);
+	return 0;
 }
+EXPORT_SYMBOL_GPL(gpiod_create_lookup);
+
+/**
+ * gpiod_remove_lookup() - remove GPIO consumer
+ * @desc: the GPIO
+ * @con_id: name of the GPIO from the device's point of view
+ * @dev_id: device name of the consumer for this GPIO
+ */
+void gpiod_remove_lookup(struct gpio_desc *desc, const char *con_id,
+			 const char *dev_id)
+{
+	struct gpiod_lookup *gl;
+
+	mutex_lock(&gpio_lookup_lock);
+	list_for_each_entry(gl, &gpio_lookup_list, list)
+		if (gl->desc == desc && !strcmp(gl->dev_id, dev_id) &&
+		    !strcmp(gl->con_id, con_id)) {
+			list_del(&gl->list);
+			kfree(gl);
+			break;
+		}
+	mutex_unlock(&gpio_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(gpiod_remove_lookup);
 
 static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 				      unsigned int idx,
@@ -1723,52 +1771,22 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 	return desc;
 }
 
-static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev)
-{
-	const char *dev_id = dev ? dev_name(dev) : NULL;
-	struct gpiod_lookup_table *table;
-
-	mutex_lock(&gpio_lookup_lock);
-
-	list_for_each_entry(table, &gpio_lookup_list, list) {
-		if (table->dev_id && dev_id) {
-			/*
-			 * Valid strings on both ends, must be identical to have
-			 * a match
-			 */
-			if (!strcmp(table->dev_id, dev_id))
-				goto found;
-		} else {
-			/*
-			 * One of the pointers is NULL, so both must be to have
-			 * a match
-			 */
-			if (dev_id == table->dev_id)
-				goto found;
-		}
-	}
-	table = NULL;
-
-found:
-	mutex_unlock(&gpio_lookup_lock);
-	return table;
-}
-
 static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 				    unsigned int idx,
 				    enum gpio_lookup_flags *flags)
 {
+	const char *dev_id = dev ? dev_name(dev) : NULL;
 	struct gpio_desc *desc = ERR_PTR(-ENOENT);
-	struct gpiod_lookup_table *table;
 	struct gpiod_lookup *p;
 
-	table = gpiod_find_lookup_table(dev);
-	if (!table)
-		return desc;
-
-	for (p = &table->table[0]; p->chip_label; p++) {
+	mutex_lock(&gpio_lookup_lock);
+	list_for_each_entry(p, &gpio_lookup_list, list) {
 		struct gpio_chip *chip;
 
+		/* If the lookup entry has a dev_id, require exact match */
+		if (p->dev_id && (!dev_id || strcmp(p->dev_id, dev_id)))
+			continue;
+
 		/* idx must always match exactly */
 		if (p->idx != idx)
 			continue;
@@ -1777,27 +1795,33 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 		if (p->con_id && (!con_id || strcmp(p->con_id, con_id)))
 			continue;
 
+		if (p->desc) {
+			desc = p->desc;
+			break;
+		}
+
 		chip = find_chip_by_name(p->chip_label);
 
 		if (!chip) {
 			dev_err(dev, "cannot find GPIO chip %s\n",
 				p->chip_label);
-			return ERR_PTR(-ENODEV);
+			break;
 		}
 
 		if (chip->ngpio <= p->chip_hwnum) {
 			dev_err(dev,
 				"requested GPIO %d is out of range [0..%d] for chip %s\n",
 				idx, chip->ngpio, chip->label);
-			return ERR_PTR(-EINVAL);
+			desc = ERR_PTR(-EINVAL);
+			break;
 		}
 
 		desc = gpiochip_get_desc(chip, p->chip_hwnum);
 		*flags = p->flags;
 
-		return desc;
+		break;
 	}
-
+	mutex_unlock(&gpio_lookup_lock);
 	return desc;
 }
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index fd85cb1..d6e7579 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -111,6 +111,13 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 					 const char *propname);
 struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
 					    struct fwnode_handle *child);
+
+/* For passing forward GPIOs */
+int gpiod_create_lookup(struct gpio_desc *desc, const char *con_id,
+			const char *dev_id);
+void gpiod_remove_lookup(struct gpio_desc *desc, const char *con_id,
+			 const char *dev_id);
+
 #else /* CONFIG_GPIOLIB */
 
 static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
@@ -329,6 +336,21 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 	return -EINVAL;
 }
 
+static inline int gpiod_create_lookup(struct gpio_desc *desc,
+				      const char *con_id, const char *dev_id)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return -ENOSYS;
+}
+
+static inline void gpiod_remove_lookup(struct gpio_desc *desc,
+				       const char *con_id, const char *dev_id)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
+
 #endif /* CONFIG_GPIOLIB */
 
 /*
diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h
index e270614..b367a19 100644
--- a/include/linux/gpio/machine.h
+++ b/include/linux/gpio/machine.h
@@ -15,6 +15,7 @@ enum gpio_lookup_flags {
  * struct gpiod_lookup - lookup table
  * @chip_label: name of the chip the GPIO belongs to
  * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @dev_id: name of the consumer device for this GPIO
  * @con_id: name of the GPIO from the device's point of view
  * @idx: index of the GPIO in case several GPIOs share the same name
  * @flags: mask of GPIO_* values
@@ -23,39 +24,38 @@ enum gpio_lookup_flags {
  * functions using platform data.
  */
 struct gpiod_lookup {
+	struct list_head list;
 	const char *chip_label;
 	u16 chip_hwnum;
+	const char *dev_id;
 	const char *con_id;
 	unsigned int idx;
 	enum gpio_lookup_flags flags;
-};
-
-struct gpiod_lookup_table {
-	struct list_head list;
-	const char *dev_id;
-	struct gpiod_lookup table[];
+	void *desc;
 };
 
 /*
  * Simple definition of a single GPIO under a con_id
  */
-#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _con_id, _flags) \
-	GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, 0, _flags)
+#define GPIO_LOOKUP(_chip_label, _chip_hwnum, _dev_id, _con_id, _flags) \
+	GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _dev_id, _con_id, 0, _flags)
 
 /*
  * Use this macro if you need to have several GPIOs under the same con_id.
  * Each GPIO needs to use a different index and can be accessed using
  * gpiod_get_index()
  */
-#define GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _con_id, _idx, _flags)  \
+#define GPIO_LOOKUP_IDX(_chip_label, _chip_hwnum, _dev_id, _con_id, _idx, \
+			_flags)						  \
 {                                                                         \
 	.chip_label = _chip_label,                                        \
 	.chip_hwnum = _chip_hwnum,                                        \
+	.dev_id = _dev_id,                                                \
 	.con_id = _con_id,                                                \
 	.idx = _idx,                                                      \
 	.flags = _flags,                                                  \
 }
 
-void gpiod_add_lookup_table(struct gpiod_lookup_table *table);
+void gpiod_add_lookup_table(struct gpiod_lookup *table);
 
 #endif /* __LINUX_GPIO_MACHINE_H */
-- 
2.1.3

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Thu, Dec 18, 2014 at 10:23:18AM +0200, Heikki Krogerus wrote:
> This makes it possible to assign GPIOs at runtime. The
> motivation for it is because of need to forward GPIOs from
> one device to an other. That feature may be useful for
> example with some mfd devices, but initially it is needed
> because on some Intel Braswell based ACPI platforms the GPIO
> resources controlling signals of the USB PHY are given to
> the controller device object for whatever reason, so the
> driver of that controller needs be able to pass them to the
> PHY device somehow.
> 
> So basically this is meant to allow patching of bad (bad
> from Linux kernels point of view) hardware descriptions
> provided by system fw in the drivers.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> 
> Hi,
> 
> I'm sending this first as a RFC in case you guys have some better
> idea how to solve our problem. I actually already have couple
> platforms where the GPIO resources are given to the "wrong" device
> objects now.
> 
> Originally we were thinking about simply handling our problem with
> hacks to the PHY drivers, basically also checking if the parent has
> GPIOs. That would only work if the controller is always the parent,
> which it's not, but even if it was, it would be too risky. The PHY
> drivers don't know which controller they are attached to, so what is
> to say that the GPIOs aren't really attached to the controller.
> 
> So the safest way to handle our problem is to deal with it in quirks
> in the controller drivers. Solving it by bouncing the GPIOs did not
> feel ideal there doesn't seem to be any other way. The API is copied
> from clkdev (clk_register_clkdev). In the end it's really only one
> function for adding the lookups and one for removing them.
> 
> The way it's implemented is by modifying the current style of handling
> the lookups a little bit. The per device lookup table is basically
> reverted back to the single linked-list format since it seems that
> these lookups may have to be assigned from several sources.
> 
> Thanks,

Gentle ping on this.

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:

> This makes it possible to assign GPIOs at runtime. The
> motivation for it is because of need to forward GPIOs from
> one device to an other. That feature may be useful for
> example with some mfd devices, but initially it is needed
> because on some Intel Braswell based ACPI platforms the GPIO
> resources controlling signals of the USB PHY are given to
> the controller device object for whatever reason, so the
> driver of that controller needs be able to pass them to the
> PHY device somehow.
>
> So basically this is meant to allow patching of bad (bad
> from Linux kernels point of view) hardware descriptions
> provided by system fw in the drivers.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>
> Hi,
>
> I'm sending this first as a RFC in case you guys have some better
> idea how to solve our problem. I actually already have couple
> platforms where the GPIO resources are given to the "wrong" device
> objects now.
>
> Originally we were thinking about simply handling our problem with
> hacks to the PHY drivers, basically also checking if the parent has
> GPIOs. That would only work if the controller is always the parent,
> which it's not, but even if it was, it would be too risky. The PHY
> drivers don't know which controller they are attached to, so what is
> to say that the GPIOs aren't really attached to the controller.
>
> So the safest way to handle our problem is to deal with it in quirks
> in the controller drivers. Solving it by bouncing the GPIOs did not
> feel ideal there doesn't seem to be any other way. The API is copied
> from clkdev (clk_register_clkdev). In the end it's really only one
> function for adding the lookups and one for removing them.
>
> The way it's implemented is by modifying the current style of handling
> the lookups a little bit. The per device lookup table is basically
> reverted back to the single linked-list format since it seems that
> these lookups may have to be assigned from several sources.

Oh ain't that great.

So now we start patching around the kernel because the ACPI
tables are erroneous for GPIOs. I'd like some feedback from
Rafael &| Darren on this patch, i.e. if you two think this is a good
way of accounting for bad GPIO descriptions in ACPI tables then
ACK this patch.

I guess the other option would be to fix up the ACPI DSDT
itself to put resources right, correct? Is this not possible?

Alexandre also need to ACK it before I dare do anything with
it.

Do we want to use the same mechanism for augmenting
bad device trees too?

What I like about it so far is the create/remove symmetry
though.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Darren Hart]

On 1/14/15 4:58 AM, Linus Walleij wrote:
> On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> 
>> This makes it possible to assign GPIOs at runtime. The
>> motivation for it is because of need to forward GPIOs from
>> one device to an other. That feature may be useful for
>> example with some mfd devices, but initially it is needed

+Samuel Ortiz for his thoughts on applicability to MFD.

>> because on some Intel Braswell based ACPI platforms the GPIO
>> resources controlling signals of the USB PHY are given to
>> the controller device object for whatever reason, so the
>> driver of that controller needs be able to pass them to the
>> PHY device somehow.
>>
>> So basically this is meant to allow patching of bad (bad
>> from Linux kernels point of view) hardware descriptions
>> provided by system fw in the drivers.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>
>> Hi,
>>
>> I'm sending this first as a RFC in case you guys have some better
>> idea how to solve our problem. I actually already have couple
>> platforms where the GPIO resources are given to the "wrong" device
>> objects now.
>>
>> Originally we were thinking about simply handling our problem with
>> hacks to the PHY drivers, basically also checking if the parent has
>> GPIOs. That would only work if the controller is always the parent,
>> which it's not, but even if it was, it would be too risky. The PHY
>> drivers don't know which controller they are attached to, so what is
>> to say that the GPIOs aren't really attached to the controller.
>>
>> So the safest way to handle our problem is to deal with it in quirks
>> in the controller drivers. Solving it by bouncing the GPIOs did not
>> feel ideal there doesn't seem to be any other way. The API is copied
>> from clkdev (clk_register_clkdev). In the end it's really only one
>> function for adding the lookups and one for removing them.
>>
>> The way it's implemented is by modifying the current style of handling
>> the lookups a little bit. The per device lookup table is basically
>> reverted back to the single linked-list format since it seems that
>> these lookups may have to be assigned from several sources.
> 
> Oh ain't that great.
> 
> So now we start patching around the kernel because the ACPI
> tables are erroneous for GPIOs. I'd like some feedback from
> Rafael &| Darren on this patch, i.e. if you two think this is a good
> way of accounting for bad GPIO descriptions in ACPI tables then
> ACK this patch.
> 
> I guess the other option would be to fix up the ACPI DSDT
> itself to put resources right, correct? Is this not possible?

This is my gut reaction as well. Heikki, why can't we correct the
firmware tables? That needs to be made clear before we start adding
hacks to the kernel.

You said, "Bad for Linux", why is this not a problem for other operating
systems?

> 
> Alexandre also need to ACK it before I dare do anything with
> it.
> 
> Do we want to use the same mechanism for augmenting
> bad device trees too?
> 
> What I like about it so far is the create/remove symmetry
> though.
> 
> Yours,
> Linus Walleij
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Darren Hart]

Ugh, Samuel actually Cc'd this time...

On 1/14/15 4:58 AM, Linus Walleij wrote:
> On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> 
>> This makes it possible to assign GPIOs at runtime. The
>> motivation for it is because of need to forward GPIOs from
>> one device to an other. That feature may be useful for
>> example with some mfd devices, but initially it is needed
>> because on some Intel Braswell based ACPI platforms the GPIO
>> resources controlling signals of the USB PHY are given to
>> the controller device object for whatever reason, so the
>> driver of that controller needs be able to pass them to the
>> PHY device somehow.
>>
>> So basically this is meant to allow patching of bad (bad
>> from Linux kernels point of view) hardware descriptions
>> provided by system fw in the drivers.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>
>> Hi,
>>
>> I'm sending this first as a RFC in case you guys have some better
>> idea how to solve our problem. I actually already have couple
>> platforms where the GPIO resources are given to the "wrong" device
>> objects now.
>>
>> Originally we were thinking about simply handling our problem with
>> hacks to the PHY drivers, basically also checking if the parent has
>> GPIOs. That would only work if the controller is always the parent,
>> which it's not, but even if it was, it would be too risky. The PHY
>> drivers don't know which controller they are attached to, so what is
>> to say that the GPIOs aren't really attached to the controller.
>>
>> So the safest way to handle our problem is to deal with it in quirks
>> in the controller drivers. Solving it by bouncing the GPIOs did not
>> feel ideal there doesn't seem to be any other way. The API is copied
>> from clkdev (clk_register_clkdev). In the end it's really only one
>> function for adding the lookups and one for removing them.
>>
>> The way it's implemented is by modifying the current style of handling
>> the lookups a little bit. The per device lookup table is basically
>> reverted back to the single linked-list format since it seems that
>> these lookups may have to be assigned from several sources.
> 
> Oh ain't that great.
> 
> So now we start patching around the kernel because the ACPI
> tables are erroneous for GPIOs. I'd like some feedback from
> Rafael &| Darren on this patch, i.e. if you two think this is a good
> way of accounting for bad GPIO descriptions in ACPI tables then
> ACK this patch.
> 
> I guess the other option would be to fix up the ACPI DSDT
> itself to put resources right, correct? Is this not possible?
> 
> Alexandre also need to ACK it before I dare do anything with
> it.
> 
> Do we want to use the same mechanism for augmenting
> bad device trees too?
> 
> What I like about it so far is the create/remove symmetry
> though.
> 
> Yours,
> Linus Walleij
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Thursday, January 08, 2015 10:25:10 AM Heikki Krogerus wrote:
> On Thu, Dec 18, 2014 at 10:23:18AM +0200, Heikki Krogerus wrote:
> > This makes it possible to assign GPIOs at runtime. The
> > motivation for it is because of need to forward GPIOs from
> > one device to an other. That feature may be useful for
> > example with some mfd devices, but initially it is needed
> > because on some Intel Braswell based ACPI platforms the GPIO
> > resources controlling signals of the USB PHY are given to
> > the controller device object for whatever reason, so the
> > driver of that controller needs be able to pass them to the
> > PHY device somehow.
> > 
> > So basically this is meant to allow patching of bad (bad
> > from Linux kernels point of view) hardware descriptions
> > provided by system fw in the drivers.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > I'm sending this first as a RFC in case you guys have some better
> > idea how to solve our problem. I actually already have couple
> > platforms where the GPIO resources are given to the "wrong" device
> > objects now.
> > 
> > Originally we were thinking about simply handling our problem with
> > hacks to the PHY drivers, basically also checking if the parent has
> > GPIOs. That would only work if the controller is always the parent,
> > which it's not, but even if it was, it would be too risky. The PHY
> > drivers don't know which controller they are attached to, so what is
> > to say that the GPIOs aren't really attached to the controller.
> > 
> > So the safest way to handle our problem is to deal with it in quirks
> > in the controller drivers. Solving it by bouncing the GPIOs did not
> > feel ideal there doesn't seem to be any other way. The API is copied
> > from clkdev (clk_register_clkdev). In the end it's really only one
> > function for adding the lookups and one for removing them.
> > 
> > The way it's implemented is by modifying the current style of handling
> > the lookups a little bit. The per device lookup table is basically
> > reverted back to the single linked-list format since it seems that
> > these lookups may have to be assigned from several sources.
> > 
> > Thanks,
> 
> Gentle ping on this.

Please CC patches involving ACPI in *any* *way* way to linux-acpi@vger.kernel.org.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Wednesday, January 14, 2015 08:32:12 AM Darren Hart wrote:
> 
> On 1/14/15 4:58 AM, Linus Walleij wrote:
> > On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > 
> >> This makes it possible to assign GPIOs at runtime. The
> >> motivation for it is because of need to forward GPIOs from
> >> one device to an other. That feature may be useful for
> >> example with some mfd devices, but initially it is needed
> 
> +Samuel Ortiz for his thoughts on applicability to MFD.
> 
> >> because on some Intel Braswell based ACPI platforms the GPIO
> >> resources controlling signals of the USB PHY are given to
> >> the controller device object for whatever reason, so the
> >> driver of that controller needs be able to pass them to the
> >> PHY device somehow.
> >>
> >> So basically this is meant to allow patching of bad (bad
> >> from Linux kernels point of view) hardware descriptions
> >> provided by system fw in the drivers.
> >>
> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >> ---
> >>
> >> Hi,
> >>
> >> I'm sending this first as a RFC in case you guys have some better
> >> idea how to solve our problem. I actually already have couple
> >> platforms where the GPIO resources are given to the "wrong" device
> >> objects now.
> >>
> >> Originally we were thinking about simply handling our problem with
> >> hacks to the PHY drivers, basically also checking if the parent has
> >> GPIOs. That would only work if the controller is always the parent,
> >> which it's not, but even if it was, it would be too risky. The PHY
> >> drivers don't know which controller they are attached to, so what is
> >> to say that the GPIOs aren't really attached to the controller.
> >>
> >> So the safest way to handle our problem is to deal with it in quirks
> >> in the controller drivers. Solving it by bouncing the GPIOs did not
> >> feel ideal there doesn't seem to be any other way. The API is copied
> >> from clkdev (clk_register_clkdev). In the end it's really only one
> >> function for adding the lookups and one for removing them.
> >>
> >> The way it's implemented is by modifying the current style of handling
> >> the lookups a little bit. The per device lookup table is basically
> >> reverted back to the single linked-list format since it seems that
> >> these lookups may have to be assigned from several sources.
> > 
> > Oh ain't that great.
> > 
> > So now we start patching around the kernel because the ACPI
> > tables are erroneous for GPIOs. I'd like some feedback from
> > Rafael &| Darren on this patch, i.e. if you two think this is a good
> > way of accounting for bad GPIO descriptions in ACPI tables then
> > ACK this patch.
> > 
> > I guess the other option would be to fix up the ACPI DSDT
> > itself to put resources right, correct? Is this not possible?
> 
> This is my gut reaction as well.

Pretty much agreed.

> Heikki, why can't we correct the firmware tables? That needs to be made clear
> before we start adding hacks to the kernel.

We need to start working in that direction really.  Fixing problems in ACPI
tables via kernel code is not going to scale sufficiently IMO.

> You said, "Bad for Linux", why is this not a problem for other operating
> systems?

Good question. :-)

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Thu, Jan 15, 2015 at 10:28:03AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, January 14, 2015 08:32:12 AM Darren Hart wrote:
> > 
> > On 1/14/15 4:58 AM, Linus Walleij wrote:
> > > On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > 
> > >> This makes it possible to assign GPIOs at runtime. The
> > >> motivation for it is because of need to forward GPIOs from
> > >> one device to an other. That feature may be useful for
> > >> example with some mfd devices, but initially it is needed
> > 
> > +Samuel Ortiz for his thoughts on applicability to MFD.
> > 
> > >> because on some Intel Braswell based ACPI platforms the GPIO
> > >> resources controlling signals of the USB PHY are given to
> > >> the controller device object for whatever reason, so the
> > >> driver of that controller needs be able to pass them to the
> > >> PHY device somehow.
> > >>
> > >> So basically this is meant to allow patching of bad (bad
> > >> from Linux kernels point of view) hardware descriptions
> > >> provided by system fw in the drivers.
> > >>
> > >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >> ---
> > >>
> > >> Hi,
> > >>
> > >> I'm sending this first as a RFC in case you guys have some better
> > >> idea how to solve our problem. I actually already have couple
> > >> platforms where the GPIO resources are given to the "wrong" device
> > >> objects now.
> > >>
> > >> Originally we were thinking about simply handling our problem with
> > >> hacks to the PHY drivers, basically also checking if the parent has
> > >> GPIOs. That would only work if the controller is always the parent,
> > >> which it's not, but even if it was, it would be too risky. The PHY
> > >> drivers don't know which controller they are attached to, so what is
> > >> to say that the GPIOs aren't really attached to the controller.
> > >>
> > >> So the safest way to handle our problem is to deal with it in quirks
> > >> in the controller drivers. Solving it by bouncing the GPIOs did not
> > >> feel ideal there doesn't seem to be any other way. The API is copied
> > >> from clkdev (clk_register_clkdev). In the end it's really only one
> > >> function for adding the lookups and one for removing them.
> > >>
> > >> The way it's implemented is by modifying the current style of handling
> > >> the lookups a little bit. The per device lookup table is basically
> > >> reverted back to the single linked-list format since it seems that
> > >> these lookups may have to be assigned from several sources.
> > > 
> > > Oh ain't that great.
> > > 
> > > So now we start patching around the kernel because the ACPI
> > > tables are erroneous for GPIOs. I'd like some feedback from
> > > Rafael &| Darren on this patch, i.e. if you two think this is a good
> > > way of accounting for bad GPIO descriptions in ACPI tables then
> > > ACK this patch.
> > > 
> > > I guess the other option would be to fix up the ACPI DSDT
> > > itself to put resources right, correct? Is this not possible?
> > 
> > This is my gut reaction as well.
> 
> Pretty much agreed.
> 
> > Heikki, why can't we correct the firmware tables? That needs to be made clear
> > before we start adding hacks to the kernel.
> 
> We need to start working in that direction really.  Fixing problems in ACPI
> tables via kernel code is not going to scale sufficiently IMO.

Fixing the DSDT produced by the firmware was my first suggesting for
this, but it just does not seem to be happening. These products are
already out on the market (this is one of them [1]) and what I have
understood is that the firmware just is what it is. It's almost as if
there is nobody even developing it for these products anymore. Even
fixes would not go in and this is not even considered a fix. Things
work just find for them with their hacked kernel.

So the firmware and the ACPI tables it provides are not going to be
fixed. What else can we do? Can we expect the users to always use
custom DSDT, or maybe somekind of custom AML snipped (is something
like that even possible), when using these products?

> > You said, "Bad for Linux", why is this not a problem for other operating
> > systems?
> 
> Good question. :-)

I would imagine certain operating systems consider a component like
PHY as something that should always be handled by the controller
driver. It does not seem to be a problem for them even if every second
product using the same SoC happens to have different PHY. All of them
will have a custom controller driver in any case. Even though the
example product below is an Android tablet, to me it looks like the
style of writing software comes straight from those "other" operation
systems.

At least it seems clear that these guys don't understand that it's not
possible to do things like write a new driver for every product using
the same SoC in Linux.

[1] http://www.trekstor.de/detail-surftabs-en/product/surftab-xintron-i-70.html

Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Jan 14, 2015 at 9:58 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Dec 18, 2014 at 9:23 AM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
>
>> This makes it possible to assign GPIOs at runtime. The
>> motivation for it is because of need to forward GPIOs from
>> one device to an other. That feature may be useful for
>> example with some mfd devices, but initially it is needed
>> because on some Intel Braswell based ACPI platforms the GPIO
>> resources controlling signals of the USB PHY are given to
>> the controller device object for whatever reason, so the
>> driver of that controller needs be able to pass them to the
>> PHY device somehow.
>>
>> So basically this is meant to allow patching of bad (bad
>> from Linux kernels point of view) hardware descriptions
>> provided by system fw in the drivers.
>>
>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> ---
>>
>> Hi,
>>
>> I'm sending this first as a RFC in case you guys have some better
>> idea how to solve our problem. I actually already have couple
>> platforms where the GPIO resources are given to the "wrong" device
>> objects now.
>>
>> Originally we were thinking about simply handling our problem with
>> hacks to the PHY drivers, basically also checking if the parent has
>> GPIOs. That would only work if the controller is always the parent,
>> which it's not, but even if it was, it would be too risky. The PHY
>> drivers don't know which controller they are attached to, so what is
>> to say that the GPIOs aren't really attached to the controller.
>>
>> So the safest way to handle our problem is to deal with it in quirks
>> in the controller drivers. Solving it by bouncing the GPIOs did not
>> feel ideal there doesn't seem to be any other way. The API is copied
>> from clkdev (clk_register_clkdev). In the end it's really only one
>> function for adding the lookups and one for removing them.
>>
>> The way it's implemented is by modifying the current style of handling
>> the lookups a little bit. The per device lookup table is basically
>> reverted back to the single linked-list format since it seems that
>> these lookups may have to be assigned from several sources.
>
> Oh ain't that great.
>
> So now we start patching around the kernel because the ACPI
> tables are erroneous for GPIOs. I'd like some feedback from
> Rafael &| Darren on this patch, i.e. if you two think this is a good
> way of accounting for bad GPIO descriptions in ACPI tables then
> ACK this patch.
>
> I guess the other option would be to fix up the ACPI DSDT
> itself to put resources right, correct? Is this not possible?
>
> Alexandre also need to ACK it before I dare do anything with
> it.

I am not really fond of this idea since it adds complexity to the
(already too complex) GPIO lookup, and only solves to a local level
(GPIO) what is a more global problem (bad ACPI tables that can affect
any subsystem).

Also I don't think any new functions is actually needed: on an ACPI
system we can safely assume that the platform lookup tables are not
used at all. So, although it is a hack as well, you can just call
gpiod_add_lookup_table() with a runtime-built table from the driver
that needs to pass the GPIO so the receipient can receive it through
the lookup table fallback gpiod_get() uses if no GPIO is defined via
ACPI.

So I think that even with the current state of the code you can
achieve what you need. Should you do it, that's another question - it
seems more to-the-point to find a way to fix/patch the ACPI tables at
runtime, if that is possible at all, to provide a more general
solution to this issue.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Mon, Jan 19, 2015 at 02:59:54PM +0900, Alexandre Courbot wrote:
> I am not really fond of this idea since it adds complexity to the
> (already too complex) GPIO lookup, and only solves to a local level
> (GPIO) what is a more global problem (bad ACPI tables that can affect
> any subsystem).
> 
> Also I don't think any new functions is actually needed: on an ACPI
> system we can safely assume that the platform lookup tables are not
> used at all. So, although it is a hack as well, you can just call
> gpiod_add_lookup_table() with a runtime-built table from the driver
> that needs to pass the GPIO so the receipient can receive it through
> the lookup table fallback gpiod_get() uses if no GPIO is defined via
> ACPI.

Unfortunately that would require us to determine details about the
gpios in the consumer driver, details like the chip_label, chip_hwnum
and flags. Stuff that I don't think the consumers should touch.

In order to dig those out of the gpio description, the driver would
either need to include linux/gpio/machine.h, linux/gpio/driver.h and
linux/gpio/consumer.h or we would need to add helper functions to
gpiolib.c that do the digging for us. Most likely it would not be
possible to avoid the helpers. We would in any case have to add
removal function for the lookups and gpiod_add_lookup_table would need
to be exported.

It simply ended up being less of a hack to add the simple function
explicitly for consumers to be used in a case like this for creating
the lookups.

Br,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:

> I am not really fond of this idea since it adds complexity to the
> (already too complex) GPIO lookup, and only solves to a local level
> (GPIO) what is a more global problem (bad ACPI tables that can affect
> any subsystem).
(...)
> it
> seems more to-the-point to find a way to fix/patch the ACPI tables at
> runtime, if that is possible at all, to provide a more general
> solution to this issue.

This is my position as well, until proven that this cannot be done.
In device tree the same mechanism is called "device tree overlays"
and I just have some vague feeling that such stuff is patched around in
some Intel platforms already, but maybe that involves replacing
the whole DSDT from userspace, surely the mechanism can be
refined?

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> 
> > I am not really fond of this idea since it adds complexity to the
> > (already too complex) GPIO lookup, and only solves to a local level
> > (GPIO) what is a more global problem (bad ACPI tables that can affect
> > any subsystem).
> (...)
> > it
> > seems more to-the-point to find a way to fix/patch the ACPI tables at
> > runtime, if that is possible at all, to provide a more general
> > solution to this issue.
> 
> This is my position as well, until proven that this cannot be done.

Well, that goes against the current practice, mind you, which *is* to put
workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
that, but it has been done for several years now.

Also someone may say to that: "Why don't *you* demonstrate that it can be done
in the first place?"  And what if it can be done, but is too complex to be
practical or similar?

My personal opinion is that having a way to apply a fix on top of broken ACPI
tables (or an extension on top of correct ones for that matter) without touching
the kernel would be very useful indeed, but making it secure may be somewhat
challenging, because in principle there's no reason why the kernel should trust
such "external" fixes.

> In device tree the same mechanism is called "device tree overlays"
> and I just have some vague feeling that such stuff is patched around in
> some Intel platforms already, but maybe that involves replacing
> the whole DSDT from userspace,

>From initramfs rather than from user space, but yes, it does.

> surely the mechanism can be refined?

Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
going to take some time.  Once we've done that, we'll see how painful it is to
"patch" ACPI tables this way in practice.

Also there is an ecosystem problem related to distributing such "patches".
Today, distributions don't need to worry about patching buggy platform
firmware, because they get workarounds in the kernel, but if we switch over
to the model in which platform firmware "overlays" need to be provided in
addition to it, then suddenly questions arise about who should be responsible
for making them available, how to avoid duplication of efforts between
distributions etc.

All of that needs to be clarified before we start making hard statements like
"No in-kernel workarounds for that!"

And, of course, there's the question of what the kernel should do if the given
firmware patch is not effective, so it doesn't really fix the problem it is
supposed to fix or it fixes that problem only partially or, worse yet, it
introuces more bugs than it fixes.  Should the kernel simply fail then (and
in what way if so) or should it try to carry out some default "sanitization"
of what the firmare (and patch) tells it and try to continue on the best
effort basis?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Jan 21, 2015 at 6:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
>> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>>
>> > I am not really fond of this idea since it adds complexity to the
>> > (already too complex) GPIO lookup, and only solves to a local level
>> > (GPIO) what is a more global problem (bad ACPI tables that can affect
>> > any subsystem).
>> (...)
>> > it
>> > seems more to-the-point to find a way to fix/patch the ACPI tables at
>> > runtime, if that is possible at all, to provide a more general
>> > solution to this issue.
>>
>> This is my position as well, until proven that this cannot be done.
>
> Well, that goes against the current practice, mind you, which *is* to put
> workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
> that, but it has been done for several years now.
>
> Also someone may say to that: "Why don't *you* demonstrate that it can be done
> in the first place?"  And what if it can be done, but is too complex to be
> practical or similar?
>
> My personal opinion is that having a way to apply a fix on top of broken ACPI
> tables (or an extension on top of correct ones for that matter) without touching
> the kernel would be very useful indeed, but making it secure may be somewhat
> challenging, because in principle there's no reason why the kernel should trust
> such "external" fixes.
>
>> In device tree the same mechanism is called "device tree overlays"
>> and I just have some vague feeling that such stuff is patched around in
>> some Intel platforms already, but maybe that involves replacing
>> the whole DSDT from userspace,
>
> From initramfs rather than from user space, but yes, it does.
>
>> surely the mechanism can be refined?
>
> Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> going to take some time.  Once we've done that, we'll see how painful it is to
> "patch" ACPI tables this way in practice.
>
> Also there is an ecosystem problem related to distributing such "patches".
> Today, distributions don't need to worry about patching buggy platform
> firmware, because they get workarounds in the kernel, but if we switch over
> to the model in which platform firmware "overlays" need to be provided in
> addition to it, then suddenly questions arise about who should be responsible
> for making them available, how to avoid duplication of efforts between
> distributions etc.
>
> All of that needs to be clarified before we start making hard statements like
> "No in-kernel workarounds for that!"
>
> And, of course, there's the question of what the kernel should do if the given
> firmware patch is not effective, so it doesn't really fix the problem it is
> supposed to fix or it fixes that problem only partially or, worse yet, it
> introuces more bugs than it fixes.  Should the kernel simply fail then (and
> in what way if so) or should it try to carry out some default "sanitization"
> of what the firmare (and patch) tells it and try to continue on the best
> effort basis?

If we decide to go ahead with the solution proposed by this patch for
practical reasons (which are good reasons indeed), I still have one
problem with its current form.

As the discussion highlighted, this is an ACPI problem, so I'd very
much like it to be confined to the ACPI GPIO code, to be enabled only
when ACPI is, and to use function names that start with acpi_gpio. The
current implementation leverages platform lookup, making said lookup
less efficient in the process and bringing confusion about its
purpose. Although the two processes are indeed similar, they are
separate things: one is a legitimate way to map GPIOs, the other is a
fixup for broken firmware.

I suppose we all agree this is a hackish fix, so let's confine it as
much as we can.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Tue, Jan 20, 2015 at 10:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> going to take some time.  Once we've done that, we'll see how painful it is to
> "patch" ACPI tables this way in practice.
>
> Also there is an ecosystem problem related to distributing such "patches".
> Today, distributions don't need to worry about patching buggy platform
> firmware, because they get workarounds in the kernel, but if we switch over
> to the model in which platform firmware "overlays" need to be provided in
> addition to it, then suddenly questions arise about who should be responsible
> for making them available, how to avoid duplication of efforts between
> distributions etc.
>
> All of that needs to be clarified before we start making hard statements like
> "No in-kernel workarounds for that!"

OK so why can't the patching happen in the kernel?

If the kernel anyway has to supply some kind of workaround for
the issue, it is more a question of where to place it. Whether it does
so by patching the ACPI tables or by detecting a bad ACPI thing
and working around it at runtime in a certain driver doesn't really
matter, does it? They are both in-kernel ACPI fixes, just that one
of the mechanisms is generic.

I don't understand why this obsession with userspace having
to do the ACPI table patching - if kernels should "just work" then
put this stuff behind Kconfig and have it in the kernel.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:
> On Tue, Jan 20, 2015 at 10:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> 
> > Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> > going to take some time.  Once we've done that, we'll see how painful it is to
> > "patch" ACPI tables this way in practice.
> >
> > Also there is an ecosystem problem related to distributing such "patches".
> > Today, distributions don't need to worry about patching buggy platform
> > firmware, because they get workarounds in the kernel, but if we switch over
> > to the model in which platform firmware "overlays" need to be provided in
> > addition to it, then suddenly questions arise about who should be responsible
> > for making them available, how to avoid duplication of efforts between
> > distributions etc.
> >
> > All of that needs to be clarified before we start making hard statements like
> > "No in-kernel workarounds for that!"
> 
> OK so why can't the patching happen in the kernel?
> 
> If the kernel anyway has to supply some kind of workaround for
> the issue, it is more a question of where to place it. Whether it does
> so by patching the ACPI tables or by detecting a bad ACPI thing
> and working around it at runtime in a certain driver doesn't really
> matter, does it?

It needs to know what to patch and how so the result is still consistent.

How do you think the kernel is going to figure that out?

> They are both in-kernel ACPI fixes, just that one
> of the mechanisms is generic.

I'm not following you here, sorry.

> I don't understand why this obsession with userspace having
> to do the ACPI table patching - if kernels should "just work" then
> put this stuff behind Kconfig and have it in the kernel.

This is not an obsession and your suggestion here leads to having custom
per-board kernels which is not supportable in the long term.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> On Wed, Jan 21, 2015 at 6:25 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, January 20, 2015 01:16:06 PM Linus Walleij wrote:
> >> On Mon, Jan 19, 2015 at 6:59 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >>
> >> > I am not really fond of this idea since it adds complexity to the
> >> > (already too complex) GPIO lookup, and only solves to a local level
> >> > (GPIO) what is a more global problem (bad ACPI tables that can affect
> >> > any subsystem).
> >> (...)
> >> > it
> >> > seems more to-the-point to find a way to fix/patch the ACPI tables at
> >> > runtime, if that is possible at all, to provide a more general
> >> > solution to this issue.
> >>
> >> This is my position as well, until proven that this cannot be done.
> >
> > Well, that goes against the current practice, mind you, which *is* to put
> > workarounds for buggy ACPI tables into the kernel.  I'm not going to defend
> > that, but it has been done for several years now.
> >
> > Also someone may say to that: "Why don't *you* demonstrate that it can be done
> > in the first place?"  And what if it can be done, but is too complex to be
> > practical or similar?
> >
> > My personal opinion is that having a way to apply a fix on top of broken ACPI
> > tables (or an extension on top of correct ones for that matter) without touching
> > the kernel would be very useful indeed, but making it secure may be somewhat
> > challenging, because in principle there's no reason why the kernel should trust
> > such "external" fixes.
> >
> >> In device tree the same mechanism is called "device tree overlays"
> >> and I just have some vague feeling that such stuff is patched around in
> >> some Intel platforms already, but maybe that involves replacing
> >> the whole DSDT from userspace,
> >
> > From initramfs rather than from user space, but yes, it does.
> >
> >> surely the mechanism can be refined?
> >
> > Yes, it can (in principle).  In fact, we have a plan to refine it, but it is
> > going to take some time.  Once we've done that, we'll see how painful it is to
> > "patch" ACPI tables this way in practice.
> >
> > Also there is an ecosystem problem related to distributing such "patches".
> > Today, distributions don't need to worry about patching buggy platform
> > firmware, because they get workarounds in the kernel, but if we switch over
> > to the model in which platform firmware "overlays" need to be provided in
> > addition to it, then suddenly questions arise about who should be responsible
> > for making them available, how to avoid duplication of efforts between
> > distributions etc.
> >
> > All of that needs to be clarified before we start making hard statements like
> > "No in-kernel workarounds for that!"
> >
> > And, of course, there's the question of what the kernel should do if the given
> > firmware patch is not effective, so it doesn't really fix the problem it is
> > supposed to fix or it fixes that problem only partially or, worse yet, it
> > introuces more bugs than it fixes.  Should the kernel simply fail then (and
> > in what way if so) or should it try to carry out some default "sanitization"
> > of what the firmare (and patch) tells it and try to continue on the best
> > effort basis?
> 
> If we decide to go ahead with the solution proposed by this patch for
> practical reasons (which are good reasons indeed), I still have one
> problem with its current form.
> 
> As the discussion highlighted, this is an ACPI problem, so I'd very
> much like it to be confined to the ACPI GPIO code, to be enabled only
> when ACPI is, and to use function names that start with acpi_gpio.

I can agree with that.

> The current implementation leverages platform lookup, making said lookup
> less efficient in the process and bringing confusion about its
> purpose. Although the two processes are indeed similar, they are
> separate things: one is a legitimate way to map GPIOs, the other is a
> fixup for broken firmware.
> 
> I suppose we all agree this is a hackish fix, so let's confine it as
> much as we can.

OK

Heikki, any comments?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Hi guys,

On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > If we decide to go ahead with the solution proposed by this patch for
> > practical reasons (which are good reasons indeed), I still have one
> > problem with its current form.
> > 
> > As the discussion highlighted, this is an ACPI problem, so I'd very
> > much like it to be confined to the ACPI GPIO code, to be enabled only
> > when ACPI is, and to use function names that start with acpi_gpio.
> 
> I can agree with that.
> 
> > The current implementation leverages platform lookup, making said lookup
> > less efficient in the process and bringing confusion about its
> > purpose. Although the two processes are indeed similar, they are
> > separate things: one is a legitimate way to map GPIOs, the other is a
> > fixup for broken firmware.
> > 
> > I suppose we all agree this is a hackish fix, so let's confine it as
> > much as we can.
> 
> OK
> 
> Heikki, any comments?

I'm fine with that.

That actually makes me think that we could then drop the lookup tables
completely and use device properties instead with the help of "generic
property" (attached):

We would just need to agree on the format how to describe a gpio
property, document it and of course convert the current users as
usual. The format could be something like this as an example (I'm
writing this out of my head so don't shoot me if you can see it would
not work. Just an example):

static const u32 example_gpio[] = { <gpio>, <flags>, };

static struct dev_gen_prop example_prop[] =
        {
                .type = DEV_PROP_U32,
                .name = "gpio,<con_id>",
                .nval = 2,
                .num = &example_gpio,
        },
        { },
};

static struct platform_device example_pdev = {
        ...
        .dev = {
                .gen_prop = &example_prop,
        },
}


In gpiolib.c we would then, instead of going through the lookups,
simply ask for that property:

        ...
        sprintf(propname, "gpio,%s", con_id);
        device_property_read_u32_array(dev, propname, &val, 2);
        ...
        desc = gpio_to_desc(val[0]);
        flags = val[1];
        ...


So this is just and idea. I think it would be relatively easy to
implement. What do you guys think?


Cheers,

-- 
heikki

[-- Attachment #2: 0001-driver-core-property-support-for-generic-property.patch --]
[-- Type: text/plain, Size: 7947 bytes --]

>From bb1146d8ca7c39386dfc53043051d389cba49cbe Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Wed, 7 Jan 2015 10:57:44 +0200
Subject: [PATCH] driver core: property: support for generic property

This extends the unified device property interface by adding
"Generic Property" to it for cases where device tree or ACPI
are not being used.

That makes the unified device property interface cover also
most of the cases where information is extracted from custom
platform_data in the drivers. So if before we had to check
separately is there custom platform_data for a driver:

	if (pdata)
		bar = pdata->bar;
	else
		device_property_read_u32(dev, "foo", &bar);

we can now drop that check and simply always use the unified
device property interface.

That makes it possible to drop a lot of boiler plate from
the drivers, plus quite a few header files under
include/linux/ describing those driver specific platform
data structures can be removed.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c  | 135 ++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/device.h   |   3 ++
 include/linux/property.h |  17 ++++++
 3 files changed, 143 insertions(+), 12 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c458458..4ea6d27 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -15,6 +15,108 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 
+static struct dev_gen_prop *dev_prop_get(struct device *dev, const char *name)
+{
+	struct dev_gen_prop *prop;
+
+	if (!dev->gen_prop)
+		return NULL;
+
+	for (prop = dev->gen_prop; prop->name; prop++)
+		if (!strcmp(name, prop->name))
+			return prop;
+	return NULL;
+}
+
+static int dev_prop_copy_array_u8(u8 *src, u8 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u16(u16 *src, u16 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u32(u32 *src, u32 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_u64(u64 *src, u64 *val, size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_copy_array_string(const char **src, const char **val,
+				      size_t nval)
+{
+	int i;
+
+	for (i = 0; i < nval; i++)
+		val[i] = src[i];
+	return 0;
+}
+
+static int dev_prop_read_array(struct device *dev, const char *name,
+			       enum dev_prop_type type, void *val, size_t nval)
+{
+	struct dev_gen_prop *prop;
+	int ret = 0;
+
+	prop = dev_prop_get(dev, name);
+	if (!prop)
+		return -ENODATA;
+
+	if (prop->type != type)
+		return -EPROTO;
+
+	if (prop->nval < nval)
+		return -EOVERFLOW;
+
+	switch (prop->type) {
+	case DEV_PROP_U8:
+		ret = dev_prop_copy_array_u8((u8 *)prop->num, (u8 *)val,
+					     nval);
+		break;
+	case DEV_PROP_U16:
+		ret = dev_prop_copy_array_u16((u16 *)prop->num, (u16 *)val,
+					      nval);
+		break;
+	case DEV_PROP_U32:
+		ret = dev_prop_copy_array_u32((u32 *)prop->num, (u32 *)val,
+					      nval);
+		break;
+	case DEV_PROP_U64:
+		ret = dev_prop_copy_array_u64(prop->num, (u64 *)val, nval);
+		break;
+	case DEV_PROP_STRING:
+		ret = dev_prop_copy_array_string(prop->str,
+						 (const char **)val, nval);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
 /**
  * device_property_present - check if a property of a device is present
  * @dev: Device whose property is being checked
@@ -26,8 +128,9 @@ bool device_property_present(struct device *dev, const char *propname)
 {
 	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
 		return of_property_read_bool(dev->of_node, propname);
-
-	return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
+	if (ACPI_HANDLE(dev))
+		return !acpi_dev_prop_get(ACPI_COMPANION(dev), propname, NULL);
+	return !!dev_prop_get(dev, propname);
 }
 EXPORT_SYMBOL_GPL(device_property_present);
 
@@ -55,8 +158,11 @@ EXPORT_SYMBOL_GPL(fwnode_property_present);
 	IS_ENABLED(CONFIG_OF) && _dev_->of_node ? \
 		(OF_DEV_PROP_READ_ARRAY(_dev_->of_node, _propname_, _type_, \
 					_val_, _nval_)) : \
-		acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
-				   _proptype_, _val_, _nval_)
+		ACPI_HANDLE(_dev_) ? \
+			acpi_dev_prop_read(ACPI_COMPANION(_dev_), _propname_, \
+					   _proptype_, _val_, _nval_) : \
+			dev_prop_read_array(_dev_, _propname_, _proptype_, \
+					   _val_, _nval_)
 
 /**
  * device_property_read_u8_array - return a u8 array property of a device
@@ -169,10 +275,13 @@ EXPORT_SYMBOL_GPL(device_property_read_u64_array);
 int device_property_read_string_array(struct device *dev, const char *propname,
 				      const char **val, size_t nval)
 {
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		of_property_read_string_array(dev->of_node, propname, val, nval) :
-		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
-				   DEV_PROP_STRING, val, nval);
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_property_read_string_array(dev->of_node, propname,
+						     val, nval);
+	if (ACPI_HANDLE(dev))
+		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
+					  DEV_PROP_STRING, val, nval);
+	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, nval);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string_array);
 
@@ -193,10 +302,12 @@ EXPORT_SYMBOL_GPL(device_property_read_string_array);
 int device_property_read_string(struct device *dev, const char *propname,
 				const char **val)
 {
-	return IS_ENABLED(CONFIG_OF) && dev->of_node ?
-		of_property_read_string(dev->of_node, propname, val) :
-		acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
-				   DEV_PROP_STRING, val, 1);
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_property_read_string(dev->of_node, propname, val);
+	if (ACPI_HANDLE(dev))
+		return acpi_dev_prop_read(ACPI_COMPANION(dev), propname,
+					  DEV_PROP_STRING, val, 1);
+	return dev_prop_read_array(dev, propname, DEV_PROP_STRING, val, 1);
 }
 EXPORT_SYMBOL_GPL(device_property_read_string);
 
diff --git a/include/linux/device.h b/include/linux/device.h
index fb50673..738dc25 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -27,6 +27,7 @@
 #include <linux/ratelimit.h>
 #include <linux/uidgid.h>
 #include <linux/gfp.h>
+#include <linux/property.h>
 #include <asm/device.h>
 
 struct device;
@@ -704,6 +705,7 @@ struct acpi_dev_node {
  * @archdata:	For arch-specific additions.
  * @of_node:	Associated device tree node.
  * @acpi_node:	Associated ACPI device node.
+ * @gen_prop:	Generic device property
  * @devt:	For creating the sysfs "dev".
  * @id:		device instance
  * @devres_lock: Spinlock to protect the resource of the device.
@@ -780,6 +782,7 @@ struct device {
 
 	struct device_node	*of_node; /* associated device tree node */
 	struct acpi_dev_node	acpi_node; /* associated ACPI device node */
+	struct dev_gen_prop	*gen_prop; /* generic device property */
 
 	dev_t			devt;	/* dev_t, creates the sysfs "dev" */
 	u32			id;	/* device instance */
diff --git a/include/linux/property.h b/include/linux/property.h
index a6a3d98..44e875f 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -26,6 +26,23 @@ enum dev_prop_type {
 	DEV_PROP_MAX,
 };
 
+/**
+ * struct dev_gen_prop - Generic Device Property
+ * @name: property name
+ * @nval: number of elements in the array
+ * @str: value array of strings
+ * @num: value array of numbers
+ *
+ * Used when of_node and acpi_node are missing.
+ */
+struct dev_gen_prop {
+	enum dev_prop_type type;
+	const char *name;
+	size_t nval;
+	const char **str;
+	u64 *num;
+};
+
 bool device_property_present(struct device *dev, const char *propname);
 int device_property_read_u8_array(struct device *dev, const char *propname,
 				  u8 *val, size_t nval);
-- 
2.1.4

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Friday, January 23, 2015 01:21:22 PM Heikki Krogerus wrote:
> 
> --Nq2Wo0NMKNjxTN9z
> Content-Type: text/plain; charset=iso-8859-1
> Content-Disposition: inline
> Content-Transfer-Encoding: 8bit
> 
> Hi guys,
> 
> On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > > If we decide to go ahead with the solution proposed by this patch for
> > > practical reasons (which are good reasons indeed), I still have one
> > > problem with its current form.
> > > 
> > > As the discussion highlighted, this is an ACPI problem, so I'd very
> > > much like it to be confined to the ACPI GPIO code, to be enabled only
> > > when ACPI is, and to use function names that start with acpi_gpio.
> > 
> > I can agree with that.
> > 
> > > The current implementation leverages platform lookup, making said lookup
> > > less efficient in the process and bringing confusion about its
> > > purpose. Although the two processes are indeed similar, they are
> > > separate things: one is a legitimate way to map GPIOs, the other is a
> > > fixup for broken firmware.
> > > 
> > > I suppose we all agree this is a hackish fix, so let's confine it as
> > > much as we can.
> > 
> > OK
> > 
> > Heikki, any comments?
> 
> I'm fine with that.
> 
> That actually makes me think that we could then drop the lookup tables
> completely and use device properties instead with the help of "generic
> property" (attached):

Which reminds me that I've lost track of this one.

Can you please resend it and CC something like linux-acpi?

Also I'm not sure what you mean by "drop the lookup tables completely".

> We would just need to agree on the format how to describe a gpio
> property, document it and of course convert the current users as
> usual. The format could be something like this as an example (I'm
> writing this out of my head so don't shoot me if you can see it would
> not work. Just an example):
> 
> static const u32 example_gpio[] = { <gpio>, <flags>, };
> 
> static struct dev_gen_prop example_prop[] =
>         {
>                 .type = DEV_PROP_U32,
>                 .name = "gpio,<con_id>",
>                 .nval = 2,
>                 .num = &example_gpio,
>         },
>         { },
> };
> 
> static struct platform_device example_pdev = {
>         ...
>         .dev = {
>                 .gen_prop = &example_prop,
>         },
> }
> 
> 
> In gpiolib.c we would then, instead of going through the lookups,
> simply ask for that property:
> 
>         ...
>         sprintf(propname, "gpio,%s", con_id);
>         device_property_read_u32_array(dev, propname, &val, 2);
>         ...
>         desc = gpio_to_desc(val[0]);
>         flags = val[1];
>         ...
> 
> 
> So this is just and idea. I think it would be relatively easy to
> implement. What do you guys think?

Well, I need some time to think about that.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Fri, Jan 23, 2015 at 04:14:13PM +0100, Rafael J. Wysocki wrote:
> > That actually makes me think that we could then drop the lookup tables
> > completely and use device properties instead with the help of "generic
> > property" (attached):
> 
> Which reminds me that I've lost track of this one.
> 
> Can you please resend it and CC something like linux-acpi?

OK, I'll resend it.

> Also I'm not sure what you mean by "drop the lookup tables completely".

So I meant removing struct gpio_lookup and struct gpio_lookup_table.

> > We would just need to agree on the format how to describe a gpio
> > property, document it and of course convert the current users as
> > usual. The format could be something like this as an example (I'm
> > writing this out of my head so don't shoot me if you can see it would
> > not work. Just an example):
> > 
> > static const u32 example_gpio[] = { <gpio>, <flags>, };
> > 
> > static struct dev_gen_prop example_prop[] =
> >         {
> >                 .type = DEV_PROP_U32,
> >                 .name = "gpio,<con_id>",
> >                 .nval = 2,
> >                 .num = &example_gpio,
> >         },
> >         { },
> > };
> > 
> > static struct platform_device example_pdev = {
> >         ...
> >         .dev = {
> >                 .gen_prop = &example_prop,
> >         },
> > }
> > 
> > 
> > In gpiolib.c we would then, instead of going through the lookups,
> > simply ask for that property:
> > 
> >         ...
> >         sprintf(propname, "gpio,%s", con_id);
> >         device_property_read_u32_array(dev, propname, &val, 2);
> >         ...
> >         desc = gpio_to_desc(val[0]);
> >         flags = val[1];
> >         ...
> > 
> > 
> > So this is just and idea. I think it would be relatively easy to
> > implement. What do you guys think?
> 
> Well, I need some time to think about that.


Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Thu, Jan 22, 2015 at 5:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:

>> If the kernel anyway has to supply some kind of workaround for
>> the issue, it is more a question of where to place it. Whether it does
>> so by patching the ACPI tables or by detecting a bad ACPI thing
>> and working around it at runtime in a certain driver doesn't really
>> matter, does it?
>
> It needs to know what to patch and how so the result is still consistent.
>
> How do you think the kernel is going to figure that out?

Isn't every DSDT (etc) unique? So you could detect one by making
a checksum of the binary or something.

And then you'd know that the table with this checksum needs
patching?

>> They are both in-kernel ACPI fixes, just that one
>> of the mechanisms is generic.
>
> I'm not following you here, sorry.

As per above? Fix the firmware table, not work around the
fact that it is buggy.

>> I don't understand why this obsession with userspace having
>> to do the ACPI table patching - if kernels should "just work" then
>> put this stuff behind Kconfig and have it in the kernel.
>
> This is not an obsession and your suggestion here leads to having custom
> per-board kernels which is not supportable in the long term.

Not at all. The other suggestion leads to having custom per-board
fixes in every driver for which broken ACPI descriptions exists,
which is just as bad, isn't it? Instead of collecting the problem
in one place (patch any broken ACPI table) it is distrubuted across
N drivers, where each and every one has to detect that it is
being malinformed by a broken ACPI table.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> On Thu, Jan 22, 2015 at 5:12 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, January 22, 2015 09:17:38 AM Linus Walleij wrote:
> 
> >> If the kernel anyway has to supply some kind of workaround for
> >> the issue, it is more a question of where to place it. Whether it does
> >> so by patching the ACPI tables or by detecting a bad ACPI thing
> >> and working around it at runtime in a certain driver doesn't really
> >> matter, does it?
> >
> > It needs to know what to patch and how so the result is still consistent.
> >
> > How do you think the kernel is going to figure that out?
> 
> Isn't every DSDT (etc) unique?

Formally, it doesn't have to be.

And it doesn't have to be a DSDT, it may be an SSDT too or even a plain
table that's buggy.

> So you could detect one by making a checksum of the binary or something.
> 
> And then you'd know that the table with this checksum needs patching?

At a single table level it is generally difficult to say whether or not
things are going to work.

What needs to work is the namespace which is built from all of the tables
provided combined.  So the namespace needs to be populated first and then
fixes applied on top of that (presumably by deleting, adding or replacing
objects).

Now, in theory, you *may* be able to figure out that combination of tables
A produces namespace B which then will require fix X if the system is Y,
but quite frankly I wouldn't count on that.

Moreover, fixups (or "patches" as I called them, but that wasn't exactly
correct) need to be provided in the form of AML definition blocks to apply on
top of an already populated namespace and if you want to use a binary kernel image,
you can't really afford putting all that stuff for all systems it can possibly
run on into it.  This means that distros need to be able to combine a fixup for
the ACPI tables with the binary kernel and install the result into the system's
boot medium (whatever it is).  Also it should be possible to update the fixup
and the kernel image separately if necessary.

Now from the kernel's perspective that raises the question: "What if the
ACPI tables fixup provided by the distro is not sufficient?"

That needs to be addressed somehow in the code.

> >> They are both in-kernel ACPI fixes, just that one
> >> of the mechanisms is generic.
> >
> > I'm not following you here, sorry.
> 
> As per above? Fix the firmware table, not work around the
> fact that it is buggy.
> 
> >> I don't understand why this obsession with userspace having
> >> to do the ACPI table patching - if kernels should "just work" then
> >> put this stuff behind Kconfig and have it in the kernel.
> >
> > This is not an obsession and your suggestion here leads to having custom
> > per-board kernels which is not supportable in the long term.
> 
> Not at all. The other suggestion leads to having custom per-board
> fixes in every driver for which broken ACPI descriptions exists,

So I'm not arguing for that.

> which is just as bad, isn't it? Instead of collecting the problem
> in one place (patch any broken ACPI table) it is distrubuted across
> N drivers, where each and every one has to detect that it is
> being malinformed by a broken ACPI table.

In the general-purpose binary kernel image distribution model drivers generally
need to treat information provided by the platform firmware, be it ACPI or a DT
or what-not, as untrusted input that needs to be validated if possible.  That
is not always possible and in those cases we have no choice but to try to use
that information, fingers crossed.  Sometimes we can validate it, though, and
then we should and do something if it turns out to be invalid.

Overall, my view on that is that (1) there needs to be a way for a distro to
provide an ACPI tables fixup for the kernel to apply on top of the already
populated ACPI namespace on a given system and (2) drivers need to be careful
about using firmware data and possibly be able to recover from errors in there.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:

>> So you could detect one by making a checksum of the binary or something.
>>
>> And then you'd know that the table with this checksum needs patching?
>
> At a single table level it is generally difficult to say whether or not
> things are going to work.
>
> What needs to work is the namespace which is built from all of the tables
> provided combined.  So the namespace needs to be populated first and then
> fixes applied on top of that (presumably by deleting, adding or replacing
> objects).
>
> Now, in theory, you *may* be able to figure out that combination of tables
> A produces namespace B which then will require fix X if the system is Y,
> but quite frankly I wouldn't count on that.
>
> Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> correct) need to be provided in the form of AML definition blocks to apply on
> top of an already populated namespace and if you want to use a binary kernel image,
> you can't really afford putting all that stuff for all systems it can possibly
> run on into it.  This means that distros need to be able to combine a fixup for
> the ACPI tables with the binary kernel and install the result into the system's
> boot medium (whatever it is).  Also it should be possible to update the fixup
> and the kernel image separately if necessary.
>
> Now from the kernel's perspective that raises the question: "What if the
> ACPI tables fixup provided by the distro is not sufficient?"
>
> That needs to be addressed somehow in the code.

Yeah I guess I'm convinced that we need to handle this particular
weirdness in the gpio-acpi code... if it can be contained there as
expressed by Alexandre.

Yours,
Linus Walleij

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> 
> >> So you could detect one by making a checksum of the binary or something.
> >>
> >> And then you'd know that the table with this checksum needs patching?
> >
> > At a single table level it is generally difficult to say whether or not
> > things are going to work.
> >
> > What needs to work is the namespace which is built from all of the tables
> > provided combined.  So the namespace needs to be populated first and then
> > fixes applied on top of that (presumably by deleting, adding or replacing
> > objects).
> >
> > Now, in theory, you *may* be able to figure out that combination of tables
> > A produces namespace B which then will require fix X if the system is Y,
> > but quite frankly I wouldn't count on that.
> >
> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> > correct) need to be provided in the form of AML definition blocks to apply on
> > top of an already populated namespace and if you want to use a binary kernel image,
> > you can't really afford putting all that stuff for all systems it can possibly
> > run on into it.  This means that distros need to be able to combine a fixup for
> > the ACPI tables with the binary kernel and install the result into the system's
> > boot medium (whatever it is).  Also it should be possible to update the fixup
> > and the kernel image separately if necessary.
> >
> > Now from the kernel's perspective that raises the question: "What if the
> > ACPI tables fixup provided by the distro is not sufficient?"
> >
> > That needs to be addressed somehow in the code.
> 
> Yeah I guess I'm convinced that we need to handle this particular
> weirdness in the gpio-acpi code... if it can be contained there as
> expressed by Alexandre.

I'm still fine if we want to confine this "gpio forwarding" to acpi
if you guys want it, but I was looking at the Sound SoC drivers and I
realised that we do have places which pass gpio descriptors to other
devices in platform data. And these of course aren't always used on
acpi platforms. By greping gpio_desc I saw at least these files are
passing it in platform data structures:

include/sound/soc.h
include/linux/leds.h
include/linux/usb/usb_phy_generic.h

There are probable others but I checked those. And of course now there
is nothing preventing people from adding more of them.

So we could use my original idea with them. I think most of the
drivers using those are tied to separate handling for dt and pdata
only because of the way the gpio descriptor is delivered to them.

What do you guys think?


Cheers,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> Hi guys,
>
> On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
>> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
>> > If we decide to go ahead with the solution proposed by this patch for
>> > practical reasons (which are good reasons indeed), I still have one
>> > problem with its current form.
>> >
>> > As the discussion highlighted, this is an ACPI problem, so I'd very
>> > much like it to be confined to the ACPI GPIO code, to be enabled only
>> > when ACPI is, and to use function names that start with acpi_gpio.
>>
>> I can agree with that.
>>
>> > The current implementation leverages platform lookup, making said lookup
>> > less efficient in the process and bringing confusion about its
>> > purpose. Although the two processes are indeed similar, they are
>> > separate things: one is a legitimate way to map GPIOs, the other is a
>> > fixup for broken firmware.
>> >
>> > I suppose we all agree this is a hackish fix, so let's confine it as
>> > much as we can.
>>
>> OK
>>
>> Heikki, any comments?
>
> I'm fine with that.
>
> That actually makes me think that we could then drop the lookup tables
> completely and use device properties instead with the help of "generic
> property" (attached):
>
> We would just need to agree on the format how to describe a gpio
> property, document it and of course convert the current users as
> usual. The format could be something like this as an example (I'm
> writing this out of my head so don't shoot me if you can see it would
> not work. Just an example):
>
> static const u32 example_gpio[] = { <gpio>, <flags>,爙;
>
> static struct dev_gen_prop example_prop[] =
>         {
>                 .type = DEV_PROP_U32,
>                 .name = "gpio,<con_id>",
>                 .nval = 2,
>                 .num = &example_gpio,
>         },
>         { },
> };
>
> static struct platform_device example_pdev = {
>         ...
>         .dev = {
>                 .gen_prop = &example_prop,
>         },
> }
>
>
> In gpiolib.c we would then, instead of going through the lookups,
> simply ask for that property:
>
>         ...
>         sprintf(propname, "gpio,%s", con_id);
>         device_property_read_u32_array(dev, propname, &val, 2);
>         ...
>         desc = gpio_to_desc(val[0]);
>         flags = val[1];
>         ...
>
>
> So this is just and idea. I think it would be relatively easy to
> implement. What do you guys think?

At first sight, that looks like a very good idea and a great use of
the device properties API. Are you willing to explore it further?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
>> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
>>
>> >> So you could detect one by making a checksum of the binary or something.
>> >>
>> >> And then you'd know that the table with this checksum needs patching?
>> >
>> > At a single table level it is generally difficult to say whether or not
>> > things are going to work.
>> >
>> > What needs to work is the namespace which is built from all of the tables
>> > provided combined.  So the namespace needs to be populated first and then
>> > fixes applied on top of that (presumably by deleting, adding or replacing
>> > objects).
>> >
>> > Now, in theory, you *may* be able to figure out that combination of tables
>> > A produces namespace B which then will require fix X if the system is Y,
>> > but quite frankly I wouldn't count on that.
>> >
>> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
>> > correct) need to be provided in the form of AML definition blocks to apply on
>> > top of an already populated namespace and if you want to use a binary kernel image,
>> > you can't really afford putting all that stuff for all systems it can possibly
>> > run on into it.  This means that distros need to be able to combine a fixup for
>> > the ACPI tables with the binary kernel and install the result into the system's
>> > boot medium (whatever it is).  Also it should be possible to update the fixup
>> > and the kernel image separately if necessary.
>> >
>> > Now from the kernel's perspective that raises the question: "What if the
>> > ACPI tables fixup provided by the distro is not sufficient?"
>> >
>> > That needs to be addressed somehow in the code.
>>
>> Yeah I guess I'm convinced that we need to handle this particular
>> weirdness in the gpio-acpi code... if it can be contained there as
>> expressed by Alexandre.
>
> I'm still fine if we want to confine this "gpio forwarding" to acpi
> if you guys want it, but I was looking at the Sound SoC drivers and I
> realised that we do have places which pass gpio descriptors to other
> devices in platform data. And these of course aren't always used on
> acpi platforms. By greping gpio_desc I saw at least these files are
> passing it in platform data structures:
>
> include/sound/soc.h
> include/linux/leds.h
> include/linux/usb/usb_phy_generic.h
>
> There are probable others but I checked those. And of course now there
> is nothing preventing people from adding more of them.

For sound/soc.h, the member is indeed public but I don't see it being
used to pass descriptors around between drivers.

For linux/leds.h, I think this is the reason why we introduced
devm_get_gpiod_from_child()

These looks more like a bad usage of GPIO descriptors, but AFAICT they
can be fixed by fixing the drivers and, by all means, this is where we
should do it.

This is a different situation from yours where we are trying to deal
with broken firmware and need to resort to tricks at one point or the
other.

Or am I missing your point?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Rafael J. Wysocki]

On Tuesday, February 10, 2015 06:32:46 PM Alexandre Courbot wrote:
> On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > Hi guys,
> >
> > On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> >> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> >> > If we decide to go ahead with the solution proposed by this patch for
> >> > practical reasons (which are good reasons indeed), I still have one
> >> > problem with its current form.
> >> >
> >> > As the discussion highlighted, this is an ACPI problem, so I'd very
> >> > much like it to be confined to the ACPI GPIO code, to be enabled only
> >> > when ACPI is, and to use function names that start with acpi_gpio.
> >>
> >> I can agree with that.
> >>
> >> > The current implementation leverages platform lookup, making said lookup
> >> > less efficient in the process and bringing confusion about its
> >> > purpose. Although the two processes are indeed similar, they are
> >> > separate things: one is a legitimate way to map GPIOs, the other is a
> >> > fixup for broken firmware.
> >> >
> >> > I suppose we all agree this is a hackish fix, so let's confine it as
> >> > much as we can.
> >>
> >> OK
> >>
> >> Heikki, any comments?
> >
> > I'm fine with that.
> >
> > That actually makes me think that we could then drop the lookup tables
> > completely and use device properties instead with the help of "generic
> > property" (attached):
> >
> > We would just need to agree on the format how to describe a gpio
> > property, document it and of course convert the current users as
> > usual. The format could be something like this as an example (I'm
> > writing this out of my head so don't shoot me if you can see it would
> > not work. Just an example):
> >
> > static const u32 example_gpio[] = { <gpio>, <flags>,爙;
> >
> > static struct dev_gen_prop example_prop[] =
> >         {
> >                 .type = DEV_PROP_U32,
> >                 .name = "gpio,<con_id>",
> >                 .nval = 2,
> >                 .num = &example_gpio,
> >         },
> >         { },
> > };
> >
> > static struct platform_device example_pdev = {
> >         ...
> >         .dev = {
> >                 .gen_prop = &example_prop,
> >         },
> > }
> >
> >
> > In gpiolib.c we would then, instead of going through the lookups,
> > simply ask for that property:
> >
> >         ...
> >         sprintf(propname, "gpio,%s", con_id);
> >         device_property_read_u32_array(dev, propname, &val, 2);
> >         ...
> >         desc = gpio_to_desc(val[0]);
> >         flags = val[1];
> >         ...
> >
> >
> > So this is just and idea. I think it would be relatively easy to
> > implement. What do you guys think?
> 
> At first sight, that looks like a very good idea and a great use of
> the device properties API. Are you willing to explore it further?

I guess Heikki is waiting for my feedback on his core patch that I'm
going to provide later today.

And yes, this is an interesting idea.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Tue, Feb 10, 2015 at 06:44:34PM +0900, Alexandre Courbot wrote:
> On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> > On Wed, Feb 04, 2015 at 10:51:27AM +0100, Linus Walleij wrote:
> >> On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >> > On Friday, January 30, 2015 03:48:30 PM Linus Walleij wrote:
> >>
> >> >> So you could detect one by making a checksum of the binary or something.
> >> >>
> >> >> And then you'd know that the table with this checksum needs patching?
> >> >
> >> > At a single table level it is generally difficult to say whether or not
> >> > things are going to work.
> >> >
> >> > What needs to work is the namespace which is built from all of the tables
> >> > provided combined.  So the namespace needs to be populated first and then
> >> > fixes applied on top of that (presumably by deleting, adding or replacing
> >> > objects).
> >> >
> >> > Now, in theory, you *may* be able to figure out that combination of tables
> >> > A produces namespace B which then will require fix X if the system is Y,
> >> > but quite frankly I wouldn't count on that.
> >> >
> >> > Moreover, fixups (or "patches" as I called them, but that wasn't exactly
> >> > correct) need to be provided in the form of AML definition blocks to apply on
> >> > top of an already populated namespace and if you want to use a binary kernel image,
> >> > you can't really afford putting all that stuff for all systems it can possibly
> >> > run on into it.  This means that distros need to be able to combine a fixup for
> >> > the ACPI tables with the binary kernel and install the result into the system's
> >> > boot medium (whatever it is).  Also it should be possible to update the fixup
> >> > and the kernel image separately if necessary.
> >> >
> >> > Now from the kernel's perspective that raises the question: "What if the
> >> > ACPI tables fixup provided by the distro is not sufficient?"
> >> >
> >> > That needs to be addressed somehow in the code.
> >>
> >> Yeah I guess I'm convinced that we need to handle this particular
> >> weirdness in the gpio-acpi code... if it can be contained there as
> >> expressed by Alexandre.
> >
> > I'm still fine if we want to confine this "gpio forwarding" to acpi
> > if you guys want it, but I was looking at the Sound SoC drivers and I
> > realised that we do have places which pass gpio descriptors to other
> > devices in platform data. And these of course aren't always used on
> > acpi platforms. By greping gpio_desc I saw at least these files are
> > passing it in platform data structures:
> >
> > include/sound/soc.h
> > include/linux/leds.h
> > include/linux/usb/usb_phy_generic.h
> >
> > There are probable others but I checked those. And of course now there
> > is nothing preventing people from adding more of them.
> 
> For sound/soc.h, the member is indeed public but I don't see it being
> used to pass descriptors around between drivers.
> 
> For linux/leds.h, I think this is the reason why we introduced
> devm_get_gpiod_from_child()
> 
> These looks more like a bad usage of GPIO descriptors, but AFAICT they
> can be fixed by fixing the drivers and, by all means, this is where we
> should do it.
> 
> This is a different situation from yours where we are trying to deal
> with broken firmware and need to resort to tricks at one point or the
> other.
> 
> Or am I missing your point?

Well, in this case the motivation would be to make it possible to live
without platform data with these drivers, but in the end the situation
would be exactly the same. We need to give a gpio descriptor to
some other device in a driver. So the goal would be that in the actual
consumer driver of the gpio we could request it always in one way,
ideally always with gpiod_get() call.

But if there is no real need in other places for this thing, then
let's forget about it.


Thanks,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Heikki Krogerus]

On Tue, Feb 10, 2015 at 04:10:04PM +0100, Rafael J. Wysocki wrote:
> On Tuesday, February 10, 2015 06:32:46 PM Alexandre Courbot wrote:
> > On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > > Hi guys,
> > >
> > > On Thu, Jan 22, 2015 at 05:14:22PM +0100, Rafael J. Wysocki wrote:
> > >> On Thursday, January 22, 2015 11:57:55 AM Alexandre Courbot wrote:
> > >> > If we decide to go ahead with the solution proposed by this patch for
> > >> > practical reasons (which are good reasons indeed), I still have one
> > >> > problem with its current form.
> > >> >
> > >> > As the discussion highlighted, this is an ACPI problem, so I'd very
> > >> > much like it to be confined to the ACPI GPIO code, to be enabled only
> > >> > when ACPI is, and to use function names that start with acpi_gpio.
> > >>
> > >> I can agree with that.
> > >>
> > >> > The current implementation leverages platform lookup, making said lookup
> > >> > less efficient in the process and bringing confusion about its
> > >> > purpose. Although the two processes are indeed similar, they are
> > >> > separate things: one is a legitimate way to map GPIOs, the other is a
> > >> > fixup for broken firmware.
> > >> >
> > >> > I suppose we all agree this is a hackish fix, so let's confine it as
> > >> > much as we can.
> > >>
> > >> OK
> > >>
> > >> Heikki, any comments?
> > >
> > > I'm fine with that.
> > >
> > > That actually makes me think that we could then drop the lookup tables
> > > completely and use device properties instead with the help of "generic
> > > property" (attached):
> > >
> > > We would just need to agree on the format how to describe a gpio
> > > property, document it and of course convert the current users as
> > > usual. The format could be something like this as an example (I'm
> > > writing this out of my head so don't shoot me if you can see it would
> > > not work. Just an example):
> > >
> > > static const u32 example_gpio[] = { <gpio>, <flags>,爙;
> > >
> > > static struct dev_gen_prop example_prop[] =
> > >         {
> > >                 .type = DEV_PROP_U32,
> > >                 .name = "gpio,<con_id>",
> > >                 .nval = 2,
> > >                 .num = &example_gpio,
> > >         },
> > >         { },
> > > };
> > >
> > > static struct platform_device example_pdev = {
> > >         ...
> > >         .dev = {
> > >                 .gen_prop = &example_prop,
> > >         },
> > > }
> > >
> > >
> > > In gpiolib.c we would then, instead of going through the lookups,
> > > simply ask for that property:
> > >
> > >         ...
> > >         sprintf(propname, "gpio,%s", con_id);
> > >         device_property_read_u32_array(dev, propname, &val, 2);
> > >         ...
> > >         desc = gpio_to_desc(val[0]);
> > >         flags = val[1];
> > >         ...
> > >
> > >
> > > So this is just and idea. I think it would be relatively easy to
> > > implement. What do you guys think?
> > 
> > At first sight, that looks like a very good idea and a great use of
> > the device properties API. Are you willing to explore it further?

Yes. If I get green light for the generic property idea, I can start
thinking about this.


Thanks,

-- 
heikki

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

Hi,

> If we decide to go ahead with the solution proposed by this patch for
> practical reasons (which are good reasons indeed), I still have one
> problem with its current form.
> 
> As the discussion highlighted, this is an ACPI problem, so I'd very
> much like it to be confined to the ACPI GPIO code, to be enabled only
> when ACPI is, and to use function names that start with acpi_gpio. The
> current implementation leverages platform lookup, making said lookup
> less efficient in the process and bringing confusion about its
> purpose. Although the two processes are indeed similar, they are
> separate things: one is a legitimate way to map GPIOs, the other is a
> fixup for broken firmware.
> 
> I suppose we all agree this is a hackish fix, so let's confine it as
> much as we can.

Are we considering MFD cases hackish as well?
i.e. if we have a driver that needs to register children devices and this
driver needs to pass GPIO to a child.

Br, David

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Alexandre Courbot]

On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> Hi,
>
>> If we decide to go ahead with the solution proposed by this patch for
>> practical reasons (which are good reasons indeed), I still have one
>> problem with its current form.
>>
>> As the discussion highlighted, this is an ACPI problem, so I'd very
>> much like it to be confined to the ACPI GPIO code, to be enabled only
>> when ACPI is, and to use function names that start with acpi_gpio. The
>> current implementation leverages platform lookup, making said lookup
>> less efficient in the process and bringing confusion about its
>> purpose. Although the two processes are indeed similar, they are
>> separate things: one is a legitimate way to map GPIOs, the other is a
>> fixup for broken firmware.
>>
>> I suppose we all agree this is a hackish fix, so let's confine it as
>> much as we can.
>
> Are we considering MFD cases hackish as well?
> i.e. if we have a driver that needs to register children devices and this
> driver needs to pass GPIO to a child.

In that case wouldn't the GPIO be best defined in the child node
itself, for the child device's driver to directly probe?

Re: [RFC PATCH] gpio: support for GPIO forwarding

[David Cohen]

On Wed, Feb 25, 2015 at 10:34:45AM +0900, Alexandre Courbot wrote:
> On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> > Hi,
> >
> >> If we decide to go ahead with the solution proposed by this patch for
> >> practical reasons (which are good reasons indeed), I still have one
> >> problem with its current form.
> >>
> >> As the discussion highlighted, this is an ACPI problem, so I'd very
> >> much like it to be confined to the ACPI GPIO code, to be enabled only
> >> when ACPI is, and to use function names that start with acpi_gpio. The
> >> current implementation leverages platform lookup, making said lookup
> >> less efficient in the process and bringing confusion about its
> >> purpose. Although the two processes are indeed similar, they are
> >> separate things: one is a legitimate way to map GPIOs, the other is a
> >> fixup for broken firmware.
> >>
> >> I suppose we all agree this is a hackish fix, so let's confine it as
> >> much as we can.
> >
> > Are we considering MFD cases hackish as well?
> > i.e. if we have a driver that needs to register children devices and this
> > driver needs to pass GPIO to a child.
> 
> In that case wouldn't the GPIO be best defined in the child node
> itself, for the child device's driver to directly probe?

In my case [1] I need 2 "virtual devices" (and more in future) to be
part of an USB OTG port control. I call it virtual because they are too
simple components connected to no bus and controlled by GPIOs:
- a fixed regulator controlled by GPIO
- a generic mux controlled by GPIO

I'd need to request official ACPI HID for them in order to make them
self-sufficient.

I can go ahead with this approach, but we have many examples of drivers
on upstream that are platform driver expecting to receive gpio via
platform data (e.g. extcon-gpio). The ACPI table of some products on
market were defined following this concept and won't change anymore.

Br, David

[1] https://lkml.org/lkml/2015/2/19/411

Re: [RFC PATCH] gpio: support for GPIO forwarding

[Linus Walleij]

On Wed, Feb 25, 2015 at 7:25 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> In my case [1] I need 2 "virtual devices" (and more in future) to be
> part of an USB OTG port control. I call it virtual because they are too
> simple components connected to no bus and controlled by GPIOs:
> - a fixed regulator controlled by GPIO
> - a generic mux controlled by GPIO
>
> I'd need to request official ACPI HID for them in order to make them
> self-sufficient.
>
> I can go ahead with this approach, but we have many examples of drivers
> on upstream that are platform driver expecting to receive gpio via
> platform data (e.g. extcon-gpio). The ACPI table of some products on
> market were defined following this concept and won't change anymore.

So it's not just going to be GPIOs I take it?

There is going to be regulator forwarding, clock forwarding, pin control
forwarding, IRQ forwarding and DMA channel forwarding etc at the end
of the day?

I think it's strange that we see this so much, is the real problem that
ACPI and the kernel have different ideas of what constitutes a device?
And how come the DT seems to be a much better fit and not experience
this? Because we haven't had to deal with deployed device trees with
resources (GPIOs, regulators, etc) bound to some complex MFD device?

Yours,
Linus Walleij