| Message ID | 20161209103522.3833-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 09, 2016 at 11:35:20AM +0100, Hans de Goede wrote: > ACPI gpios may return -EBUSY this means that the gpio is owned by the > ACPI code, and will be set / cleared as needed by the ACPI code. > > Treat gpiod_get returning -EBUSY as not having a gpio, fixing the > driver not loading on tablets where this happens. Hmm, I'd say ACPI should not be exposing existence of GPIO to the drivers if it decides to manage it itself. Can we hide this in gpiolib ACPI code? > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/input/touchscreen/silead.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c > index f502c84..4387cd8 100644 > --- a/drivers/input/touchscreen/silead.c > +++ b/drivers/input/touchscreen/silead.c > @@ -467,6 +467,14 @@ static int silead_ts_probe(struct i2c_client *client, > > /* Power GPIO pin */ > data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); > +#ifdef CONFIG_ACPI > + /* > + * ACPI gpios may return -EBUSY this means that the gpio is owned by > + * the ACPI code, and will be set / cleared by the ACPI code. > + */ > + if (IS_ERR(data->gpio_power) && PTR_ERR(data->gpio_power) == -EBUSY) > + data->gpio_power = NULL; > +#endif > if (IS_ERR(data->gpio_power)) { > if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) > dev_err(dev, "Shutdown GPIO request failed\n"); > -- > 2.9.3 > Thanks.
Hi, On 27-12-16 23:14, Dmitry Torokhov wrote: > On Fri, Dec 09, 2016 at 11:35:20AM +0100, Hans de Goede wrote: >> ACPI gpios may return -EBUSY this means that the gpio is owned by the >> ACPI code, and will be set / cleared as needed by the ACPI code. >> >> Treat gpiod_get returning -EBUSY as not having a gpio, fixing the >> driver not loading on tablets where this happens. > > Hmm, I'd say ACPI should not be exposing existence of GPIO to the > drivers if it decides to manage it itself. Can we hide this in gpiolib > ACPI code? I don't think we really can, the -EBUSY comes from the chipset driver, where there is a special bit in the gpio-cfg reg signalling whether the gpio is owned by the host or by the firmware. So only the chipset specific implementation knows about this. And for other drivers not being able to access the gpio is not acceptable. We could simply make the gpio completely optional by doing: data->gpio_power = devm_gpiod_get(dev, "power", GPIOD_OUT_LOW); if (IS_ERR(data->gpio_power)) { error = PTR_ERR(data->gpio_power); if (error == -EPROBE_DEFER) return error; dev_info(dev, "Not using power gpio: %d\n", error); data->gpio_power = NULL; } Then we don't end up ugly-fying the silead.c code... Regards, Hans > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/input/touchscreen/silead.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c >> index f502c84..4387cd8 100644 >> --- a/drivers/input/touchscreen/silead.c >> +++ b/drivers/input/touchscreen/silead.c >> @@ -467,6 +467,14 @@ static int silead_ts_probe(struct i2c_client *client, >> >> /* Power GPIO pin */ >> data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); >> +#ifdef CONFIG_ACPI >> + /* >> + * ACPI gpios may return -EBUSY this means that the gpio is owned by >> + * the ACPI code, and will be set / cleared by the ACPI code. >> + */ >> + if (IS_ERR(data->gpio_power) && PTR_ERR(data->gpio_power) == -EBUSY) >> + data->gpio_power = NULL; >> +#endif >> if (IS_ERR(data->gpio_power)) { >> if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) >> dev_err(dev, "Shutdown GPIO request failed\n"); >> -- >> 2.9.3 >> > > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> We could simply make the gpio completely optional by doing: > > Then we don't end up ugly-fying the silead.c code... I remember vaguely that this will make the driver useless in many cases. Some DSDTs I've seen have ACPI PM functions that handle the GPIO lines, and I tried calling them instead of managing the GPIO directly. This did not appear to work, but maybe I did it wrong. See here: https://github.com/onitake/gslx680-acpi/blob/master/gslx680_ts_acpi.c#L571 And this is what's supposed to be called: https://github.com/onitake/gslx680-acpi/blob/master/acpi/gsl-dsdt.aml#L12 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 31-12-16 18:06, Gregor Riepl wrote: >> We could simply make the gpio completely optional by doing: >> >> Then we don't end up ugly-fying the silead.c code... > > I remember vaguely that this will make the driver useless in many cases. > > Some DSDTs I've seen have ACPI PM functions that handle the GPIO lines, and I tried calling them instead of managing the GPIO directly. This did not appear to work, but maybe I did it wrong. > > See here: https://github.com/onitake/gslx680-acpi/blob/master/gslx680_ts_acpi.c#L571 Hmm, I've just written (and deleted) a blurb about how the platform-bus takes care of power states for us. But this is an i2c driver, so that is not relevant. For i2c drivers we should indeed do this ourselves. I'll write a patch based on the code blurb you pointed to and test that on the 2 different model cherrytrail (x86) tablets I've access to. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c index f502c84..4387cd8 100644 --- a/drivers/input/touchscreen/silead.c +++ b/drivers/input/touchscreen/silead.c @@ -467,6 +467,14 @@ static int silead_ts_probe(struct i2c_client *client, /* Power GPIO pin */ data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); +#ifdef CONFIG_ACPI + /* + * ACPI gpios may return -EBUSY this means that the gpio is owned by + * the ACPI code, and will be set / cleared by the ACPI code. + */ + if (IS_ERR(data->gpio_power) && PTR_ERR(data->gpio_power) == -EBUSY) + data->gpio_power = NULL; +#endif if (IS_ERR(data->gpio_power)) { if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) dev_err(dev, "Shutdown GPIO request failed\n");
ACPI gpios may return -EBUSY this means that the gpio is owned by the ACPI code, and will be set / cleared as needed by the ACPI code. Treat gpiod_get returning -EBUSY as not having a gpio, fixing the driver not loading on tablets where this happens. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/input/touchscreen/silead.c | 8 ++++++++ 1 file changed, 8 insertions(+)