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 <[email protected]>
---
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
On Fri, Jan 30, 2015 at 5:17 PM, Rafael J. Wysocki <[email protected]> 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
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 <[email protected]> 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
On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
<[email protected]> 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?
On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
<[email protected]> 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 <[email protected]> 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?
On Tuesday, February 10, 2015 06:32:46 PM Alexandre Courbot wrote:
> On Fri, Jan 23, 2015 at 8:21 PM, Heikki Krogerus
> <[email protected]> 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.
On Tue, Feb 10, 2015 at 06:44:34PM +0900, Alexandre Courbot wrote:
> On Wed, Feb 4, 2015 at 11:11 PM, Heikki Krogerus
> <[email protected]> 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 <[email protected]> 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
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
> > <[email protected]> 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
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
On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
<[email protected]> 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?
On Wed, Feb 25, 2015 at 10:34:45AM +0900, Alexandre Courbot wrote:
> On Wed, Feb 25, 2015 at 5:34 AM, David Cohen
> <[email protected]> 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