On Fri, Oct 30, 2015 at 09:33:28AM -0700, Dmitry Torokhov wrote: > On Mon, Oct 19, 2015 at 05:52:39PM +0300, mika.westerberg@xxxxxxxxxxxxxxx wrote: > > On Mon, Oct 19, 2015 at 02:32:24PM +0000, Tirdea, Irina wrote: > > > > > > > > > > -----Original Message----- > > > > From: linux-input-owner@xxxxxxxxxxxxxxx [mailto:linux-input-owner@xxxxxxxxxxxxxxx] On Behalf Of > > > > mika.westerberg@xxxxxxxxxxxxxxx > > > > Sent: 14 October, 2015 16:44 > > > > To: Dmitry Torokhov > > > > Cc: Tirdea, Irina; Bastien Nocera; Aleksei Mamlin; Karsten Merker; linux-input@xxxxxxxxxxxxxxx; Mark Rutland; Purdila, Octavian; linux- > > > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > > > > Subject: Re: [PATCH v9 2/9] Input: goodix - reset device at init > > > > > > > > On Wed, Oct 14, 2015 at 02:18:20PM +0300, mika.westerberg@xxxxxxxxxxxxxxx wrote: > > > > > On Tue, Oct 13, 2015 at 11:23:03PM -0700, Dmitry Torokhov wrote: > > > > > > I understand why one might use acpi_dev_add_driver_gpios() to augment > > > > > > data in ACPI, however here we have completely different issue: driver > > > > > > that expects named gpios gets returned gpio that has nothing to do with > > > > > > what it requested, because gpiolib acpi code always falls back to > > > > > > unnamed gpio if it does not find named gpio. That can be acceptable if > > > > > > driver uses the same con_id for all requests to gpiolib, but is not > > > > > > working when driver supplies different con_ids. > > > > > > > > > > Right, the ACPI fallback ignores con_id completely and uses only the > > > > > index. > > > > > > > > > > AFAIK there is only one driver using ACPI _CRS index method: > > > > > sdhci-[acpi|pci].c. If we can convert that to use acpi_dev_add_driver_gpios() > > > > > to feed names for card detection GPIOs, I think we can remove the > > > > > fallback alltogether in favor of named GPIOs for ACPI. > > > > > > > > Nah, there seems to be several drivers relying on this already :-/ > > > > > > Would it be possible to add an optional parameter to the GPIO API > > > to specify whether we want to fall back to indexed GPIOs for ACPI? > > > > I don't think it's a good idea to add ACPI specifics to generic APIs. > > > > I went through ACPI enabled drivers calling GPIO APIs and majority of > > them are doing this: > > > > static int stk8312_gpio_probe(struct i2c_client *client) > > { > > struct device *dev; > > struct gpio_desc *gpio; > > int ret; > > > > if (!client) > > return -EINVAL; > > > > dev = &client->dev; > > > > /* data ready gpio interrupt pin */ > > gpio = devm_gpiod_get_index(dev, STK8312_GPIO, 0, GPIOD_IN); > > if (IS_ERR(gpio)) { > > dev_err(dev, "acpi gpio get index failed\n"); > > return PTR_ERR(gpio); > > } > > > > ret = gpiod_to_irq(gpio); > > dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret); > > > > return ret; > > } > > > > We can drop all this because I2C core already handles GpioInt -> interrupt > > number translation. > > > > Few drivers are doing something more complex but I think we can still convert > > them to use acpi_dev_add_driver_gpios() and eventually get rid of the whole > > _CRS index lookup. > > cpi_dev_add_driver_gpios() does not really help with generic drivers > (unless we keep adding more and more board specific data to them). How > about we keep track of names used and only allow conversion for the > first name used, like in the patch below? > > -- > Dmitry > > gpiolib: tighten up ACPI legacy gpio lookups > > From: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > > We should not fall back to the legacy unnamed gpio lookup style if the > driver requests gpios with different names, because we'll give out the same > gpio twice. Let's keep track of the names that were used for the device and > only do the fallback for the first name used. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> > --- > drivers/gpio/gpiolib.c | 42 +++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 5db3445..4ae5447 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1738,6 +1738,45 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, > return desc; > } > > +struct acpi_gpio_lookup { > + struct list_head node; > + struct device *dev; > + const char *con_id; > +}; > + > +static DEFINE_MUTEX(acpi_gpio_lookup_lock); > +static LIST_HEAD(acpi_gpio_lookup_list); > + > +static bool acpi_can_fallback_crs(struct device *dev, const char *con_id) > +{ > + struct acpi_gpio_lookup *l, *lookup = NULL; > + > + mutex_lock(&acpi_gpio_lookup_lock); > + > + list_for_each_entry(l, &acpi_gpio_lookup_list, node) { > + if (l->dev == dev) { > + lookup = l; > + break; > + } > + } > + > + if (!lookup) { > + lookup = kmalloc(sizeof(*lookup), GFP_KERNEL); > + if (lookup) { > + lookup->dev = dev; > + lookup->con_id = con_id; > + list_add_tail(&lookup->node, &acpi_gpio_lookup_list); > + } > + } > + > + mutex_lock(&acpi_gpio_lookup_lock); This should of course be mutex_unlock() according to the awesome 0-day build bot. > + > + return lookup && > + ((!lookup->con_id && !con_id) || > + (lookup->con_id && con_id && > + strcmp(lookup->con_id, con_id) == 0)); > +} > + > static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id, > unsigned int idx, > enum gpio_lookup_flags *flags) > @@ -1765,7 +1804,8 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id, > > /* Then from plain _CRS GPIOs */ > if (IS_ERR(desc)) { > - desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info); > + if (acpi_can_fallback_crs(dev, con_id)) > + desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info); > if (IS_ERR(desc)) > return desc; > } -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
