| Message ID | 20170122200008.27027-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: > On some x86 tablets we cannot directly access the GPIOs as they are > claimed by the ACPI tables, so check it the i2c client is not being > power-managed by ACPI before trying to get the power pin GPIO. Why do we even get this GPIO if driver is not supposed to be using it? I'd much rather gpio provider hid it from the driver instead of every driver having this check. Thanks. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v2: > -Check acpi_bus_power_manageable() instead of trying to directly > control the acpi power level ourselves > --- > drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------ > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c > index 404830a..2fbcd7f 100644 > --- a/drivers/input/touchscreen/silead.c > +++ b/drivers/input/touchscreen/silead.c > @@ -31,6 +31,7 @@ > #include <linux/irq.h> > #include <linux/regulator/consumer.h> > > +#include <acpi/acpi_bus.h> > #include <asm/unaligned.h> > > #define SILEAD_TS_NAME "silead_ts" > @@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client, > if (error) > return error; > > - /* Power GPIO pin */ > - data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); > - if (IS_ERR(data->gpio_power)) { > - if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) > - dev_err(dev, "Shutdown GPIO request failed\n"); > - return PTR_ERR(data->gpio_power); > + /* > + * If device power is not managed by ACPI, get the power_gpio > + * and manage it ourselves. > + */ > +#ifdef CONFIG_ACPI > + if (!acpi_bus_power_manageable(ACPI_HANDLE(dev))) > +#endif > + { > + data->gpio_power = devm_gpiod_get_optional(dev, "power", > + GPIOD_OUT_LOW); > + if (IS_ERR(data->gpio_power)) { > + if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) > + dev_err(dev, "Power GPIO request failed\n"); > + return PTR_ERR(data->gpio_power); > + } > } > > error = silead_ts_setup(client); > -- > 2.9.3 >
Hi, On 22-01-17 23:20, Dmitry Torokhov wrote: > On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: >> On some x86 tablets we cannot directly access the GPIOs as they are >> claimed by the ACPI tables, so check it the i2c client is not being >> power-managed by ACPI before trying to get the power pin GPIO. > > Why do we even get this GPIO if driver is not supposed to be using it? > I'd much rather gpio provider hid it from the driver instead of every > driver having this check. The problem is that the gpio subsys does not really know about ACPI managed GPIOs the way this works is that the firmware sets a special "reserved for firmware use" bit in the gpio control register and directly bit-bangs the gpio control register when it wants to toggle the gpio. So there is no awareness of these gpios being reserved (as gpios) at the ACPI level AFAICT. The hardware specific low-level gpio chip driver checks this bit when we request the gpio and returns -EBUSY. Regards, Hans > > Thanks. > >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v2: >> -Check acpi_bus_power_manageable() instead of trying to directly >> control the acpi power level ourselves >> --- >> drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------ >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c >> index 404830a..2fbcd7f 100644 >> --- a/drivers/input/touchscreen/silead.c >> +++ b/drivers/input/touchscreen/silead.c >> @@ -31,6 +31,7 @@ >> #include <linux/irq.h> >> #include <linux/regulator/consumer.h> >> >> +#include <acpi/acpi_bus.h> >> #include <asm/unaligned.h> >> >> #define SILEAD_TS_NAME "silead_ts" >> @@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client, >> if (error) >> return error; >> >> - /* Power GPIO pin */ >> - data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); >> - if (IS_ERR(data->gpio_power)) { >> - if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) >> - dev_err(dev, "Shutdown GPIO request failed\n"); >> - return PTR_ERR(data->gpio_power); >> + /* >> + * If device power is not managed by ACPI, get the power_gpio >> + * and manage it ourselves. >> + */ >> +#ifdef CONFIG_ACPI >> + if (!acpi_bus_power_manageable(ACPI_HANDLE(dev))) >> +#endif >> + { >> + data->gpio_power = devm_gpiod_get_optional(dev, "power", >> + GPIOD_OUT_LOW); >> + if (IS_ERR(data->gpio_power)) { >> + if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) >> + dev_err(dev, "Power GPIO request failed\n"); >> + return PTR_ERR(data->gpio_power); >> + } >> } >> >> error = silead_ts_setup(client); >> -- >> 2.9.3 >> > -- 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
On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote: > Hi, > > On 22-01-17 23:20, Dmitry Torokhov wrote: > >On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: > >>On some x86 tablets we cannot directly access the GPIOs as they are > >>claimed by the ACPI tables, so check it the i2c client is not being > >>power-managed by ACPI before trying to get the power pin GPIO. > > > >Why do we even get this GPIO if driver is not supposed to be using it? > >I'd much rather gpio provider hid it from the driver instead of every > >driver having this check. > > The problem is that the gpio subsys does not really know about ACPI > managed GPIOs the way this works is that the firmware sets a special > "reserved for firmware use" bit in the gpio control register and > directly bit-bangs the gpio control register when it wants to toggle > the gpio. So there is no awareness of these gpios being reserved > (as gpios) at the ACPI level AFAICT. > > The hardware specific low-level gpio chip driver checks this bit > when we request the gpio and returns -EBUSY. I'd say that, if GPIOs are reserved for firmware use, and kernel should not or can not touch them, then they should not be visible, if not to the gpio core, then to consumers for sure. Let's add Mika and Linus. Thanks. > > Regards, > > Hans > > > > > > > > > >Thanks. > > > >> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >>--- > >>Changes in v2: > >>-Check acpi_bus_power_manageable() instead of trying to directly > >> control the acpi power level ourselves > >>--- > >> drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------ > >> 1 file changed, 16 insertions(+), 6 deletions(-) > >> > >>diff --git a/drivers/input/touchscreen/silead.c b/drivers/input/touchscreen/silead.c > >>index 404830a..2fbcd7f 100644 > >>--- a/drivers/input/touchscreen/silead.c > >>+++ b/drivers/input/touchscreen/silead.c > >>@@ -31,6 +31,7 @@ > >> #include <linux/irq.h> > >> #include <linux/regulator/consumer.h> > >> > >>+#include <acpi/acpi_bus.h> > >> #include <asm/unaligned.h> > >> > >> #define SILEAD_TS_NAME "silead_ts" > >>@@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client, > >> if (error) > >> return error; > >> > >>- /* Power GPIO pin */ > >>- data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); > >>- if (IS_ERR(data->gpio_power)) { > >>- if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) > >>- dev_err(dev, "Shutdown GPIO request failed\n"); > >>- return PTR_ERR(data->gpio_power); > >>+ /* > >>+ * If device power is not managed by ACPI, get the power_gpio > >>+ * and manage it ourselves. > >>+ */ > >>+#ifdef CONFIG_ACPI > >>+ if (!acpi_bus_power_manageable(ACPI_HANDLE(dev))) > >>+#endif > >>+ { > >>+ data->gpio_power = devm_gpiod_get_optional(dev, "power", > >>+ GPIOD_OUT_LOW); > >>+ if (IS_ERR(data->gpio_power)) { > >>+ if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) > >>+ dev_err(dev, "Power GPIO request failed\n"); > >>+ return PTR_ERR(data->gpio_power); > >>+ } > >> } > >> > >> error = silead_ts_setup(client); > >>-- > >>2.9.3 > >> > >
On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote: > On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote: > > Hi, > > > > On 22-01-17 23:20, Dmitry Torokhov wrote: > > >On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: > > >>On some x86 tablets we cannot directly access the GPIOs as they are > > >>claimed by the ACPI tables, so check it the i2c client is not being > > >>power-managed by ACPI before trying to get the power pin GPIO. > > > > > >Why do we even get this GPIO if driver is not supposed to be using it? > > >I'd much rather gpio provider hid it from the driver instead of every > > >driver having this check. > > > > The problem is that the gpio subsys does not really know about ACPI > > managed GPIOs the way this works is that the firmware sets a special > > "reserved for firmware use" bit in the gpio control register and > > directly bit-bangs the gpio control register when it wants to toggle > > the gpio. So there is no awareness of these gpios being reserved > > (as gpios) at the ACPI level AFAICT. > > > > The hardware specific low-level gpio chip driver checks this bit > > when we request the gpio and returns -EBUSY. > > I'd say that, if GPIOs are reserved for firmware use, and kernel should > not or can not touch them, then they should not be visible, if not to > the gpio core, then to consumers for sure. > > Let's add Mika and Linus. It is not always possible for the GPIO driver to find out if a certain GPIO is reserved for the firmware use or not. I don't think we have any API in gpiolib that allows excluding certain GPIOs though. What we do for example with the ACPI OpRegion GPIOs is that gpiolib reserves those automatically thus preventing any consumer from using those. -- 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 02-02-17 11:41, Mika Westerberg wrote: > On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote: >> On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote: >>> Hi, >>> >>> On 22-01-17 23:20, Dmitry Torokhov wrote: >>>> On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: >>>>> On some x86 tablets we cannot directly access the GPIOs as they are >>>>> claimed by the ACPI tables, so check it the i2c client is not being >>>>> power-managed by ACPI before trying to get the power pin GPIO. >>>> >>>> Why do we even get this GPIO if driver is not supposed to be using it? >>>> I'd much rather gpio provider hid it from the driver instead of every >>>> driver having this check. >>> >>> The problem is that the gpio subsys does not really know about ACPI >>> managed GPIOs the way this works is that the firmware sets a special >>> "reserved for firmware use" bit in the gpio control register and >>> directly bit-bangs the gpio control register when it wants to toggle >>> the gpio. So there is no awareness of these gpios being reserved >>> (as gpios) at the ACPI level AFAICT. >>> >>> The hardware specific low-level gpio chip driver checks this bit >>> when we request the gpio and returns -EBUSY. >> >> I'd say that, if GPIOs are reserved for firmware use, and kernel should >> not or can not touch them, then they should not be visible, if not to >> the gpio core, then to consumers for sure. >> >> Let's add Mika and Linus. > > It is not always possible for the GPIO driver to find out if a certain > GPIO is reserved for the firmware use or not. I don't think we have any > API in gpiolib that allows excluding certain GPIOs though. > > What we do for example with the ACPI OpRegion GPIOs is that gpiolib > reserves those automatically thus preventing any consumer from using > those. Right, but that would result in a -EBUSY error from gpiod_get, so iow gpiod_get_optional would still fail and not just return NULL as it would if the requested gpio does not exist? IOW even if that was happening here, we would still need to handle either -EBUSY and treat it as gpiod_get_optional returning NULL, or we need the acpi_bus_power_manageable check the patch from this thread is adding, right ? 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
On Thu, Feb 02, 2017 at 12:57:05PM +0100, Hans de Goede wrote: > Hi, > > On 02-02-17 11:41, Mika Westerberg wrote: > > On Wed, Feb 01, 2017 at 09:42:57AM -0800, Dmitry Torokhov wrote: > > > On Mon, Jan 23, 2017 at 11:05:14AM +0100, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 22-01-17 23:20, Dmitry Torokhov wrote: > > > > > On Sun, Jan 22, 2017 at 09:00:08PM +0100, Hans de Goede wrote: > > > > > > On some x86 tablets we cannot directly access the GPIOs as they are > > > > > > claimed by the ACPI tables, so check it the i2c client is not being > > > > > > power-managed by ACPI before trying to get the power pin GPIO. > > > > > > > > > > Why do we even get this GPIO if driver is not supposed to be using it? > > > > > I'd much rather gpio provider hid it from the driver instead of every > > > > > driver having this check. > > > > > > > > The problem is that the gpio subsys does not really know about ACPI > > > > managed GPIOs the way this works is that the firmware sets a special > > > > "reserved for firmware use" bit in the gpio control register and > > > > directly bit-bangs the gpio control register when it wants to toggle > > > > the gpio. So there is no awareness of these gpios being reserved > > > > (as gpios) at the ACPI level AFAICT. > > > > > > > > The hardware specific low-level gpio chip driver checks this bit > > > > when we request the gpio and returns -EBUSY. > > > > > > I'd say that, if GPIOs are reserved for firmware use, and kernel should > > > not or can not touch them, then they should not be visible, if not to > > > the gpio core, then to consumers for sure. > > > > > > Let's add Mika and Linus. > > > > It is not always possible for the GPIO driver to find out if a certain > > GPIO is reserved for the firmware use or not. I don't think we have any > > API in gpiolib that allows excluding certain GPIOs though. > > > > What we do for example with the ACPI OpRegion GPIOs is that gpiolib > > reserves those automatically thus preventing any consumer from using > > those. > > Right, but that would result in a -EBUSY error from gpiod_get, so > iow gpiod_get_optional would still fail and not just return NULL as it > would if the requested gpio does not exist? Yes, that's my understanding as well. > IOW even if that was happening here, we would still need to handle > either -EBUSY and treat it as gpiod_get_optional returning NULL, or > we need the acpi_bus_power_manageable check the patch from this > thread is adding, right ? I do not have a copy of the patch in this thread but sounds like something that might work. -- 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
On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: > I do not have a copy of the patch in this thread but sounds like > something that might work. Actually, I seem have a copy of that patch. So you are saying that the device has a power GPIO in ACPI _CRS but it should not be used for some reason? -- 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 02-02-17 13:32, Mika Westerberg wrote: > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: >> I do not have a copy of the patch in this thread but sounds like >> something that might work. > > Actually, I seem have a copy of that patch. > > So you are saying that the device has a power GPIO in ACPI _CRS but it > should not be used for some reason? Yes, it should not be used as it is controlled by the _PS0 and _PS3 functions. It is reserved for firmware use by setting a special bit indicating this in the gpio pin's control register in the intel cherrytrail gpio controller. So arguably it should not be in _CRS at all, since the OS is supposed to not touch it, yet it is there: Device (TCS4) { Name (_ADR, Zero) // _ADR: Address Name (_HID, "MSSL1680") // _HID: Hardware ID Name (_CID, "PNP1680") // _CID: Compatible ID Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_DDN, "MSSL1680") // _DDN: DOS Device Name <snip> Method (_PS3, 0, Serialized) // _PS3: Power State 3 { If (^^^^GPO1.AVBL == One) { ^^^^GPO1.TCTL = One } Sleep (0x78) } Method (_PS0, 0, Serialized) // _PS0: Power State 0 { If (^^^^GPO1.AVBL == One) { ^^^^GPO1.TCTL = One } Sleep (0x05) If (^^^^GPO1.AVBL == One) { ^^^^GPO1.TCTL = Zero } Sleep (0x1E) If (^^^^GPO1.AVBL == One) { ^^^^GPO1.TCTL = Zero } Sleep (0x78) } Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti { Name (WBUF, ResourceTemplate () { I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\\_SB.PCI0.I2C6", 0x00, ResourceConsumer, , Exclusive, ) GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri "\\_SB.GPO1", 0x00, ResourceConsumer, , ) { // Pin list 0x0019 } GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000, "\\_SB.GPO1", 0x00, ResourceConsumer, , ) { // Pin list 0x0013 } }) Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ } The setting of the special bit in the gpio control register leads to drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() returning -EBUSY, which in return makes gpiod_get_optional return -EBUSY for this pin, rather then NULL (as we would like). 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
On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: > Hi, > > On 02-02-17 13:32, Mika Westerberg wrote: > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: > > > I do not have a copy of the patch in this thread but sounds like > > > something that might work. > > > > Actually, I seem have a copy of that patch. > > > > So you are saying that the device has a power GPIO in ACPI _CRS but it > > should not be used for some reason? > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti > { > Name (WBUF, ResourceTemplate () > { > I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, > AddressingMode7Bit, "\\_SB.PCI0.I2C6", > 0x00, ResourceConsumer, , Exclusive, > ) > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri > "\\_SB.GPO1", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0019 > } > GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000, > "\\_SB.GPO1", 0x00, ResourceConsumer, , > ) > { // Pin list > 0x0013 > } > }) > Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ > } > > The setting of the special bit in the gpio control register leads to > drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() > returning -EBUSY, which in return makes gpiod_get_optional > return -EBUSY for this pin, rather then NULL (as we would like). Actually what is wrong here is that your gpiod_get(dev, "power") falls back to use plain indexes and returns the first GPIO even though it should not as the driver specifically requests GPIO with name "power" and there is no _DSD. Andy (Cc'd) has a patch that tries to make the fallback mechanism more stricter which should in theory fix the problem as well. The patch series is here: https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master -- 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 02-02-17 14:12, Mika Westerberg wrote: > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: >> Hi, >> >> On 02-02-17 13:32, Mika Westerberg wrote: >>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: >>>> I do not have a copy of the patch in this thread but sounds like >>>> something that might work. >>> >>> Actually, I seem have a copy of that patch. >>> >>> So you are saying that the device has a power GPIO in ACPI _CRS but it >>> should not be used for some reason? >> >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti >> { >> Name (WBUF, ResourceTemplate () >> { >> I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, >> AddressingMode7Bit, "\\_SB.PCI0.I2C6", >> 0x00, ResourceConsumer, , Exclusive, >> ) >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri >> "\\_SB.GPO1", 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x0019 >> } >> GpioInt (Edge, ActiveHigh, Shared,// PullDefault, 0x0000, >> "\\_SB.GPO1", 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x0013 >> } >> }) >> Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ >> } >> >> The setting of the special bit in the gpio control register leads to >> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() >> returning -EBUSY, which in return makes gpiod_get_optional >> return -EBUSY for this pin, rather then NULL (as we would like). > > Actually what is wrong here is that your gpiod_get(dev, "power") falls > back to use plain indexes and returns the first GPIO even though it > should not as the driver specifically requests GPIO with name "power" > and there is no _DSD. There is no clear "binding" for this device in ACPI, so the fallback is actually used as a feature by the Silead driver (more or less), the use of "power" as name here is for the ARM + devicetree usage of the driver really, so IOW the series: > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more > stricter which should in theory fix the problem as well. The patch > series is here: > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master You are referring to might fix this, but then we may need to add an attempt to get the gpio by index for some boards which do not control it themselves from _PS#. I can give the linked series a try, but I would still like a fallback plan if we indeed encounter boards where we need to fallback to getting the gpio by index. Once we do that we're back to having the same problem as then we would do the same fallback on boards where the pin is reserved for _PS# usage, and end up with an -EBUSY error again. I guess we could ignore -EBUSY in the fallback path, or only do the fallback if acpi_bus_power_manageable() returns false. 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
On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote: > > Actually what is wrong here is that your gpiod_get(dev, "power") falls > > back to use plain indexes and returns the first GPIO even though it > > should not as the driver specifically requests GPIO with name "power" > > and there is no _DSD. > > There is no clear "binding" for this device in ACPI, so the fallback > is actually used as a feature by the Silead driver (more or less), > the use of "power" as name here is for the ARM + devicetree usage > of the driver really, so IOW the series: > > > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more > > stricter which should in theory fix the problem as well. The patch > > series is here: > > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master > > You are referring to might fix this, but then we may need to add an > attempt to get the gpio by index for some boards which do not control > it themselves from _PS#. > > I can give the linked series a try, but I would still like a fallback > plan if we indeed encounter boards where we need to fallback to > getting the gpio by index. Once we do that we're back to having the > same problem as then we would do the same fallback on boards where > the pin is reserved for _PS# usage, and end up with an -EBUSY error > again. I guess we could ignore -EBUSY in the fallback path, or only > do the fallback if acpi_bus_power_manageable() returns false. I don't think using acpi_bus_power_manageable() is a proper way to fix this. This can be fixed without the fallback so that for the boards you know need to handle the GPIO themselves (based on the _HID for example), they will call acpi_dev_add_driver_gpios() passing the mapping for "power". The other boards then just don't get the GPIO. We really want to get rid of the whole fallback mess. -- 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 02-02-17 14:44, Mika Westerberg wrote: > On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote: >>> Actually what is wrong here is that your gpiod_get(dev, "power") falls >>> back to use plain indexes and returns the first GPIO even though it >>> should not as the driver specifically requests GPIO with name "power" >>> and there is no _DSD. >> >> There is no clear "binding" for this device in ACPI, so the fallback >> is actually used as a feature by the Silead driver (more or less), >> the use of "power" as name here is for the ARM + devicetree usage >> of the driver really, so IOW the series: >> >>> >>> Andy (Cc'd) has a patch that tries to make the fallback mechanism more >>> stricter which should in theory fix the problem as well. The patch >>> series is here: >>> >>> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master >> >> You are referring to might fix this, but then we may need to add an >> attempt to get the gpio by index for some boards which do not control >> it themselves from _PS#. >> >> I can give the linked series a try, but I would still like a fallback >> plan if we indeed encounter boards where we need to fallback to >> getting the gpio by index. Once we do that we're back to having the >> same problem as then we would do the same fallback on boards where >> the pin is reserved for _PS# usage, and end up with an -EBUSY error >> again. I guess we could ignore -EBUSY in the fallback path, or only >> do the fallback if acpi_bus_power_manageable() returns false. > > I don't think using acpi_bus_power_manageable() is a proper way to fix > this. Ok. > This can be fixed without the fallback so that for the boards you know > need to handle the GPIO themselves (based on the _HID for example), The _HID is the same everywhere, these boards dstd's are mostly a copy and paste fest. So this would to be DMI based. > they > will call acpi_dev_add_driver_gpios() passing the mapping for "power". > The other boards then just don't get the GPIO. The list of boards needing this might be huge. Anyways we will figure this out as we encounter boards needing some form of fallback to getting gpios based on index. If you think using acpi_bus_power_manageable() is a bad way to detect that no gpio is needed then we may just end up ignoring the -EBUSY return from gpiod_get_by_index 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
On Thu, Feb 02, 2017 at 02:55:57PM +0100, Hans de Goede wrote: > Hi, > > On 02-02-17 14:44, Mika Westerberg wrote: > > On Thu, Feb 02, 2017 at 02:27:28PM +0100, Hans de Goede wrote: > > > > Actually what is wrong here is that your gpiod_get(dev, "power") falls > > > > back to use plain indexes and returns the first GPIO even though it > > > > should not as the driver specifically requests GPIO with name "power" > > > > and there is no _DSD. > > > > > > There is no clear "binding" for this device in ACPI, so the fallback > > > is actually used as a feature by the Silead driver (more or less), > > > the use of "power" as name here is for the ARM + devicetree usage > > > of the driver really, so IOW the series: > > > > > > > > > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more > > > > stricter which should in theory fix the problem as well. The patch > > > > series is here: > > > > > > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master > > > > > > You are referring to might fix this, but then we may need to add an > > > attempt to get the gpio by index for some boards which do not control > > > it themselves from _PS#. > > > > > > I can give the linked series a try, but I would still like a fallback > > > plan if we indeed encounter boards where we need to fallback to > > > getting the gpio by index. Once we do that we're back to having the > > > same problem as then we would do the same fallback on boards where > > > the pin is reserved for _PS# usage, and end up with an -EBUSY error > > > again. I guess we could ignore -EBUSY in the fallback path, or only > > > do the fallback if acpi_bus_power_manageable() returns false. > > > > I don't think using acpi_bus_power_manageable() is a proper way to fix > > this. > > Ok. > > > This can be fixed without the fallback so that for the boards you know > > need to handle the GPIO themselves (based on the _HID for example), > > The _HID is the same everywhere, these boards dstd's are mostly a copy > and paste fest. So this would to be DMI based. :( I suppose the Windows driver just works everywhere, right? > > they > > will call acpi_dev_add_driver_gpios() passing the mapping for "power". > > The other boards then just don't get the GPIO. > > The list of boards needing this might be huge. Anyways we will figure > this out as we encounter boards needing some form of fallback to > getting gpios based on index. If you think using acpi_bus_power_manageable() > is a bad way to detect that no gpio is needed then we may just end up ignoring > the -EBUSY return from gpiod_get_by_index Well, that is certainly better than using acpi_bus_power_manageable() if nothing else can be done here. -- 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
> I suppose the Windows driver just works everywhere, right?
Not really, no.
AFAIK every vendor builds their own driver using a special configuration
utility from Silead.
All the device-specific data, including panel parameters and calibration data,
is compiled into this driver.
--
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 02-02-17 14:12, Mika Westerberg wrote: > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: >> Hi, >> >> On 02-02-17 13:32, Mika Westerberg wrote: >>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: >>>> I do not have a copy of the patch in this thread but sounds like >>>> something that might work. >>> >>> Actually, I seem have a copy of that patch. >>> >>> So you are saying that the device has a power GPIO in ACPI _CRS but it >>> should not be used for some reason? >> >> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti >> { >> Name (WBUF, ResourceTemplate () >> { >> I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, >> AddressingMode7Bit, "\\_SB.PCI0.I2C6", >> 0x00, ResourceConsumer, , Exclusive, >> ) >> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri >> "\\_SB.GPO1", 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x0019 >> } >> GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000, >> "\\_SB.GPO1", 0x00, ResourceConsumer, , >> ) >> { // Pin list >> 0x0013 >> } >> }) >> Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ >> } >> >> The setting of the special bit in the gpio control register leads to >> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() >> returning -EBUSY, which in return makes gpiod_get_optional >> return -EBUSY for this pin, rather then NULL (as we would like). > > Actually what is wrong here is that your gpiod_get(dev, "power") falls > back to use plain indexes and returns the first GPIO even though it > should not as the driver specifically requests GPIO with name "power" > and there is no _DSD. > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more > stricter which should in theory fix the problem as well. The patch > series is here: > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master Ok, that patches fixes the issues I was seeing with the silead driver on my cube iwork8 air cherrytrail tablet. As discussed we might need some extra handling to fallback to actually get a gpio by index on some tablets which don't control the gpio from ACPI AML code, but lets cross that bridge when we get there. 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
On Fri, Feb 10, 2017 at 12:52:41PM +0100, Hans de Goede wrote: > Hi, > > On 02-02-17 14:12, Mika Westerberg wrote: > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 02-02-17 13:32, Mika Westerberg wrote: > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: > > > > > I do not have a copy of the patch in this thread but sounds like > > > > > something that might work. > > > > > > > > Actually, I seem have a copy of that patch. > > > > > > > > So you are saying that the device has a power GPIO in ACPI _CRS but it > > > > should not be used for some reason? > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti > > > { > > > Name (WBUF, ResourceTemplate () > > > { > > > I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, > > > AddressingMode7Bit, "\\_SB.PCI0.I2C6", > > > 0x00, ResourceConsumer, , Exclusive, > > > ) > > > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri > > > "\\_SB.GPO1", 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x0019 > > > } > > > GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000, > > > "\\_SB.GPO1", 0x00, ResourceConsumer, , > > > ) > > > { // Pin list > > > 0x0013 > > > } > > > }) > > > Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ > > > } > > > > > > The setting of the special bit in the gpio control register leads to > > > drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() > > > returning -EBUSY, which in return makes gpiod_get_optional > > > return -EBUSY for this pin, rather then NULL (as we would like). > > > > Actually what is wrong here is that your gpiod_get(dev, "power") falls > > back to use plain indexes and returns the first GPIO even though it > > should not as the driver specifically requests GPIO with name "power" > > and there is no _DSD. > > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism more > > stricter which should in theory fix the problem as well. The patch > > series is here: > > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master > > Ok, that patches fixes the issues I was seeing with the silead driver > on my cube iwork8 air cherrytrail tablet. OK, good to hear. I suppose Andy can start cleaning up that branch and send them for review then ;-) > As discussed we might need some extra handling to fallback to actually > get a gpio by index on some tablets which don't control the gpio from > ACPI AML code, but lets cross that bridge when we get there. Agreed. -- 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 10-02-17 12:52, Hans de Goede wrote: > Hi, > > On 02-02-17 14:12, Mika Westerberg wrote: >> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: >>> Hi, >>> >>> On 02-02-17 13:32, Mika Westerberg wrote: >>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg wrote: >>>>> I do not have a copy of the patch in this thread but sounds like >>>>> something that might work. >>>> >>>> Actually, I seem have a copy of that patch. >>>> >>>> So you are saying that the device has a power GPIO in ACPI _CRS but it >>>> should not be used for some reason? >>> >>> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Setti >>> { >>> Name (WBUF, ResourceTemplate () >>> { >>> I2cSerialBusV2 (0x0040, ControllerInitiated, 0x00061A80, >>> AddressingMode7Bit, "\\_SB.PCI0.I2C6", >>> 0x00, ResourceConsumer, , Exclusive, >>> ) >>> GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestri >>> "\\_SB.GPO1", 0x00, ResourceConsumer, , >>> ) >>> { // Pin list >>> 0x0019 >>> } >>> GpioInt (Edge, ActiveHigh, Shared, PullDefault, 0x0000, >>> "\\_SB.GPO1", 0x00, ResourceConsumer, , >>> ) >>> { // Pin list >>> 0x0013 >>> } >>> }) >>> Return (WBUF) /* \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ >>> } >>> >>> The setting of the special bit in the gpio control register leads to >>> drivers/pinctrl/intel/pinctrl-cherryview.c chv_gpio_request_enable() >>> returning -EBUSY, which in return makes gpiod_get_optional >>> return -EBUSY for this pin, rather then NULL (as we would like). >> >> Actually what is wrong here is that your gpiod_get(dev, "power") falls >> back to use plain indexes and returns the first GPIO even though it >> should not as the driver specifically requests GPIO with name "power" >> and there is no _DSD. >> >> Andy (Cc'd) has a patch that tries to make the fallback mechanism more >> stricter which should in theory fix the problem as well. The patch >> series is here: >> >> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d143070a301d8b8883c349?at=master > > Ok, that patches fixes the issues I was seeing with the silead driver > on my cube iwork8 air cherrytrail tablet. But unfortunately it causes regressions for drivers which actually use gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which does: data->gpio_usb_id = devm_gpiod_get_index(dev, "id", INT3496_GPIO_USB_ID, GPIOD_IN); Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess this driver can be fixed by replacing "id" with NULL, but the name gets used in things like /sys/kernel/debug/gpio and is actually useful there, so it looks like that patch from Andy needs some work so as to not see getting by index as an undesirable fallback while the driver is actually doing a request gpio by index. 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
On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > Hi, > > On 10-02-17 12:52, Hans de Goede wrote: > > Hi, > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 02-02-17 13:32, Mika Westerberg wrote: > > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg > > > > > wrote: > > > > > > I do not have a copy of the patch in this thread but sounds > > > > > > like > > > > > > something that might work. > > > > > > > > > > Actually, I seem have a copy of that patch. > > > > > > > > > > So you are saying that the device has a power GPIO in ACPI > > > > > _CRS but it > > > > > should not be used for some reason? > > > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: > > > > Current Resource Setti > > > > { > > > > Name (WBUF, ResourceTemplate () > > > > { > > > > I2cSerialBusV2 (0x0040, > > > > ControllerInitiated, 0x00061A80, > > > > AddressingMode7Bit, > > > > "\\_SB.PCI0.I2C6", > > > > 0x00, ResourceConsumer, , Exclusive, > > > > ) > > > > GpioIo (Exclusive, PullDefault, 0x0000, > > > > 0x0000, IoRestri > > > > "\\_SB.GPO1", 0x00, > > > > ResourceConsumer, , > > > > ) > > > > { // Pin list > > > > 0x0019 > > > > } > > > > GpioInt (Edge, ActiveHigh, Shared, > > > > PullDefault, 0x0000, > > > > "\\_SB.GPO1", 0x00, > > > > ResourceConsumer, , > > > > ) > > > > { // Pin list > > > > 0x0013 > > > > } > > > > }) > > > > Return (WBUF) /* > > > > \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ > > > > } > > > > > > > > The setting of the special bit in the gpio control register > > > > leads to > > > > drivers/pinctrl/intel/pinctrl-cherryview.c > > > > chv_gpio_request_enable() > > > > returning -EBUSY, which in return makes gpiod_get_optional > > > > return -EBUSY for this pin, rather then NULL (as we would like). > > > > > > Actually what is wrong here is that your gpiod_get(dev, "power") > > > falls > > > back to use plain indexes and returns the first GPIO even though > > > it > > > should not as the driver specifically requests GPIO with name > > > "power" > > > and there is no _DSD. > > > > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism > > > more > > > stricter which should in theory fix the problem as well. The patch > > > series is here: > > > > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1 > > > 43070a301d8b8883c349?at=master > > > > Ok, that patches fixes the issues I was seeing with the silead > > driver > > on my cube iwork8 air cherrytrail tablet. > > But unfortunately it causes regressions for drivers which actually use > gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which > does: > > data->gpio_usb_id = devm_gpiod_get_index(dev, "id", > INT3496_GPIO_USB_ID, > GPIOD_IN); > > Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess > this driver can be fixed by replacing "id" with NULL, but the name > gets used in things like /sys/kernel/debug/gpio and is actually > useful there, so it looks like that patch from Andy needs some > work so as to not see getting by index as an undesirable fallback > while the driver is actually doing a request gpio by index. Yes, this part is missed. We would like to introduce a flag for that to encourage drivers to fix this mess by adding compatible lookup table.
On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > Hi, > > On 10-02-17 12:52, Hans de Goede wrote: > > Hi, > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: > > > > Hi, > > > > > > > > On 02-02-17 13:32, Mika Westerberg wrote: > > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg > > > > > wrote: > > > > > > I do not have a copy of the patch in this thread but sounds > > > > > > like > > > > > > something that might work. > > > > > > > > > > Actually, I seem have a copy of that patch. > > > > > > > > > > So you are saying that the device has a power GPIO in ACPI > > > > > _CRS but it > > > > > should not be used for some reason? > > > > > > > > Method (_CRS, 0, NotSerialized) // _CRS: > > > > Current Resource Setti > > > > { > > > > Name (WBUF, ResourceTemplate () > > > > { > > > > I2cSerialBusV2 (0x0040, > > > > ControllerInitiated, 0x00061A80, > > > > AddressingMode7Bit, > > > > "\\_SB.PCI0.I2C6", > > > > 0x00, ResourceConsumer, , Exclusive, > > > > ) > > > > GpioIo (Exclusive, PullDefault, 0x0000, > > > > 0x0000, IoRestri > > > > "\\_SB.GPO1", 0x00, > > > > ResourceConsumer, , > > > > ) > > > > { // Pin list > > > > 0x0019 > > > > } > > > > GpioInt (Edge, ActiveHigh, Shared, > > > > PullDefault, 0x0000, > > > > "\\_SB.GPO1", 0x00, > > > > ResourceConsumer, , > > > > ) > > > > { // Pin list > > > > 0x0013 > > > > } > > > > }) > > > > Return (WBUF) /* > > > > \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ > > > > } > > > > > > > > The setting of the special bit in the gpio control register > > > > leads to > > > > drivers/pinctrl/intel/pinctrl-cherryview.c > > > > chv_gpio_request_enable() > > > > returning -EBUSY, which in return makes gpiod_get_optional > > > > return -EBUSY for this pin, rather then NULL (as we would like). > > > > > > Actually what is wrong here is that your gpiod_get(dev, "power") > > > falls > > > back to use plain indexes and returns the first GPIO even though > > > it > > > should not as the driver specifically requests GPIO with name > > > "power" > > > and there is no _DSD. > > > > > > Andy (Cc'd) has a patch that tries to make the fallback mechanism > > > more > > > stricter which should in theory fix the problem as well. The patch > > > series is here: > > > > > > https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1 > > > 43070a301d8b8883c349?at=master > > > > Ok, that patches fixes the issues I was seeing with the silead > > driver > > on my cube iwork8 air cherrytrail tablet. > > But unfortunately it causes regressions for drivers which actually use > gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which > does: > > data->gpio_usb_id = devm_gpiod_get_index(dev, "id", > INT3496_GPIO_USB_ID, > GPIOD_IN); > > Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess > this driver can be fixed by replacing "id" with NULL, but the name > gets used in things like /sys/kernel/debug/gpio and is actually > useful there, so it looks like that patch from Andy needs some > work so as to not see getting by index as an undesirable fallback > while the driver is actually doing a request gpio by index. Hans, I have just pushed most recent stuff into my branch. Would you have a chance test it? It has extcon patches embedded.
Hi Andy, On 22-02-17 16:52, Andy Shevchenko wrote: > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: >> Hi, >> >> On 10-02-17 12:52, Hans de Goede wrote: >>> Hi, >>> >>> On 02-02-17 14:12, Mika Westerberg wrote: >>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: >>>>> Hi, >>>>> >>>>> On 02-02-17 13:32, Mika Westerberg wrote: >>>>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg >>>>>> wrote: >>>>>>> I do not have a copy of the patch in this thread but sounds >>>>>>> like >>>>>>> something that might work. >>>>>> >>>>>> Actually, I seem have a copy of that patch. >>>>>> >>>>>> So you are saying that the device has a power GPIO in ACPI >>>>>> _CRS but it >>>>>> should not be used for some reason? >>>>> >>>>> Method (_CRS, 0, NotSerialized) // _CRS: >>>>> Current Resource Setti >>>>> { >>>>> Name (WBUF, ResourceTemplate () >>>>> { >>>>> I2cSerialBusV2 (0x0040, >>>>> ControllerInitiated, 0x00061A80, >>>>> AddressingMode7Bit, >>>>> "\\_SB.PCI0.I2C6", >>>>> 0x00, ResourceConsumer, , Exclusive, >>>>> ) >>>>> GpioIo (Exclusive, PullDefault, 0x0000, >>>>> 0x0000, IoRestri >>>>> "\\_SB.GPO1", 0x00, >>>>> ResourceConsumer, , >>>>> ) >>>>> { // Pin list >>>>> 0x0019 >>>>> } >>>>> GpioInt (Edge, ActiveHigh, Shared, >>>>> PullDefault, 0x0000, >>>>> "\\_SB.GPO1", 0x00, >>>>> ResourceConsumer, , >>>>> ) >>>>> { // Pin list >>>>> 0x0013 >>>>> } >>>>> }) >>>>> Return (WBUF) /* >>>>> \_SB_.PCI0.I2C6.TCS4._CRS.WBUF */ >>>>> } >>>>> >>>>> The setting of the special bit in the gpio control register >>>>> leads to >>>>> drivers/pinctrl/intel/pinctrl-cherryview.c >>>>> chv_gpio_request_enable() >>>>> returning -EBUSY, which in return makes gpiod_get_optional >>>>> return -EBUSY for this pin, rather then NULL (as we would like). >>>> >>>> Actually what is wrong here is that your gpiod_get(dev, "power") >>>> falls >>>> back to use plain indexes and returns the first GPIO even though >>>> it >>>> should not as the driver specifically requests GPIO with name >>>> "power" >>>> and there is no _DSD. >>>> >>>> Andy (Cc'd) has a patch that tries to make the fallback mechanism >>>> more >>>> stricter which should in theory fix the problem as well. The patch >>>> series is here: >>>> >>>> https://bitbucket.org/andy-shev/linux/commits/338c0226b631b8b497d1 >>>> 43070a301d8b8883c349?at=master >>> >>> Ok, that patches fixes the issues I was seeing with the silead >>> driver >>> on my cube iwork8 air cherrytrail tablet. >> >> But unfortunately it causes regressions for drivers which actually use >> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, which >> does: >> >> data->gpio_usb_id = devm_gpiod_get_index(dev, "id", >> INT3496_GPIO_USB_ID, >> GPIOD_IN); >> >> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I guess >> this driver can be fixed by replacing "id" with NULL, but the name >> gets used in things like /sys/kernel/debug/gpio and is actually >> useful there, so it looks like that patch from Andy needs some >> work so as to not see getting by index as an undesirable fallback >> while the driver is actually doing a request gpio by index. > > Hans, I have just pushed most recent stuff into my branch. Would you > have a chance test it? It has extcon patches embedded. First of all thank you for working on this. Before I spend time on testing this I must say that I've the feeling these patches are going in the wrong direction. I would expect you to modify gpiod_get_index to internally inside the gpiolib code pass a flag which makes it clear that the name is just a hint and that it should fallback to the index (*), as it is doing before your patches to clean things up. That way we avoid needing to fixup the drivers and add with IMHO is unnecessary boilerplate to them, in both the extcon-intel-int3496.c and soc_button_array.c cases we really just want to get a gpio by index and the name is just there to make debugging easier. Also if you look at the ACPI 6.0 or later spec. then there is a new "generic button device" defined there and I've patches to soc_button_array.c pending to add support for that: https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e6030dfcaddb555d0e2d https://github.com/jwrdegoede/linux-sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5 The ACPI spec clearly defines the _DSD (device specific data) for these devices with a ACPI0011 _HID as containing an index into the ACPI resources table for the device, since your patches make it impossible to directly get a gpio by index (if one still want the gpio to have a sensible name) that means I now need to create an acpi_gpio_mapping table on the fly for this. TL;DR: this approach seems like a lot of extra work / churn and boilerplate code in drivers for no gain. Can't we please just simply keep the fallback as-is when a driver calls gpiod_get_index rather then gpiod_get ? That seems like a lot simpler and cleaner solution to me. Regards, Hans *) Or maybe even a flag that it is the index which should be looked at and not the name, but that may break some existing users -- 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
On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: > On 22-02-17 16:52, Andy Shevchenko wrote: > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > > > On 10-02-17 12:52, Hans de Goede wrote: > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: > > > > > > On 02-02-17 13:32, Mika Westerberg wrote: > > > > > > > On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg > > > > > > > wrote: > > > > Ok, that patches fixes the issues I was seeing with the silead > > > > driver > > > > on my cube iwork8 air cherrytrail tablet. > > > > > > But unfortunately it causes regressions for drivers which actually > > > use > > > gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, > > > which > > > does: > > > > > > data->gpio_usb_id = devm_gpiod_get_index(dev, "id", > > > INT3496_GPIO_USB_ > > > ID, > > > GPIOD_IN); > > > > > > Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I > > > guess > > > this driver can be fixed by replacing "id" with NULL, but the name > > > gets used in things like /sys/kernel/debug/gpio and is actually > > > useful there, so it looks like that patch from Andy needs some > > > work so as to not see getting by index as an undesirable fallback > > > while the driver is actually doing a request gpio by index. > > > > Hans, I have just pushed most recent stuff into my branch. Would you > > have a chance test it? It has extcon patches embedded. > > First of all thank you for working on this. > > Before I spend time on testing this I must say that I've the feeling > these patches are going in the wrong direction. > > I would expect you to modify gpiod_get_index to internally inside > the gpiolib code pass a flag which makes it clear that the name is > just a hint and that it should fallback to the index (*), as it is > doing before your patches to clean things up. That way we avoid > needing to fixup the drivers and add with IMHO is unnecessary > boilerplate to them, in both the extcon-intel-int3496.c and > soc_button_array.c cases we really just want to get a gpio by > index and the name is just there to make debugging easier. Unfortunately the flag solution was discussed as a *temporary* one to makes someone's eye hurt when see the flag. Real solution is exactly what I'm doing right now. (Side note: when I started implementing flag option, I realized that it even uglier than I thought, that's why I decide to go for fixing users first) The problem is that previously ACPI has no mean of mapping between indexes and names, and connection ID has being abused for ACPI case. Basically it means you can put *anything* as connection ID right now. And this is bad, very bad idea! Now, since we have _DSD and specification for mapping GPIO resources to names (connection IDs!) we should *not* allow drivers to put anything they want there. It means that any driver that is supposed to be used on ACPI-based platfroms with *or* without _DSD should provide a mapping table for the latter case. Other solution is to extend GPIO API to have almost all same set of calls with an additional field "label" as it was recently done for fwnode_get_gpiod_child() (whatever its name nowadays). I don't think this is best (though allows less intrusion to the existing drivers) way because (see above) an heavy abuse in the kernel of connection ID meaning for ACPI-enabled platforms. > Also if you look at the ACPI 6.0 or later spec. then there is > a new "generic button device" defined there and I've patches to > soc_button_array.c pending to add support for that: > > https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e > 6030dfcaddb555d0e2d > https://github.com/jwrdegoede/linux- > sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5 Side note. The one patch is okay, the main one needs a comprehensive review (at least two points just came to my mind: a) it should be done in generic ACPICA / Linux ACPI glue code, b) it should use UUID library in kernel). > The ACPI spec clearly defines the _DSD (device specific data) > for these devices with a ACPI0011 _HID as containing an index > into the ACPI resources table for the device, since your patches > make it impossible to directly get a gpio by index (if one still > want the gpio to have a sensible name) that means I now need > to create an acpi_gpio_mapping table on the fly for this. Since the GPIO API doesn't provide an additional "label" field when you get it. Otherwise the problem that you never know what you get by index. Regarding to how to create this, I think, as I already said above, it would be internal stuff to Linux ACPI glue layer and perhaps GPIO ACPI library. > TL;DR: this approach seems like a lot of extra work / churn and > boilerplate code in drivers for no gain. Yes, because of current *abuse* of connection ID field in ACPI case. > Can't we please just simply keep the fallback as-is when a driver > calls gpiod_get_index rather then gpiod_get ? That seems like a > lot simpler and cleaner solution to me. No. We can't. This is explained by documentation addon in: https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0db1df 4182673826646c (with flag removed approach) https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf17093 dacccd30e349cc and in commit message https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3a139 0075613daee56f > *) Or maybe even a flag that it is the index which should be looked at > and not the name, but that may break some existing users Mika, Linus, I would really appreciate your input to the topic: opinions, critique, ideas, etc.
Hi, On 02-03-17 12:38, Andy Shevchenko wrote: > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: >> On 22-02-17 16:52, Andy Shevchenko wrote: >>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: >>>> On 10-02-17 12:52, Hans de Goede wrote: >>>>> On 02-02-17 14:12, Mika Westerberg wrote: >>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede wrote: >>>>>>> On 02-02-17 13:32, Mika Westerberg wrote: >>>>>>>> On Thu, Feb 02, 2017 at 02:10:18PM +0200, Mika Westerberg >>>>>>>> wrote: > >>>>> Ok, that patches fixes the issues I was seeing with the silead >>>>> driver >>>>> on my cube iwork8 air cherrytrail tablet. >>>> >>>> But unfortunately it causes regressions for drivers which actually >>>> use >>>> gpiod_get_by_index, e.g. drivers/extcon/extcon-intel-int3496.c, >>>> which >>>> does: >>>> >>>> data->gpio_usb_id = devm_gpiod_get_index(dev, "id", >>>> INT3496_GPIO_USB_ >>>> ID, >>>> GPIOD_IN); >>>> >>>> Where GPIOD_IN is 0, (it also gets gpios for index 1 and 2), I >>>> guess >>>> this driver can be fixed by replacing "id" with NULL, but the name >>>> gets used in things like /sys/kernel/debug/gpio and is actually >>>> useful there, so it looks like that patch from Andy needs some >>>> work so as to not see getting by index as an undesirable fallback >>>> while the driver is actually doing a request gpio by index. >>> >>> Hans, I have just pushed most recent stuff into my branch. Would you >>> have a chance test it? It has extcon patches embedded. >> >> First of all thank you for working on this. >> >> Before I spend time on testing this I must say that I've the feeling >> these patches are going in the wrong direction. >> >> I would expect you to modify gpiod_get_index to internally inside >> the gpiolib code pass a flag which makes it clear that the name is >> just a hint and that it should fallback to the index (*), as it is >> doing before your patches to clean things up. That way we avoid >> needing to fixup the drivers and add with IMHO is unnecessary >> boilerplate to them, in both the extcon-intel-int3496.c and >> soc_button_array.c cases we really just want to get a gpio by >> index and the name is just there to make debugging easier. > > Unfortunately the flag solution was discussed as a *temporary* one to > makes someone's eye hurt when see the flag. Real solution is exactly > what I'm doing right now. > > (Side note: when I started implementing flag option, I realized that it > even uglier than I thought, that's why I decide to go for fixing users > first) Ok. > The problem is that previously ACPI has no mean of mapping between > indexes and names, and connection ID has being abused for ACPI case. > > Basically it means you can put *anything* as connection ID right now. > And this is bad, very bad idea! Ok. > Now, since we have _DSD and specification for mapping GPIO resources to > names (connection IDs!) we should *not* allow drivers to put anything > they want there. > > It means that any driver that is supposed to be used on ACPI-based > platfroms with *or* without _DSD should provide a mapping table for the > latter case. > > Other solution is to extend GPIO API to have almost all same set of > calls with an additional field "label" as it was recently done for > fwnode_get_gpiod_child() (whatever its name nowadays). I don't think > this is best (though allows less intrusion to the existing drivers) way > because (see above) an heavy abuse in the kernel of connection ID > meaning for ACPI-enabled platforms. Hmm, I actually like the label vs connection-id distinction there are many ACPI device "bindings" where we simply get an index into the ACPI resource table for the device as only way to get the right gpio. Forcing the addition of a connection-id table to all those drivers not only is needless churn / boilerplate, but also gives the false impression that we actually have more info (a valid connection id) then we really have. So my vote goes to adding a label field, and passing NULL as connection id in these drivers, rather then adding a fake connection-id table. >> Also if you look at the ACPI 6.0 or later spec. then there is >> a new "generic button device" defined there and I've patches to >> soc_button_array.c pending to add support for that: >> >> https://github.com/jwrdegoede/linux-sunxi/commit/4fad488f818a9e45bd27e >> 6030dfcaddb555d0e2d >> https://github.com/jwrdegoede/linux- >> sunxi/commit/ae8a643e9060979e43950ae3ad09623c6b7fcaa5 > > Side note. > The one patch is okay, the main one needs a comprehensive review (at > least two points just came to my mind: a) it should be done in generic > ACPICA / Linux ACPI glue code, b) it should use UUID library in kernel). > >> The ACPI spec clearly defines the _DSD (device specific data) >> for these devices with a ACPI0011 _HID as containing an index >> into the ACPI resources table for the device, since your patches >> make it impossible to directly get a gpio by index (if one still >> want the gpio to have a sensible name) that means I now need >> to create an acpi_gpio_mapping table on the fly for this. > > Since the GPIO API doesn't provide an additional "label" field when you > get it. Otherwise the problem that you never know what you get by index. > > Regarding to how to create this, I think, as I already said above, it > would be internal stuff to Linux ACPI glue layer and perhaps GPIO ACPI > library. > >> TL;DR: this approach seems like a lot of extra work / churn and >> boilerplate code in drivers for no gain. > > Yes, because of current *abuse* of connection ID field in ACPI case. > >> Can't we please just simply keep the fallback as-is when a driver >> calls gpiod_get_index rather then gpiod_get ? That seems like a >> lot simpler and cleaner solution to me. > > No. We can't. > > This is explained by documentation addon in: > > https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0db1df > 4182673826646c > > (with flag removed approach) > https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf17093 > dacccd30e349cc > > and in commit message > > https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3a139 > 0075613daee56f > >> *) Or maybe even a flag that it is the index which should be looked at >> and not the name, but that may break some existing users > > Mika, Linus, I would really appreciate your input to the topic: > opinions, critique, ideas, etc. So my opinion on this is that I prefer the add a label field idea over the everything must have either a connection-id in ACPI or a connection-id-table in the driver. 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
On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote: > Hi, > > On 02-03-17 12:38, Andy Shevchenko wrote: > > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: > > > On 22-02-17 16:52, Andy Shevchenko wrote: > > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > > > > > On 10-02-17 12:52, Hans de Goede wrote: > > > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede > > > > > > > wrote: > > Now, since we have _DSD and specification for mapping GPIO resources > > to > > names (connection IDs!) we should *not* allow drivers to put > > anything > > they want there. > > > > It means that any driver that is supposed to be used on ACPI-based > > platfroms with *or* without _DSD should provide a mapping table for > > the > > latter case. > > > > Other solution is to extend GPIO API to have almost all same set of > > calls with an additional field "label" as it was recently done for > > fwnode_get_gpiod_child() (whatever its name nowadays). I don't think > > this is best (though allows less intrusion to the existing drivers) > > way > > because (see above) an heavy abuse in the kernel of connection ID > > meaning for ACPI-enabled platforms. > > Hmm, I actually like the label vs connection-id distinction there > are many ACPI device "bindings" where we simply get an index into > the ACPI resource table for the device as only way to get the right > gpio. Yeah, and we will be screwed if the index matrix is changed per device ID / board. > Forcing the addition of a connection-id table to all those > drivers not only is needless churn / boilerplate, but also gives the > false impression that we actually have more info (a valid connection > id) then we really have. Again, we have no idea what exactly we get if we call gpiod_get_index(..., NULL, index); in case of ACPI. It would work *if and only if* we assume that all versions of the same device on all possible platforms will have *very same* mapping. I have heard it's already not true. Check the commit 89ab37b489d1 ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96"). > So my vote goes to adding a label field, and passing NULL as > connection id in these drivers, rather then adding a fake connection- > id > table. There are also few concerns: 1) it would be often passed the same string as connection ID and label (okay, meaning we need like two functions per each in current API, something like gpiod_get_*label(dev, con_id, ..., label); and gpiod_get_label_nocid(dev, ..., label); besides the former ones); 2) using labels different to connection ID sounds not okay for debugging purposes (I dunno why we have this done for fwnode_get_gpiod_child() in the first place); 3) additionally different labels will not play good in _DSD enabled case, since we must use connection ID there (we believe firmware until otherwise is proven). 4) if some firmwares have different indexes for the same device we will need to have device ID (PCI ID, ... or alike) to _CRS index mapping anyway in the code. > > > TL;DR: this approach seems like a lot of extra work / churn and > > > boilerplate code in drivers for no gain. > > > > Yes, because of current *abuse* of connection ID field in ACPI case. > > > > > Can't we please just simply keep the fallback as-is when a driver > > > calls gpiod_get_index rather then gpiod_get ? That seems like a > > > lot simpler and cleaner solution to me. > > > > No. We can't. > > > > This is explained by documentation addon in: > > > > https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0d > > b1df > > 4182673826646c > > > > (with flag removed approach) > > https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf1 > > 7093 > > dacccd30e349cc > > > > and in commit message > > > > https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3 > > a139 > > 0075613daee56f > > > > > *) Or maybe even a flag that it is the index which should be > > > looked at > > > and not the name, but that may break some existing users > > > > Mika, Linus, I would really appreciate your input to the topic: > > opinions, critique, ideas, etc. > > So my opinion on this is that I prefer the add a label field idea over > the everything must have either a connection-id in ACPI or a > connection-id-table in the driver. As a ultimate workaround it would work, in long-term prospective it's not a solution.
Hi, On 03-03-17 15:57, Andy Shevchenko wrote: > On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote: >> Hi, >> >> On 02-03-17 12:38, Andy Shevchenko wrote: >>> On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: >>>> On 22-02-17 16:52, Andy Shevchenko wrote: >>>>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: >>>>>> On 10-02-17 12:52, Hans de Goede wrote: >>>>>>> On 02-02-17 14:12, Mika Westerberg wrote: >>>>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de Goede >>>>>>>> wrote: > >>> Now, since we have _DSD and specification for mapping GPIO resources >>> to >>> names (connection IDs!) we should *not* allow drivers to put >>> anything >>> they want there. >>> >>> It means that any driver that is supposed to be used on ACPI-based >>> platfroms with *or* without _DSD should provide a mapping table for >>> the >>> latter case. >>> >>> Other solution is to extend GPIO API to have almost all same set of >>> calls with an additional field "label" as it was recently done for >>> fwnode_get_gpiod_child() (whatever its name nowadays). I don't think >>> this is best (though allows less intrusion to the existing drivers) >>> way >>> because (see above) an heavy abuse in the kernel of connection ID >>> meaning for ACPI-enabled platforms. >> >> Hmm, I actually like the label vs connection-id distinction there >> are many ACPI device "bindings" where we simply get an index into >> the ACPI resource table for the device as only way to get the right >> gpio. > > Yeah, and we will be screwed if the index matrix is changed per device > ID / board. > >> Forcing the addition of a connection-id table to all those >> drivers not only is needless churn / boilerplate, but also gives the >> false impression that we actually have more info (a valid connection >> id) then we really have. > > Again, we have no idea what exactly we get if we call > gpiod_get_index(..., NULL, index); in case of ACPI. It would work *if > and only if* we assume that all versions of the same device on all > possible platforms will have *very same* mapping. > > I have heard it's already not true. Check the commit 89ab37b489d1 > ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96"). > >> So my vote goes to adding a label field, and passing NULL as >> connection id in these drivers, rather then adding a fake connection- >> id >> table. > > There are also few concerns: > > 1) it would be often passed the same string as connection ID and label > (okay, meaning we need like two functions per each in current API, > something like gpiod_get_*label(dev, con_id, ..., label); > and gpiod_get_label_nocid(dev, ..., label); besides the former ones); I would think the _label variants would not allow a connection_id at all. > 2) using labels different to connection ID sounds not okay for debugging > purposes (I dunno why we have this done for fwnode_get_gpiod_child() in > the first place); Right, which is why the _label variants would not allow a connection_id at all using an _label variants implies connection_id == NULL. And using a variant with connection_id argument implies label = connection_id. > 3) additionally different labels will not play good in _DSD enabled > case, since we must use connection ID there (we believe firmware until > otherwise is proven). Again, gpios would have a connection_id OR a label, so in _DSD case only a connection_id. > 4) if some firmwares have different indexes for the same device we will > need to have device ID (PCI ID, ... or alike) to _CRS index mapping > anyway in the code. This problem will exist independent of which solution we choose. >>>> TL;DR: this approach seems like a lot of extra work / churn and >>>> boilerplate code in drivers for no gain. >>> >>> Yes, because of current *abuse* of connection ID field in ACPI case. >>> >>>> Can't we please just simply keep the fallback as-is when a driver >>>> calls gpiod_get_index rather then gpiod_get ? That seems like a >>>> lot simpler and cleaner solution to me. >>> >>> No. We can't. >>> >>> This is explained by documentation addon in: >>> >>> https://bitbucket.org/andy-shev/linux/commits/27f4d31dcf7997613dfb0d >>> b1df >>> 4182673826646c >>> >>> (with flag removed approach) >>> https://bitbucket.org/andy-shev/linux/commits/ab99ade0bab99983f63bf1 >>> 7093 >>> dacccd30e349cc >>> >>> and in commit message >>> >>> https://bitbucket.org/andy-shev/linux/commits/3a50bf0c17df18df6758a3 >>> a139 >>> 0075613daee56f >>> >>>> *) Or maybe even a flag that it is the index which should be >>>> looked at >>>> and not the name, but that may break some existing users >>> >>> Mika, Linus, I would really appreciate your input to the topic: >>> opinions, critique, ideas, etc. >> >> So my opinion on this is that I prefer the add a label field idea over >> the everything must have either a connection-id in ACPI or a >> connection-id-table in the driver. > > As a ultimate workaround it would work, in long-term prospective it's > not a solution. I believe that this will work fine even in the long run, better then forcible adding fake _DSD tables everywhere, see above. 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
On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote: > Hi, > > On 03-03-17 15:57, Andy Shevchenko wrote: > > On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 02-03-17 12:38, Andy Shevchenko wrote: > > > > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: > > > > > On 22-02-17 16:52, Andy Shevchenko wrote: > > > > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > > > > > > > On 10-02-17 12:52, Hans de Goede wrote: > > > > > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de > > > > > > > > > Goede > > > > > > > > > wrote: > > > Forcing the addition of a connection-id table to all those > > > drivers not only is needless churn / boilerplate, but also gives > > > the > > > false impression that we actually have more info (a valid > > > connection > > > id) then we really have. > > > > Again, we have no idea what exactly we get if we call > > gpiod_get_index(..., NULL, index); in case of ACPI. It would work > > *if > > and only if* we assume that all versions of the same device on all > > possible platforms will have *very same* mapping. > > > > I have heard it's already not true. Check the commit 89ab37b489d1 > > ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96"). Any comment / remark on this? > > > So my vote goes to adding a label field, and passing NULL as > > > connection id in these drivers, rather then adding a fake > > > connection- > > > id > > > table. > > > > There are also few concerns: > > > > 1) it would be often passed the same string as connection ID and > > label > > (okay, meaning we need like two functions per each in current API, > > something like gpiod_get_*label(dev, con_id, ..., label); > > and gpiod_get_label_nocid(dev, ..., label); besides the former > > ones); > > I would think the _label variants would not allow a connection_id at > all. Ah, okay. > > 2) using labels different to connection ID sounds not okay for > > debugging > > purposes (I dunno why we have this done for fwnode_get_gpiod_child() > > in > > the first place); > > Right, which is why the _label variants would not allow a > connection_id > at all using an _label variants implies connection_id == NULL. > > And using a variant with connection_id argument implies label > = connection_id. > > > 3) additionally different labels will not play good in _DSD enabled > > case, since we must use connection ID there (we believe firmware > > until > > otherwise is proven). > > Again, gpios would have a connection_id OR a label, so in > _DSD case only a connection_id. > > > 4) if some firmwares have different indexes for the same device we > > will > > need to have device ID (PCI ID, ... or alike) to _CRS index mapping > > anyway in the code. > > This problem will exist independent of which solution we choose. Yes. > > > > > > > > Mika, Linus, I would really appreciate your input to the topic: > > > > opinions, critique, ideas, etc. > > > > > > So my opinion on this is that I prefer the add a label field idea > > > over > > > the everything must have either a connection-id in ACPI or a > > > connection-id-table in the driver. > > > > As a ultimate workaround it would work, in long-term prospective > > it's > > not a solution. > I believe that this will work fine even in the long run, better then See above about bluetooth case. > forcible adding fake _DSD tables everywhere, see above. Why are they fake?
Hi, On 03-03-17 16:23, Andy Shevchenko wrote: > On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote: >> Hi, >> >> On 03-03-17 15:57, Andy Shevchenko wrote: >>> On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote: >>>> Hi, >>>> >>>> On 02-03-17 12:38, Andy Shevchenko wrote: >>>>> On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: >>>>>> On 22-02-17 16:52, Andy Shevchenko wrote: >>>>>>> On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: >>>>>>>> On 10-02-17 12:52, Hans de Goede wrote: >>>>>>>>> On 02-02-17 14:12, Mika Westerberg wrote: >>>>>>>>>> On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de >>>>>>>>>> Goede >>>>>>>>>> wrote: > >>>> Forcing the addition of a connection-id table to all those >>>> drivers not only is needless churn / boilerplate, but also gives >>>> the >>>> false impression that we actually have more info (a valid >>>> connection >>>> id) then we really have. >>> >>> Again, we have no idea what exactly we get if we call >>> gpiod_get_index(..., NULL, index); in case of ACPI. It would work >>> *if >>> and only if* we assume that all versions of the same device on all >>> possible platforms will have *very same* mapping. >>> >>> I have heard it's already not true. Check the commit 89ab37b489d1 >>> ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96"). > > Any comment / remark on this? Looking at that commit, the indxex order is consistent for a single device HID/CID but at some point in time the decided to change the order for newer HIDs which sucks from a driver pov, but is less bad then it could have been (they could have different orders for the same HID and we would need to fallback to dmi quirks). But I fail to see how this is really relevant for this discussion. >>>> So my vote goes to adding a label field, and passing NULL as >>>> connection id in these drivers, rather then adding a fake >>>> connection- >>>> id >>>> table. >>> >>> There are also few concerns: >>> >>> 1) it would be often passed the same string as connection ID and >>> label >>> (okay, meaning we need like two functions per each in current API, >>> something like gpiod_get_*label(dev, con_id, ..., label); >>> and gpiod_get_label_nocid(dev, ..., label); besides the former >>> ones); >> >> I would think the _label variants would not allow a connection_id at >> all. > > Ah, okay. > >>> 2) using labels different to connection ID sounds not okay for >>> debugging >>> purposes (I dunno why we have this done for fwnode_get_gpiod_child() >>> in >>> the first place); >> >> Right, which is why the _label variants would not allow a >> connection_id >> at all using an _label variants implies connection_id == NULL. >> >> And using a variant with connection_id argument implies label >> = connection_id. >> >>> 3) additionally different labels will not play good in _DSD enabled >>> case, since we must use connection ID there (we believe firmware >>> until >>> otherwise is proven). >> >> Again, gpios would have a connection_id OR a label, so in >> _DSD case only a connection_id. >> >>> 4) if some firmwares have different indexes for the same device we >>> will >>> need to have device ID (PCI ID, ... or alike) to _CRS index mapping >>> anyway in the code. >> >> This problem will exist independent of which solution we choose. > > Yes. > >>>>> >>>>> Mika, Linus, I would really appreciate your input to the topic: >>>>> opinions, critique, ideas, etc. >>>> >>>> So my opinion on this is that I prefer the add a label field idea >>>> over >>>> the everything must have either a connection-id in ACPI or a >>>> connection-id-table in the driver. >>> >>> As a ultimate workaround it would work, in long-term prospective >>> it's >>> not a solution. > >> I believe that this will work fine even in the long run, better then > > See above about bluetooth case. > >> forcible adding fake _DSD tables everywhere, see above. > > Why are they fake? Because they are not coming from the firmware, as said I believe it is better to clearly differentiate between the no-connection-id (so we use indexes) and the firmware provides connection-ids cases. I believe this is better for 2 reasons: 1) Cleaner (and less) code for drivers which need to use indexes 2) It is easier to debug things if it is clear at all levels if we are dealing with indexes or connection ids 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
On Mon, 2017-03-06 at 10:31 +0100, Hans de Goede wrote: > Hi, > > On 03-03-17 16:23, Andy Shevchenko wrote: > > On Fri, 2017-03-03 at 16:19 +0100, Hans de Goede wrote: > > > Hi, > > > > > > On 03-03-17 15:57, Andy Shevchenko wrote: > > > > On Thu, 2017-03-02 at 16:34 +0100, Hans de Goede wrote: > > > > > Hi, > > > > > > > > > > On 02-03-17 12:38, Andy Shevchenko wrote: > > > > > > On Thu, 2017-02-23 at 15:19 +0100, Hans de Goede wrote: > > > > > > > On 22-02-17 16:52, Andy Shevchenko wrote: > > > > > > > > On Sun, 2017-02-12 at 11:40 +0100, Hans de Goede wrote: > > > > > > > > > On 10-02-17 12:52, Hans de Goede wrote: > > > > > > > > > > On 02-02-17 14:12, Mika Westerberg wrote: > > > > > > > > > > > On Thu, Feb 02, 2017 at 01:50:58PM +0100, Hans de > > > > > > > > > > > Goede > > > > > > > > > > > wrote: > > > > > Forcing the addition of a connection-id table to all those > > > > > drivers not only is needless churn / boilerplate, but also > > > > > gives > > > > > the > > > > > false impression that we actually have more info (a valid > > > > > connection > > > > > id) then we really have. > > > > > > > > Again, we have no idea what exactly we get if we call > > > > gpiod_get_index(..., NULL, index); in case of ACPI. It would > > > > work > > > > *if > > > > and only if* we assume that all versions of the same device on > > > > all > > > > possible platforms will have *very same* mapping. > > > > > > > > I have heard it's already not true. Check the commit > > > > 89ab37b489d1 > > > > ("Bluetooth: hci_bcm: Add support for BCM2E95 and BCM2E96"). > > > > Any comment / remark on this? > > Looking at that commit, the indxex order is consistent for a single > device HID/CID but at some point in time the decided to change > the order for newer HIDs which sucks from a driver pov, but is > less bad then it could have been (they could have different orders > for the same HID and we would need to fallback to dmi quirks). > > But I fail to see how this is really relevant for this discussion. My point is that the chaos with connection ID vs. label will not be simple suppressed by adding a parameter to API. In some cases it wouldn't be enough. > > > > 2) using labels different to connection ID sounds not okay for > > > > debugging > > > > purposes (I dunno why we have this done for > > > > fwnode_get_gpiod_child() > > > > in > > > > the first place); > > > > > > Right, which is why the _label variants would not allow a > > > connection_id > > > at all using an _label variants implies connection_id == NULL. > > > > > > And using a variant with connection_id argument implies label > > > = connection_id. > > > > > > > 3) additionally different labels will not play good in _DSD > > > > enabled > > > > case, since we must use connection ID there (we believe firmware > > > > until > > > > otherwise is proven). > > > > > > Again, gpios would have a connection_id OR a label, so in > > > _DSD case only a connection_id. > > > > > > > 4) if some firmwares have different indexes for the same device > > > > we > > > > will > > > > need to have device ID (PCI ID, ... or alike) to _CRS index > > > > mapping > > > > anyway in the code. > > > > > > This problem will exist independent of which solution we choose. > > > > Yes. > > > > > > > > > > > > > > Mika, Linus, I would really appreciate your input to the > > > > > > topic: > > > > > > opinions, critique, ideas, etc. > > > > > > > > > > So my opinion on this is that I prefer the add a label field > > > > > idea > > > > > over > > > > > the everything must have either a connection-id in ACPI or a > > > > > connection-id-table in the driver. > > > > > > > > As a ultimate workaround it would work, in long-term prospective > > > > it's > > > > not a solution. > > > I believe that this will work fine even in the long run, better > > > then > > > > See above about bluetooth case. > > > > > forcible adding fake _DSD tables everywhere, see above. > > > > Why are they fake? > > Because they are not coming from the firmware, They actually are. The problem here is that older firmwares and ACPI specification lack of naming GPIO resources. So, this information is complimentary to the existing GPIO resources. It's not fake. > as said I believe it > is better to clearly differentiate between the no-connection-id (so we > use indexes) and the firmware provides connection-ids cases. Indexes without label is error prone approach. Sorry, I'm not going to vote for them. This is explained in the patches I have: we never know what we get by index. The mapping makes it robust. It was clearly my mistake to even think in this direction. > I believe this is better for 2 reasons: > > 1) Cleaner (and less) code for drivers which need to use indexes Yes and no. Cleaner, indeed, but less code does not always mean less error prone, more robust, etc. > 2) It is easier to debug things if it is clear at all levels if we > are dealing with indexes or connection ids And my point that adding labels along with connection IDs is a hiding of a huge abuse of at least ACPI case! It will not get rid of the chaos we have. And it will make API more confusing.
Hi, On 07-03-17 12:51, Andy Shevchenko wrote: > On Mon, 2017-03-06 at 10:31 +0100, Hans de Goede wrote: <mega snip> >>>> forcible adding fake _DSD tables everywhere, see above. >>> >>> Why are they fake? >> >> Because they are not coming from the firmware, > > They actually are. The problem here is that older firmwares and ACPI > specification lack of naming GPIO resources. So, this information is > complimentary to the existing GPIO resources. It's not fake. > >> as said I believe it >> is better to clearly differentiate between the no-connection-id (so we >> use indexes) and the firmware provides connection-ids cases. > > Indexes without label is error prone approach. Sorry, I'm not going to > vote for them. This is explained in the patches I have: we never know > what we get by index. The mapping makes it robust. > > It was clearly my mistake to even think in this direction. > >> I believe this is better for 2 reasons: >> >> 1) Cleaner (and less) code for drivers which need to use indexes > > Yes and no. Cleaner, indeed, but less code does not always mean less > error prone, more robust, etc. > >> 2) It is easier to debug things if it is clear at all levels if we >> are dealing with indexes or connection ids > > And my point that adding labels along with connection IDs is a hiding of > a huge abuse of at least ACPI case! It will not get rid of the chaos we > have. And it will make API more confusing. Ok, since it seems clear that I'm not going to be able to change your mind on this, I will give your patches a try and see if they fix the silead ts problems. 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
Hi, On 07-03-17 14:55, Hans de Goede wrote: > Hi, <more snip> > Ok, since it seems clear that I'm not going to be able to change your > mind on this, I will give your patches a try and see if they fix the > silead ts problems. So I've cherry picked all the gpio related patches from your topic/uart/rpm branch into my wip branch and then ran some tests. I did not get around to actually test if the fix the silead issue (I believe they will) as I started testing on a cht device and looking if soc_button_array still works with your patches applied. Unfortunately it no longer works, there are 2 problems: 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should also replace: desc = gpiod_get_index(dev, info->name, info->acpi_index, GPIOD_ASIS); with: desc = gpiod_get(dev, info->name, GPIOD_ASIS); At which point we can also drop the acpi_index field from the buttoninfo struct altogether. I think that "extcon: int3496: Add GPIO ACPI mapping table" will need a similar change (I haven't tested it yet). 2) acpi_gpio_count() does not seem to work right in combination with the new patches. It returns -ENOENT rather then the number of gpios specified in the table passed to devm_acpi_dev_add_driver_gpios. It seems to only check for gpios actually in the acpi-properties without looking at adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for that and disallows fallback to counting the gpios in the _CRS causing acpi_gpio_count() to not find any gpios. I believe the right fix for this is to make acpi_gpio_count() also count the number of entries in the adev->driver_gpios table. For now I've just removed the acpi_gpio_count() check from soc_button_array, with that removed and 1) fixed soc_button_array does work. I will try to do some more testing later today, but all my cht work is a side project and I first need to finish some stuff for my actual main $dayjob project. 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
On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: > Hi, > > On 07-03-17 14:55, Hans de Goede wrote: > > Hi, > > <more snip> Thanks for looking into this! My comments below. > > Ok, since it seems clear that I'm not going to be able to change > > your > > mind on this, I will give your patches a try and see if they fix the > > silead ts problems. > > So I've cherry picked all the gpio related patches from your > topic/uart/rpm > branch into my wip branch and then ran some tests. Some of them are WIP, so, they might break something as well. > I did not get around > to actually test if the fix the silead issue (I believe they will) as > I > started testing on a cht device and looking if soc_button_array still > works with your patches applied. > > Unfortunately it no longer works, there are 2 problems: > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should > also replace: > > desc = gpiod_get_index(dev, info->name, info->acpi_index, > GPIOD_ASIS); > > with: > > desc = gpiod_get(dev, info->name, GPIOD_ASIS); > > At which point we can also drop the acpi_index field from the > buttoninfo struct > altogether. > I was thinking about passing NULL as connection ID there as it's done for surface3 button array driver. In current soc-button-array we have file name passed for all of the pins, which is slightly informative. > I think that "extcon: int3496: Add GPIO ACPI mapping table" will need > a similar change (I haven't tested it yet). The mapping table converts Linux index, which you pass via gpiod_get_index(), and _CRS index pair (resource, index in a list). If it doesn't work that way, there is another bug then. > 2) acpi_gpio_count() does not seem to work right in combination with > the > new patches. It returns -ENOENT rather then the number of gpios > specified > in the table passed to devm_acpi_dev_add_driver_gpios. It seems to > only > check for gpios actually in the acpi-properties without looking at > adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for > that and disallows fallback to counting the gpios in the _CRS causing > acpi_gpio_count() to not find any gpios. I believe the right fix for > this is to make acpi_gpio_count() also count the number of entries > in the adev->driver_gpios table. > Thanks for catching this, it sounds indeed as a bug. > For now I've just removed the acpi_gpio_count() check from > soc_button_array, > with that removed and 1) fixed soc_button_array does work. > > I will try to do some more testing later today, but all my cht work is > a side project and I first need to finish some stuff for my actual > main $dayjob project. Understood.
Hi, On 08-03-17 11:30, Andy Shevchenko wrote: > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: >> Hi, >> >> On 07-03-17 14:55, Hans de Goede wrote: >>> Hi, >> >> <more snip> > > Thanks for looking into this! > My comments below. > >>> Ok, since it seems clear that I'm not going to be able to change >>> your >>> mind on this, I will give your patches a try and see if they fix the >>> silead ts problems. >> >> So I've cherry picked all the gpio related patches from your >> topic/uart/rpm >> branch into my wip branch and then ran some tests. > > Some of them are WIP, so, they might break something as well. Understood. >> I did not get around >> to actually test if the fix the silead issue (I believe they will) as >> I >> started testing on a cht device and looking if soc_button_array still >> works with your patches applied. >> >> Unfortunately it no longer works, there are 2 problems: >> >> 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should >> also replace: >> >> desc = gpiod_get_index(dev, info->name, info->acpi_index, >> GPIOD_ASIS); >> >> with: >> >> desc = gpiod_get(dev, info->name, GPIOD_ASIS); >> >> At which point we can also drop the acpi_index field from the >> buttoninfo struct >> altogether. >> > > I was thinking about passing NULL as connection ID there as it's done > for surface3 button array driver. In current soc-button-array we have > file name passed for all of the pins, which is slightly informative. No currently the button name, so "power", "volume-up", "volume-down" gets passed. The only place where the file-name used to get passed is in the gpiod_count call, but you've replaced it with NULL there. But passing NULL and removing the need for a mapping table is fine with me. >> I think that "extcon: int3496: Add GPIO ACPI mapping table" will need >> a similar change (I haven't tested it yet). > > The mapping table converts Linux index, which you pass via > gpiod_get_index(), and _CRS index pair (resource, index in a list). > > If it doesn't work that way, there is another bug then. Hmm, so maybe this: static const struct acpi_gpio_params power_gpios = { 0, 0, false }; static const struct acpi_gpio_params home_gpios = { 1, 0, false }; static const struct acpi_gpio_params volume_up_gpios = { 2, 0, false }; static const struct acpi_gpio_params volume_down_gpios = { 3, 0, false }; static const struct acpi_gpio_params rotation_lock_gpios = { 4, 0, false }; Needs to be like this then? : static const struct acpi_gpio_params power_gpios = { 0, 0, false }; static const struct acpi_gpio_params home_gpios = { 1, 1, false }; static const struct acpi_gpio_params volume_up_gpios = { 2, 2, false }; static const struct acpi_gpio_params volume_down_gpios = { 3, 3, false }; static const struct acpi_gpio_params rotation_lock_gpios = { 4, 4, false }; In order for gpiod_get_index() to work ? Note that if we are going to use the mapping table I believe we really should just use gpiod_get (instead of gpiod_get_index) as the map also provides a name so the index is not necessary (I've tested that using gpiod_get() works fine with your current code). >> 2) acpi_gpio_count() does not seem to work right in combination with >> the >> new patches. It returns -ENOENT rather then the number of gpios >> specified >> in the table passed to devm_acpi_dev_add_driver_gpios. It seems to >> only >> check for gpios actually in the acpi-properties without looking at >> adev->driver_gpios, where as acpi_can_fallback_to_crs() does check for >> that and disallows fallback to counting the gpios in the _CRS causing >> acpi_gpio_count() to not find any gpios. I believe the right fix for >> this is to make acpi_gpio_count() also count the number of entries >> in the adev->driver_gpios table. >> > > Thanks for catching this, it sounds indeed as a bug. > >> For now I've just removed the acpi_gpio_count() check from >> soc_button_array, >> with that removed and 1) fixed soc_button_array does work. >> >> I will try to do some more testing later today, but all my cht work is >> a side project and I first need to finish some stuff for my actual >> main $dayjob project. > > Understood. 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
On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote: > On 08-03-17 11:30, Andy Shevchenko wrote: > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: > > > On 07-03-17 14:55, Hans de Goede wrote: > > > I did not get around > > > to actually test if the fix the silead issue (I believe they will) > > > as > > > I > > > started testing on a cht device and looking if soc_button_array > > > still > > > works with your patches applied. > > > > > > Unfortunately it no longer works, there are 2 problems: > > > > > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table" should > > > also replace: > > > > > > desc = gpiod_get_index(dev, info->name, info->acpi_index, > > > GPIOD_ASIS); > > > > > > with: > > > > > > desc = gpiod_get(dev, info->name, GPIOD_ASIS); > > > > > > At which point we can also drop the acpi_index field from the > > > buttoninfo struct > > > altogether. > > > > > > > I was thinking about passing NULL as connection ID there as it's > > done > > for surface3 button array driver. In current soc-button-array we > > have > > file name passed for all of the pins, which is slightly informative. > > No currently the button name, so "power", "volume-up", "volume-down" > gets passed. Because of my WIP patches? They have FIXME: in commit message where I'm in doubt on the way to go. > The only place where the file-name used to get passed > is in the gpiod_count call, but you've replaced it with NULL there. Yes. > But passing NULL and removing the need for a mapping table is fine > with me. NULL sounds to me a bit clearer in this case, since the original name of connection IDs with underscores, not dashes. > > > I think that "extcon: int3496: Add GPIO ACPI mapping table" will > > > need > > > a similar change (I haven't tested it yet). > > > > The mapping table converts Linux index, which you pass via > > gpiod_get_index(), and _CRS index pair (resource, index in a list). > > > > If it doesn't work that way, there is another bug then. > > Hmm, so maybe this: > > static const struct acpi_gpio_params power_gpios = { 0, 0, false }; > static const struct acpi_gpio_params home_gpios = { 1, 0, false }; > Needs to be like this then? : > > static const struct acpi_gpio_params power_gpios = { 0, 0, false }; > static const struct acpi_gpio_params home_gpios = { 1, 1, false }; Obviously not. Just really small pseudo ASL to consider: _CRS: GpioIo(...) { pin #5 } GpioIo(...) { pin #3, pin #4, pin #2 } GpioIo(...) { pin #15 } In Linux (for example) [index, connection ID]: index 0 "reset" (pin #2) index 1 "func1" (pin #4) index 2 "func2" (pin #3) index 3 "enable" (pin #5) index 4 "ready" (pin #15) Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table): index 0 pin #2 to 1,2 index 1 pin #4 to 1,1 index 2 pin #3 to 1,0 index 3 pin #5 to 0,0 index 4 pin #15 to 2,0 > In order for gpiod_get_index() to work ? Note that if we are going > to use the mapping table I believe we really should just use > gpiod_get (instead of gpiod_get_index) as the map also provides a > name so the index is not necessary (I've tested that using > gpiod_get() works fine with your current code). See above, I think it makes picture clearer to understand the problems we have.
On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote: > On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote: > > On 08-03-17 11:30, Andy Shevchenko wrote: > > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: > > > > On 07-03-17 14:55, Hans de Goede wrote: > > > > Unfortunately it no longer works, there are 2 problems: > > > > > > > > 1) "Input: soc_button_array - Add GPIO ACPI mapping table" > > > > should > > > > also replace: > > > > > > > > desc = gpiod_get_index(dev, info->name, info- > > > > >acpi_index, > > > > GPIOD_ASIS); > > > > > > > > with: > > > > > > > > desc = gpiod_get(dev, info->name, GPIOD_ASIS); > > > > > > > > At which point we can also drop the acpi_index field from the > > > > buttoninfo struct > > > > altogether. > Obviously not. However this statement still true, you seems to be right about indexes. What a horrible mess :-) Below corrections to my initial view. > Just really small pseudo ASL to consider: > > _CRS: > > GpioIo(...) { pin #5 } > GpioIo(...) { pin #3, pin #4, pin #2 } > GpioIo(...) { pin #15 } > > In Linux (for example) [index, connection ID]: > > index 0 "reset" (pin #2) > index 1 "func1" (pin #4) > index 2 "func2" (pin #3) An this is completely reversed, should be index 2 "reset" (pin #2) index 1 "func1" (pin #4) index 0 "func2" (pin #3) > index 3 "enable" (pin #5) > index 4 "ready" (pin #15) Both above should have indexes 0 on Linux side! > Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table): > > index 0 pin #2 to 1,2 > index 1 pin #4 to 1,1 > index 2 pin #3 to 1,0 > index 3 pin #5 to 0,0 > index 4 pin #15 to 2,0 Ditto. So, basically with GPIO ACPI mapping table we have to replace indexes in most cases to 0, which effectively means drop of _index() variant of gpiod_get() calls. And here you are right. Sorry for my broken picture.
Hi, On 08-03-17 18:01, Andy Shevchenko wrote: > On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote: >> On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote: >>> On 08-03-17 11:30, Andy Shevchenko wrote: >>>> On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: >>>>> On 07-03-17 14:55, Hans de Goede wrote: > >>>>> Unfortunately it no longer works, there are 2 problems: >>>>> >>>>> 1) "Input: soc_button_array - Add GPIO ACPI mapping table" >>>>> should >>>>> also replace: >>>>> >>>>> desc = gpiod_get_index(dev, info->name, info- >>>>>> acpi_index, >>>>> GPIOD_ASIS); >>>>> >>>>> with: >>>>> >>>>> desc = gpiod_get(dev, info->name, GPIOD_ASIS); >>>>> >>>>> At which point we can also drop the acpi_index field from the >>>>> buttoninfo struct >>>>> altogether. > >> Obviously not. > > However this statement still true, you seems to be right about indexes. > What a horrible mess :-) > > Below corrections to my initial view. > >> Just really small pseudo ASL to consider: >> >> _CRS: >> >> GpioIo(...) { pin #5 } >> GpioIo(...) { pin #3, pin #4, pin #2 } >> GpioIo(...) { pin #15 } >> >> In Linux (for example) [index, connection ID]: >> > >> index 0 "reset" (pin #2) >> index 1 "func1" (pin #4) >> index 2 "func2" (pin #3) > > An this is completely reversed, should be > > index 2 "reset" (pin #2) > index 1 "func1" (pin #4) > index 0 "func2" (pin #3) > >> index 3 "enable" (pin #5) >> index 4 "ready" (pin #15) > > Both above should have indexes 0 on Linux side! > >> Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table): >> >> index 0 pin #2 to 1,2 >> index 1 pin #4 to 1,1 >> index 2 pin #3 to 1,0 >> index 3 pin #5 to 0,0 >> index 4 pin #15 to 2,0 > > Ditto. > > So, basically with GPIO ACPI mapping table we have to replace indexes in > most cases to 0, which effectively means drop of _index() variant of > gpiod_get() calls. And here you are right. Ok, that explains why switching to gpiod_get() fixed things for me. But I do believe that just not using a GPIO ACPI mapping table at all for the soc_button_array driver, combined with using gpiod_get_index() with a NULL con-id as you suggested is the best solution for the soc_button_array driver. 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
On Wed, 2017-03-08 at 19:01 +0200, Andy Shevchenko wrote: > On Wed, 2017-03-08 at 13:46 +0200, Andy Shevchenko wrote: > > On Wed, 2017-03-08 at 12:27 +0100, Hans de Goede wrote: > > > On 08-03-17 11:30, Andy Shevchenko wrote: > > > > On Wed, 2017-03-08 at 10:08 +0100, Hans de Goede wrote: > > > > > On 07-03-17 14:55, Hans de Goede wrote: > Below corrections to my initial view. > > > Just really small pseudo ASL to consider: > > > > _CRS: > > > > GpioIo(...) { pin #5 } > > GpioIo(...) { pin #3, pin #4, pin #2 } > > GpioIo(...) { pin #15 } > > > > In Linux (for example) [index, connection ID]: > > > > index 0 "reset" (pin #2) > > index 1 "func1" (pin #4) > > index 2 "func2" (pin #3) > > An this is completely reversed, should be > > index 2 "reset" (pin #2) > index 1 "func1" (pin #4) > index 0 "func2" (pin #3) zOMG, since we have *different* connection IDs, they are all 0!!! I'm really messing up the things in my brain :-( > > > index 3 "enable" (pin #5) > > index 4 "ready" (pin #15) > > Both above should have indexes 0 on Linux side! > > > Mapping Linux <-> _CRS (either from _DSD or hard coded mapping > > table): > > > > index 0 pin #2 to 1,2 > > index 1 pin #4 to 1,1 > > index 2 pin #3 to 1,0 > > index 3 pin #5 to 0,0 > > index 4 pin #15 to 2,0 > > Ditto. Ditto. -- 8< -- 8< -- Alter scheme to see the different indexes on Linux side is In Linux (for example) [index, connection ID]: index 2 "reset" (pin #2) index 1 "reset" (pin #4) index 0 "reset" (pin #3) Mapping Linux <-> _CRS (either from _DSD or hard coded mapping table): index 2 pin #2 to 1,2 index 1 pin #4 to 1,1 index 0 pin #3 to 1,0 -- 8< -- 8< -- > Sorry for my broken picture. Sorry again.
On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com> wrote: >> I suppose the Windows driver just works everywhere, right? > > Not really, no. > AFAIK every vendor builds their own driver using a special configuration > utility from Silead. > All the device-specific data, including panel parameters and calibration > data, is compiled into this driver. Isn't that just defeating the whole purpose of ACPI (or any other hardware description)? Isn't the idea to describe all this in ACPI tables, and that is what the vendor should be doing rather than compiling in hardcoded things into drivers? I just see this as another sign that the ACPI "ecosystem" is not really working because vendors choose to arbitrarily bypass it like this. Is it too hard for them to use ACPI or is ACPI lacking the right parameters to tweak or what is the real problem? Yours, Linus Walleij -- 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 14-03-17 11:21, Linus Walleij wrote: > On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com> wrote: > >>> I suppose the Windows driver just works everywhere, right? >> >> Not really, no. >> AFAIK every vendor builds their own driver using a special configuration >> utility from Silead. >> All the device-specific data, including panel parameters and calibration >> data, is compiled into this driver. > > Isn't that just defeating the whole purpose of ACPI (or any other > hardware description)? > > Isn't the idea to describe all this in ACPI tables, and that is what the > vendor should be doing rather than compiling in hardcoded things > into drivers? > > I just see this as another sign that the ACPI "ecosystem" is not > really working because vendors choose to arbitrarily bypass it like > this. > > Is it too hard for them to use ACPI or is ACPI lacking the right > parameters to tweak or what is the real problem? It is certainly possible to do this in a more sane manner using ACPI, if I were to guess what the real problem is it is a combination of time-to-marker + getting what you pay for (pay peanuts ...). 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
On Tue, 2017-03-14 at 11:21 +0100, Linus Walleij wrote: > On Thu, Feb 2, 2017 at 3:24 PM, Gregor Riepl <onitake@gmail.com> > wrote: > > > > I suppose the Windows driver just works everywhere, right? > > > > Not really, no. > > AFAIK every vendor builds their own driver using a special > > configuration > > utility from Silead. > > All the device-specific data, including panel parameters and > > calibration > > data, is compiled into this driver. > > Isn't that just defeating the whole purpose of ACPI (or any other > hardware description)? > > Isn't the idea to describe all this in ACPI tables, and that is what > the > vendor should be doing rather than compiling in hardcoded things > into drivers? > I just see this as another sign that the ACPI "ecosystem" is not > really working because vendors choose to arbitrarily bypass it like > this. > > Is it too hard for them to use ACPI or is ACPI lacking the right > parameters to tweak or what is the real problem? With _DSD is possible to enhance ACPI tables to cover almost everything, but there is another issue(s): a) we still have to cope with old firmwares, b) Windows is doing things differently, and c) some vendors are abusing ACPI (existing!) standards. Here is a dilemma: make user's life miserable with hardware they bought, or make developers' life miserable to support all that. And the winner is... vendor which is abusing whatever it wants to! One recent example I found is a Kontron xBTI x86 board (Linux compatible!) where they abused ACPI by creating a table that even iasl can't decode (not a major issue, the table name is just non-standard, but nevertheless).
> Isn't that just defeating the whole purpose of ACPI (or any other > hardware description)? > > Isn't the idea to describe all this in ACPI tables, and that is what the > vendor should be doing rather than compiling in hardcoded things > into drivers? > > I just see this as another sign that the ACPI "ecosystem" is not > really working because vendors choose to arbitrarily bypass it like > this. I hear you... If you want my opinion - in this case it wasn't just a chip manufacturer that didn't understand or care about the ecosystem, it was also OEMs that didn't give a shit and Intel that failed to grasp the problem and counteract. But there's no point in arguing about it, it is how it is and we have to deal with the fallout. What we can do is provide better guidance for inexperienced Chinese manufacturers and lobby Intel so they take more responsibility for their platforms. A single person can't do much, but I do think that a vital open source project like the Linux kernel has some leverage. -- 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 404830a..2fbcd7f 100644 --- a/drivers/input/touchscreen/silead.c +++ b/drivers/input/touchscreen/silead.c @@ -31,6 +31,7 @@ #include <linux/irq.h> #include <linux/regulator/consumer.h> +#include <acpi/acpi_bus.h> #include <asm/unaligned.h> #define SILEAD_TS_NAME "silead_ts" @@ -494,12 +495,21 @@ static int silead_ts_probe(struct i2c_client *client, if (error) return error; - /* Power GPIO pin */ - data->gpio_power = devm_gpiod_get_optional(dev, "power", GPIOD_OUT_LOW); - if (IS_ERR(data->gpio_power)) { - if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) - dev_err(dev, "Shutdown GPIO request failed\n"); - return PTR_ERR(data->gpio_power); + /* + * If device power is not managed by ACPI, get the power_gpio + * and manage it ourselves. + */ +#ifdef CONFIG_ACPI + if (!acpi_bus_power_manageable(ACPI_HANDLE(dev))) +#endif + { + data->gpio_power = devm_gpiod_get_optional(dev, "power", + GPIOD_OUT_LOW); + if (IS_ERR(data->gpio_power)) { + if (PTR_ERR(data->gpio_power) != -EPROBE_DEFER) + dev_err(dev, "Power GPIO request failed\n"); + return PTR_ERR(data->gpio_power); + } } error = silead_ts_setup(client);
On some x86 tablets we cannot directly access the GPIOs as they are claimed by the ACPI tables, so check it the i2c client is not being power-managed by ACPI before trying to get the power pin GPIO. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Check acpi_bus_power_manageable() instead of trying to directly control the acpi power level ourselves --- drivers/input/touchscreen/silead.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)