From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:46:30
Currently GPIO ACPI library provides an API to get a GPIO resources
(IO or interrupt) from ACPI tables for the individual drivers.
This library has few flaws which makes some devices not working:
- the library allows to abuse ACPI by using a _CRS fallback mechanism
- the library neglects flags of the resource
In this series:
- the _CRS fallback is forbidden
- the pin configuration follows what firmware wants to
- the documentation is updated in order to clarify corner cases
After this series it's possible to use GPIO pins for input (interrupt)
which were configured as output by BIOS by some reason. It's a crucial
functionality for IoT open connected boards where user may choose any of
available pin for almost any of available function, including GPIO input
(interrupt).
Current bad behaviour was first reported by Jarkko Nikula few months ago.
Andy Shevchenko (8):
gpiolib: Export gpiod_configure_flags() to internal users
gpio: acpi: Align acpi_find_gpio() with DT version
gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()
gpio: acpi: Even more tighten up ACPI GPIO lookups
gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()
gpio: acpi: Explain how to get GPIO descriptors in ACPI case
gpio: acpi: Factor out acpi_gpio_to_gpiod_flags() helper
gpio: acpi: Override GPIO initialization flags
Documentation/acpi/gpio-properties.txt | 60 ++++++++++++
drivers/gpio/gpiolib-acpi.c | 162 +++++++++++++++++++--------------
drivers/gpio/gpiolib.c | 10 +-
drivers/gpio/gpiolib.h | 17 +++-
4 files changed, 176 insertions(+), 73 deletions(-)
--
2.11.0
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:46:27
By some reason acpi_find_gpio() and acpi_gpio_count() have compared connection
ID to "gpios" when tries to check if suffix is needed or not.
Don't do any assumptions about what connection ID can be and, when defined, use
it only with suffix as it's done in the device tree version.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -721,6 +761,7 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)returnPTR_ERR(desc);if(info.gpioint&&idx++==index){+charlabel[32];intirq;if(IS_ERR(desc))
@@ -730,6 +771,11 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)if(irq<0)returnirq;+snprintf(label,sizeof(label),"GpioInt() %d",index);+ret=gpiod_configure_flags(desc,label,0,info.flags);+if(ret<0)+returnret;+irq_flags=acpi_dev_get_irq_type(info.triggering,info.polarity);
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:46:58
Documentation lacks of explanation how we actually use device properties
for GPIO resources.
Add a section to the documentation about that.
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
@@ -156,3 +156,63 @@ pointed to by its first argument. That should be done in the driver's .probe() routine. On removal, the driver should unregister its GPIO mapping table by calling acpi_dev_remove_driver_gpios() on the ACPI device object where that table was previously registered.++Using the _CRS fallback+-----------------------++If a device does not have _DSD or the driver does not create ACPI GPIO+mapping, the Linux GPIO framework refuses to return any GPIOs. This is+because the driver does not know what it actually gets. For example if we+have a device like below:++ Device (BTH)+ {+ Name (_HID, ...)++ Name (_CRS, ResourceTemplate () {+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,+ "\\_SB.GPO0", 0, ResourceConsumer) {15}+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,+ "\\_SB.GPO0", 0, ResourceConsumer) {27}+ })+ }++The driver might expect to get the right GPIO when it does:++ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);++but since there is no way to know the mapping between "reset" and+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).++The driver author can solve this by passing the mapping explictly+(the recommended way and documented in the above chapter).++Getting GPIO descriptor+-----------------------++There are two main approaches to get GPIO resource from ACPI:+ desc = gpiod_get(dev, connection_id, flags);+ desc = gpiod_get_index(dev, connection_id, index, flags);++We may consider two different cases here, i.e. when connection ID is+provided and otherwise.++Case 1:+ desc = gpiod_get(dev, "non-null-connection-id", flags);+ desc = gpiod_get_index(dev, "non-null-connection-id", index, flags);++Case 2:+ desc = gpiod_get(dev, NULL, flags);+ desc = gpiod_get_index(dev, NULL, index, flags);++Case 1 assumes that corresponding ACPI device description must have+defined device properties and will prevent to getting any GPIO resources+otherwise.++Case 2 explicitly tells GPIO core to look for resources in _CRS.++Be aware that gpiod_get_index() in cases 1 and 2, assuming that there+are two versions of ACPI device description provided and no mapping is+present in the driver, will return different resources. That's why a+certain driver has to handle them carefully as explained in previous+chapter.
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:47:01
If we pass connection ID to the both functions and at the same time
acpi_can_fallback_to_crs() returns false we will get different results, i.e.
the number of GPIO resourses returned by acpi_gpio_count() might be not
correct.
Fix this by calling acpi_can_fallback_to_crs() in acpi_gpio_count() before
trying to fallback.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 3 +++
1 file changed, 3 insertions(+)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:47:04
The helper function acpi_gpio_to_gpiod_flags() will be used later to configure
pin properly whenever it's requested.
While here, introduce a checking error code returned by gpiod_configure_flags()
and bail out if it's not okay.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 61 ++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 25 deletions(-)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:47:38
Check that we don't ask for output direction on GpioInt resource in cases with
or without _DSD defined.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-23 19:48:08
The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups")
prevents to getting same resource twice if the driver asks twice using same
connection ID.
But the whole idea of fallback might bring some problems. Imagine the case when
we have two versions of BIOS/hardware where in one _DSD is introduced along
with GPIO resources, but the other one uses just plain GPIO resource for
another purpose
Case 1:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }},
}
})
}
Case 2:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27}
})
}
To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups
for case without connection ID provided.
In the past the issue had been triggered by "use mctrl_gpio helpers" series
[1,2].
Besides above, removal of the main logic of acpi_can_fallback_to_crs()
eliminates a potential memory leak when the same device has been unbound and
bound again.
[1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
[2] https://patchwork.kernel.org/patch/9283745/
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/gpio/gpiolib-acpi.c | 36 +-----------------------------------
1 file changed, 1 insertion(+), 35 deletions(-)
@@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)returncount?count:-ENOENT;}-structacpi_crs_lookup{-structlist_headnode;-structacpi_device*adev;-constchar*con_id;-};--staticDEFINE_MUTEX(acpi_crs_lookup_lock);-staticLIST_HEAD(acpi_crs_lookup_list);-boolacpi_can_fallback_to_crs(structacpi_device*adev,constchar*con_id){-structacpi_crs_lookup*l,*lookup=NULL;-/* Never allow fallback if the device has properties */if(adev->data.properties||adev->driver_gpios)returnfalse;-mutex_lock(&acpi_crs_lookup_lock);--list_for_each_entry(l,&acpi_crs_lookup_list,node){-if(l->adev==adev){-lookup=l;-break;-}-}--if(!lookup){-lookup=kmalloc(sizeof(*lookup),GFP_KERNEL);-if(lookup){-lookup->adev=adev;-lookup->con_id=kstrdup(con_id,GFP_KERNEL);-list_add_tail(&lookup->node,&acpi_crs_lookup_list);-}-}--mutex_unlock(&acpi_crs_lookup_lock);--returnlookup&&-((!lookup->con_id&&!con_id)||-(lookup->con_id&&con_id&&-strcmp(lookup->con_id,con_id)==0));+returncon_id==NULL;}
On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote:
The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups")
prevents to getting same resource twice if the driver asks twice using same
s/same/different/
connection ID.
But the whole idea of fallback might bring some problems. Imagine the case when
we have two versions of BIOS/hardware where in one _DSD is introduced along
with GPIO resources, but the other one uses just plain GPIO resource for
another purpose
Case 1:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }},
}
})
}
Case 2:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27}
})
}
To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups
for case without connection ID provided.
I wonder if this will break Goodix. Irina, Bastien?
In the past the issue had been triggered by "use mctrl_gpio helpers" series
[1,2].
Besides above, removal of the main logic of acpi_can_fallback_to_crs()
eliminates a potential memory leak when the same device has been unbound and
bound again.
Where? We'll reuse lookup table as ACPI device is still the same.
@@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)returncount?count:-ENOENT;}-structacpi_crs_lookup{-structlist_headnode;-structacpi_device*adev;-constchar*con_id;-};--staticDEFINE_MUTEX(acpi_crs_lookup_lock);-staticLIST_HEAD(acpi_crs_lookup_list);-boolacpi_can_fallback_to_crs(structacpi_device*adev,constchar*con_id){-structacpi_crs_lookup*l,*lookup=NULL;-/* Never allow fallback if the device has properties */if(adev->data.properties||adev->driver_gpios)returnfalse;-mutex_lock(&acpi_crs_lookup_lock);--list_for_each_entry(l,&acpi_crs_lookup_list,node){-if(l->adev==adev){-lookup=l;-break;-}-}--if(!lookup){-lookup=kmalloc(sizeof(*lookup),GFP_KERNEL);-if(lookup){-lookup->adev=adev;-lookup->con_id=kstrdup(con_id,GFP_KERNEL);-list_add_tail(&lookup->node,&acpi_crs_lookup_list);-}-}--mutex_unlock(&acpi_crs_lookup_lock);--returnlookup&&-((!lookup->con_id&&!con_id)||-(lookup->con_id&&con_id&&-strcmp(lookup->con_id,con_id)==0));+returncon_id==NULL;}
On Thu, Mar 23, 2017 at 09:46:12PM +0200, Andy Shevchenko wrote:
By some reason acpi_find_gpio() and acpi_gpio_count() have compared connection
ID to "gpios" when tries to check if suffix is needed or not.
Don't do any assumptions about what connection ID can be and, when defined, use
it only with suffix as it's done in the device tree version.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Thu, Mar 23, 2017 at 09:46:13PM +0200, Andy Shevchenko wrote:
Check that we don't ask for output direction on GpioInt resource in cases with
or without _DSD defined.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted hunk
Documentation lacks of explanation how we actually use device properties
for GPIO resources.
Add a section to the documentation about that.
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
@@ -156,3 +156,63 @@ pointed to by its first argument. That should be done in the driver's .probe() routine. On removal, the driver should unregister its GPIO mapping table by calling acpi_dev_remove_driver_gpios() on the ACPI device object where that table was previously registered.++Using the _CRS fallback+-----------------------++If a device does not have _DSD or the driver does not create ACPI GPIO+mapping, the Linux GPIO framework refuses to return any GPIOs. This is+because the driver does not know what it actually gets. For example if we+have a device like below:++ Device (BTH)+ {+ Name (_HID, ...)++ Name (_CRS, ResourceTemplate () {+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,+ "\\_SB.GPO0", 0, ResourceConsumer) {15}+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,+ "\\_SB.GPO0", 0, ResourceConsumer) {27}+ })+ }++The driver might expect to get the right GPIO when it does:++ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);++but since there is no way to know the mapping between "reset" and+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).++The driver author can solve this by passing the mapping explictly+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no idea about
mapping between _CRS GPIOs and names. All such stuff should be hidden in
platform glue (i.e drivers/platform/x86/platform_crap.c).
quoted hunk
++Getting GPIO descriptor+-----------------------++There are two main approaches to get GPIO resource from ACPI:+ desc = gpiod_get(dev, connection_id, flags);+ desc = gpiod_get_index(dev, connection_id, index, flags);++We may consider two different cases here, i.e. when connection ID is+provided and otherwise.++Case 1:+ desc = gpiod_get(dev, "non-null-connection-id", flags);+ desc = gpiod_get_index(dev, "non-null-connection-id", index, flags);++Case 2:+ desc = gpiod_get(dev, NULL, flags);+ desc = gpiod_get_index(dev, NULL, index, flags);++Case 1 assumes that corresponding ACPI device description must have+defined device properties and will prevent to getting any GPIO resources+otherwise.++Case 2 explicitly tells GPIO core to look for resources in _CRS.++Be aware that gpiod_get_index() in cases 1 and 2, assuming that there+are two versions of ACPI device description provided and no mapping is+present in the driver, will return different resources. That's why a+certain driver has to handle them carefully as explained in previous+chapter.
I think that this wording is too x86-centric. We are talking about
consumers of GPIOs here (i.e. drivers), which need unified behavior
between ACPI, DT, and static board properties, they do not really care
about _CRS or _DSD.
Thanks.
--
Dmitry
On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote:
<snip>
quoted
To prevent the possible misconfiguration tighten up even more ACPI
GPIO lookups
for case without connection ID provided.
I wonder if this will break Goodix. Irina, Bastien?
My Goodix tablet has been on the fritz for nearly a year. Hopefully,
32-bit-UEFI support will be available for my preferred distribution
soon, and I'd be able to tell you.
From: Jarkko Nikula <hidden> Date: 2017-03-24 13:51:10
On 03/23/17 21:46, Andy Shevchenko wrote:
After this series it's possible to use GPIO pins for input (interrupt)
which were configured as output by BIOS by some reason. It's a crucial
functionality for IoT open connected boards where user may choose any of
available pin for almost any of available function, including GPIO input
(interrupt).
Current bad behaviour was first reported by Jarkko Nikula few months ago.
It was actually due HW reset default is input buffer disabled and BIOS didn't touch that pin at all if I recall correctly.
For the set:
Tested-by: Jarkko Nikula <redacted>
From: Mika Westerberg <mika.westerberg@linux.intel.com> Date: 2017-03-24 15:58:51
On Thu, Mar 23, 2017 at 09:46:15PM +0200, Andy Shevchenko wrote:
If we pass connection ID to the both functions and at the same time
acpi_can_fallback_to_crs() returns false we will get different results, i.e.
the number of GPIO resourses returned by acpi_gpio_count() might be not
From: Mika Westerberg <mika.westerberg@linux.intel.com> Date: 2017-03-24 16:03:19
On Thu, Mar 23, 2017 at 09:46:10PM +0200, Andy Shevchenko wrote:
Currently GPIO ACPI library provides an API to get a GPIO resources
(IO or interrupt) from ACPI tables for the individual drivers.
This library has few flaws which makes some devices not working:
- the library allows to abuse ACPI by using a _CRS fallback mechanism
- the library neglects flags of the resource
In this series:
- the _CRS fallback is forbidden
- the pin configuration follows what firmware wants to
- the documentation is updated in order to clarify corner cases
After this series it's possible to use GPIO pins for input (interrupt)
which were configured as output by BIOS by some reason. It's a crucial
functionality for IoT open connected boards where user may choose any of
available pin for almost any of available function, including GPIO input
(interrupt).
Current bad behaviour was first reported by Jarkko Nikula few months ago.
Andy Shevchenko (8):
gpiolib: Export gpiod_configure_flags() to internal users
gpio: acpi: Align acpi_find_gpio() with DT version
gpio: acpi: Do sanity check for GpioInt in acpi_find_gpio()
gpio: acpi: Even more tighten up ACPI GPIO lookups
gpio: acpi: Synchronize acpi_find_gpio() and acpi_gpio_count()
gpio: acpi: Explain how to get GPIO descriptors in ACPI case
gpio: acpi: Factor out acpi_gpio_to_gpiod_flags() helper
gpio: acpi: Override GPIO initialization flags
Documentation/acpi/gpio-properties.txt | 60 ++++++++++++
drivers/gpio/gpiolib-acpi.c | 162 +++++++++++++++++++--------------
drivers/gpio/gpiolib.c | 10 +-
drivers/gpio/gpiolib.h | 17 +++-
4 files changed, 176 insertions(+), 73 deletions(-)
Really nice work!
For the whole series,
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On Thu, Mar 23, 2017 at 8:46 PM, Andy Shevchenko
[off-list ref] wrote:
Currently GPIO ACPI library provides an API to get a GPIO resources
(IO or interrupt) from ACPI tables for the individual drivers.
This library has few flaws which makes some devices not working:
- the library allows to abuse ACPI by using a _CRS fallback mechanism
- the library neglects flags of the resource
In this series:
- the _CRS fallback is forbidden
- the pin configuration follows what firmware wants to
- the documentation is updated in order to clarify corner cases
After this series it's possible to use GPIO pins for input (interrupt)
which were configured as output by BIOS by some reason. It's a crucial
functionality for IoT open connected boards where user may choose any of
available pin for almost any of available function, including GPIO input
(interrupt).
Current bad behaviour was first reported by Jarkko Nikula few months ago.
I see there are some outstanding comments in the patches but it looks
OK overall to me, waiting for a v2 before applying, please collect ACKs
and relevant Tested-by:s.
Yours,
Linus Walleij
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-28 16:33:32
On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote:
On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote:
quoted
The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio
lookups")
prevents to getting same resource twice if the driver asks twice
using same
s/same/different/
quoted
connection ID.
Oh, yeah, though it didn't fix a potential issue described below.
quoted
But the whole idea of fallback might bring some problems. Imagine
the case when
we have two versions of BIOS/hardware where in one _DSD is
introduced along
with GPIO resources, but the other one uses just plain GPIO resource
for
another purpose
Case 1:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"some-gpios", Package() {^DEVX, 0, 0, 0
}},
}
})
}
Case 2:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27}
})
}
To prevent the possible misconfiguration tighten up even more ACPI
GPIO lookups
for case without connection ID provided.
I wonder if this will break Goodix. Irina, Bastien?
It would be helpful if anyone can test it.
Btw, which particular driver do you have in mind that might be broken
after such change? Ah, Goodix is a vendor of touchscreens, right?
I'm going through drivers which are using ACPI enumeration and
gpiod_get() API to create mapping tables. I didn't touch drivers/input/
folder yet.
So, potential problems might be there:
drivers/input/touchscreen/elants_i2c.c
drivers/input/touchscreen/goodix.c
drivers/input/touchscreen/melfas_mip4.c
drivers/input/touchscreen/raydium_i2c_ts.c
drivers/input/touchscreen/silead.c
drivers/input/touchscreen/surface3_spi.c
(except silead which Hans tested few times)
quoted
In the past the issue had been triggered by "use mctrl_gpio helpers"
series
[1,2].
Besides above, removal of the main logic of
acpi_can_fallback_to_crs()
eliminates a potential memory leak when the same device has been
unbound and
bound again.
Where? We'll reuse lookup table as ACPI device is still the same.
I used to have issues with unbind/bind cycle with pointer to struct
device * (IIRC platform device), but you probably right since pointer to
struct acpi_device is used here in your change.
I will remove this paragraph from commit message.
Thanks for review!
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-28 16:44:44
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create ACPI
GPIO
+mapping, the Linux GPIO framework refuses to return any GPIOs. This
is
+because the driver does not know what it actually gets. For example
if we
+have a device like below:
+
+ Device (BTH)
+ {
+ Name (_HID, ...)
+
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {15}
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {27}
+ })
+ }
+
+The driver might expect to get the right GPIO when it does:
+
+ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+but since there is no way to know the mapping between "reset" and
+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
+
+The driver author can solve this by passing the mapping explictly
+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no idea
about
mapping between _CRS GPIOs and names. All such stuff should be hidden
in
platform glue (i.e drivers/platform/x86/platform_crap.c).
It might be interpreted that all platform data from all the drivers
should gone. While ideal case should be like this and I totally agree
with you, we are living in non-ideal world, that's why we used to and
continue using some ID-based quirks (PCI enumeration, I2C enumeration,
ACPI enumeration, SPI enumeration, UART enumeration, an so on, so on).
Moreover ACPI comes into ARM(64) world which might have its own troubles
with generating correct tables and we might end up with quirks there.
So, I disagree that here is possible to hide like you said "all such
stuff in ...platform_crap.c".
quoted
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+ desc = gpiod_get(dev, connection_id, flags);
+ desc = gpiod_get_index(dev, connection_id, index, flags);
+
+We may consider two different cases here, i.e. when connection ID
is
+provided and otherwise.
+
+Case 1:
+ desc = gpiod_get(dev, "non-null-connection-id", flags);
+ desc = gpiod_get_index(dev, "non-null-connection-id",
index, flags);
+
+Case 2:
+ desc = gpiod_get(dev, NULL, flags);
+ desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description must have
+defined device properties and will prevent to getting any GPIO
resources
+otherwise.
+
+Case 2 explicitly tells GPIO core to look for resources in _CRS.
+
+Be aware that gpiod_get_index() in cases 1 and 2, assuming that
there
+are two versions of ACPI device description provided and no mapping
is
+present in the driver, will return different resources. That's why
a
+certain driver has to handle them carefully as explained in
previous
+chapter.
I think that this wording is too x86-centric. We are talking about
consumers of GPIOs here (i.e. drivers), which need unified behavior
between ACPI, DT, and static board properties, they do not really care
about _CRS or _DSD.
If the certain driver cares about ACPI enumerated devices it might care
about supporting it disregarding on how new firmware is used (supporting
_DSD or not).
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
On Tue, Mar 28, 2017 at 07:31:24PM +0300, Andy Shevchenko wrote:
On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote:
quoted
The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio
lookups")
prevents to getting same resource twice if the driver asks twice
using same
s/same/different/
quoted
connection ID.
Oh, yeah, though it didn't fix a potential issue described below.
quoted
quoted
But the whole idea of fallback might bring some problems. Imagine
the case when
we have two versions of BIOS/hardware where in one _DSD is
introduced along
with GPIO resources, but the other one uses just plain GPIO resource
for
another purpose
Case 1:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {15}
})
Name (_DSD, Package ()
{
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package ()
{
Package () {"some-gpios", Package() {^DEVX, 0, 0, 0
}},
}
})
}
Case 2:
Device (DEVX)
{
...
Name (_CRS, ResourceTemplate ()
{
GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly,
"\\_SB.GPO0", 0, ResourceConsumer) {27}
})
}
To prevent the possible misconfiguration tighten up even more ACPI
GPIO lookups
for case without connection ID provided.
quoted
I wonder if this will break Goodix. Irina, Bastien?
It would be helpful if anyone can test it.
Btw, which particular driver do you have in mind that might be broken
after such change? Ah, Goodix is a vendor of touchscreens, right?
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted
quoted
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create ACPI
GPIO
+mapping, the Linux GPIO framework refuses to return any GPIOs. This
is
+because the driver does not know what it actually gets. For example
if we
+have a device like below:
+
+ Device (BTH)
+ {
+ Name (_HID, ...)
+
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {15}
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {27}
+ })
+ }
+
+The driver might expect to get the right GPIO when it does:
+
+ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+but since there is no way to know the mapping between "reset" and
+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
+
+The driver author can solve this by passing the mapping explictly
+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no idea
about
mapping between _CRS GPIOs and names. All such stuff should be hidden
in
platform glue (i.e drivers/platform/x86/platform_crap.c).
It might be interpreted that all platform data from all the drivers
should gone. While ideal case should be like this and I totally agree
with you, we are living in non-ideal world, that's why we used to and
continue using some ID-based quirks (PCI enumeration, I2C enumeration,
ACPI enumeration, SPI enumeration, UART enumeration, an so on, so on).
Moreover ACPI comes into ARM(64) world which might have its own troubles
with generating correct tables and we might end up with quirks there.
*gasp* I thought ACPI was the magic that would fix all issues with cure
embedded hacks.
So, I disagree that here is possible to hide like you said "all such
stuff in ...platform_crap.c".
Well, Hans already posted such patch for select x86 platforms with
Silead touchscreens. I am sure these platforms have more warts that
could be added to the same file in platform/x86/...
quoted
quoted
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+ desc = gpiod_get(dev, connection_id, flags);
+ desc = gpiod_get_index(dev, connection_id, index, flags);
+
+We may consider two different cases here, i.e. when connection ID
is
+provided and otherwise.
+
+Case 1:
+ desc = gpiod_get(dev, "non-null-connection-id", flags);
+ desc = gpiod_get_index(dev, "non-null-connection-id",
index, flags);
+
+Case 2:
+ desc = gpiod_get(dev, NULL, flags);
+ desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description must have
+defined device properties and will prevent to getting any GPIO
resources
+otherwise.
+
+Case 2 explicitly tells GPIO core to look for resources in _CRS.
+
+Be aware that gpiod_get_index() in cases 1 and 2, assuming that
there
+are two versions of ACPI device description provided and no mapping
is
+present in the driver, will return different resources. That's why
a
+certain driver has to handle them carefully as explained in
previous
+chapter.
I think that this wording is too x86-centric. We are talking about
consumers of GPIOs here (i.e. drivers), which need unified behavior
between ACPI, DT, and static board properties, they do not really care
about _CRS or _DSD.
If the certain driver cares about ACPI enumerated devices it might care
about supporting it disregarding on how new firmware is used (supporting
_DSD or not).
The drivers might care about ACPI enumerations, but they do not care
about warts of particular platform that chose to implement their ACPI
tables with missing or invalid data. I say that such knowledge should
not go into generic driver, but rather some other entity that woudl fix
up whatever wrong the platform did. It could be an ACPI table override,
or block of code in platform/x86/..., DT overlay, it does not really
matter as long as we do not litter drivers with hacks for random boxes.
Yes, we used to do that (DMI tables, etc), because there was no better
alternative. Now that we have generic device properties, we have better
ways of addressing these issues.
Thanks.
--
Dmitry
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-03-29 15:04:32
On Wed, 2017-03-29 at 00:12 -0700, Dmitry Torokhov wrote:
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
quoted
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted
quoted
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create
ACPI
GPIO
+mapping, the Linux GPIO framework refuses to return any GPIOs.
This
is
+because the driver does not know what it actually gets. For
example
if we
+have a device like below:
+
+ Device (BTH)
+ {
+ Name (_HID, ...)
+
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {15}
+ GpioIo (Exclusive, PullNone, 0, 0, IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {27}
+ })
+ }
+
+The driver might expect to get the right GPIO when it does:
+
+ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+but since there is no way to know the mapping between "reset"
and
+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
+
+The driver author can solve this by passing the mapping
explictly
+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no idea
about
mapping between _CRS GPIOs and names. All such stuff should be
hidden
in
platform glue (i.e drivers/platform/x86/platform_crap.c).
It might be interpreted that all platform data from all the drivers
should gone. While ideal case should be like this and I totally
agree
with you, we are living in non-ideal world, that's why we used to
and
continue using some ID-based quirks (PCI enumeration, I2C
enumeration,
ACPI enumeration, SPI enumeration, UART enumeration, an so on, so
on).
Moreover ACPI comes into ARM(64) world which might have its own
troubles
with generating correct tables and we might end up with quirks
there.
*gasp* I thought ACPI was the magic that would fix all issues with
cure
embedded hacks.
In which version of the spec? I think ACPI r6.2 (anticipating soon)
would have solved a lot of issues regarding GPIO and pin configuration.
I also was and is thinking that ACPI has its own strong sides.
quoted
So, I disagree that here is possible to hide like you said "all such
stuff in ...platform_crap.c".
Well, Hans already posted such patch for select x86 platforms with
Silead touchscreens. I am sure these platforms have more warts that
could be added to the same file in platform/x86/...
So, do we agree on the following paragraph will be added to this
documentation?
"GPIO ACPI mapping tables should not contaminate drivers that are not
knowing about which exact device they are servicing on. It implies that
GPIO ACPI mapping tables are hardly linked to ACPI ID of the device in
question and their location is determined solely by location of the ACPI
ID table."
quoted
quoted
quoted
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+ desc = gpiod_get(dev, connection_id, flags);
+ desc = gpiod_get_index(dev, connection_id, index,
flags);
+
+We may consider two different cases here, i.e. when connection
ID
is
+provided and otherwise.
+
+Case 1:
+ desc = gpiod_get(dev, "non-null-connection-id", flags);
+ desc = gpiod_get_index(dev, "non-null-connection-id",
index, flags);
+
+Case 2:
+ desc = gpiod_get(dev, NULL, flags);
+ desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description must
have
+defined device properties and will prevent to getting any GPIO
resources
+otherwise.
+
+Case 2 explicitly tells GPIO core to look for resources in
_CRS.
+
+Be aware that gpiod_get_index() in cases 1 and 2, assuming that
there
+are two versions of ACPI device description provided and no
mapping
is
+present in the driver, will return different resources. That's
why
a
+certain driver has to handle them carefully as explained in
previous
+chapter.
I think that this wording is too x86-centric. We are talking about
consumers of GPIOs here (i.e. drivers), which need unified
behavior
between ACPI, DT, and static board properties, they do not really
care
about _CRS or _DSD.
If the certain driver cares about ACPI enumerated devices it might
care
about supporting it disregarding on how new firmware is used
(supporting
_DSD or not).
The drivers might care about ACPI enumerations, but they do not care
about warts of particular platform that chose to implement their ACPI
tables with missing or invalid data. I say that such knowledge should
not go into generic driver, but rather some other entity that woudl
fix
up whatever wrong the platform did. It could be an ACPI table
override,
or block of code in platform/x86/..., DT overlay, it does not really
matter as long as we do not litter drivers with hacks for random
boxes.
Yes, we used to do that (DMI tables, etc), because there was no better
alternative. Now that we have generic device properties, we have
better
ways of addressing these issues.
See above.
Otherwise I'm reading something like this:
"If we have platform driverX.c which has DT/platform and ACPI
enumeration, we must split ACPI part out, duplicate a lot of code and
use platform driver as a library."
Is that what you mean?
P.S. This all _CRS fallback shouldn't be allowed in the first place. Now
I'm trying to sort this crap out to nicely work with _DSD enabled
firmwares without breaking devices with *old* firmwares. I see no
other/better way to do such things and I'm open to any _example_ how it
can be done differently.
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-04-04 16:16:01
On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
On Wed, 2017-03-29 at 00:12 -0700, Dmitry Torokhov wrote:
quoted
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
quoted
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted
quoted
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create
ACPI
GPIO
+mapping, the Linux GPIO framework refuses to return any
GPIOs.
This
is
+because the driver does not know what it actually gets. For
example
if we
+have a device like below:
+
+ Device (BTH)
+ {
+ Name (_HID, ...)
+
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullNone, 0, 0,
IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {15}
+ GpioIo (Exclusive, PullNone, 0, 0,
IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {27}
+ })
+ }
+
+The driver might expect to get the right GPIO when it does:
+
+ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+but since there is no way to know the mapping between "reset"
and
+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
+
+The driver author can solve this by passing the mapping
explictly
+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no
idea
about
mapping between _CRS GPIOs and names. All such stuff should be
hidden
in
platform glue (i.e drivers/platform/x86/platform_crap.c).
It might be interpreted that all platform data from all the
drivers
should gone. While ideal case should be like this and I totally
agree
with you, we are living in non-ideal world, that's why we used to
and
continue using some ID-based quirks (PCI enumeration, I2C
enumeration,
ACPI enumeration, SPI enumeration, UART enumeration, an so on, so
on).
Moreover ACPI comes into ARM(64) world which might have its own
troubles
with generating correct tables and we might end up with quirks
there.
*gasp* I thought ACPI was the magic that would fix all issues with
cure
embedded hacks.
In which version of the spec? I think ACPI r6.2 (anticipating soon)
would have solved a lot of issues regarding GPIO and pin
configuration.
I also was and is thinking that ACPI has its own strong sides.
quoted
quoted
So, I disagree that here is possible to hide like you said "all
such
stuff in ...platform_crap.c".
Well, Hans already posted such patch for select x86 platforms with
Silead touchscreens. I am sure these platforms have more warts that
could be added to the same file in platform/x86/...
So, do we agree on the following paragraph will be added to this
documentation?
"GPIO ACPI mapping tables should not contaminate drivers that are not
knowing about which exact device they are servicing on. It implies
that
GPIO ACPI mapping tables are hardly linked to ACPI ID of the device in
question and their location is determined solely by location of the
ACPI
ID table."
quoted
quoted
quoted
quoted
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+ desc = gpiod_get(dev, connection_id, flags);
+ desc = gpiod_get_index(dev, connection_id, index,
flags);
+
+We may consider two different cases here, i.e. when
connection
ID
is
+provided and otherwise.
+
+Case 1:
+ desc = gpiod_get(dev, "non-null-connection-id",
flags);
+ desc = gpiod_get_index(dev, "non-null-connection-id",
index, flags);
+
+Case 2:
+ desc = gpiod_get(dev, NULL, flags);
+ desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description
must
have
+defined device properties and will prevent to getting any
GPIO
resources
+otherwise.
+
+Case 2 explicitly tells GPIO core to look for resources in
_CRS.
+
+Be aware that gpiod_get_index() in cases 1 and 2, assuming
that
there
+are two versions of ACPI device description provided and no
mapping
is
+present in the driver, will return different resources.
That's
why
a
+certain driver has to handle them carefully as explained in
previous
+chapter.
I think that this wording is too x86-centric. We are talking
about
consumers of GPIOs here (i.e. drivers), which need unified
behavior
between ACPI, DT, and static board properties, they do not
really
care
about _CRS or _DSD.
If the certain driver cares about ACPI enumerated devices it might
care
about supporting it disregarding on how new firmware is used
(supporting
_DSD or not).
The drivers might care about ACPI enumerations, but they do not care
about warts of particular platform that chose to implement their
ACPI
tables with missing or invalid data. I say that such knowledge
should
not go into generic driver, but rather some other entity that woudl
fix
up whatever wrong the platform did. It could be an ACPI table
override,
or block of code in platform/x86/..., DT overlay, it does not really
matter as long as we do not litter drivers with hacks for random
boxes.
Yes, we used to do that (DMI tables, etc), because there was no
better
alternative. Now that we have generic device properties, we have
better
ways of addressing these issues.
See above.
Otherwise I'm reading something like this:
"If we have platform driverX.c which has DT/platform and ACPI
enumeration, we must split ACPI part out, duplicate a lot of code and
use platform driver as a library."
Is that what you mean?
P.S. This all _CRS fallback shouldn't be allowed in the first place.
Now
I'm trying to sort this crap out to nicely work with _DSD enabled
firmwares without breaking devices with *old* firmwares. I see no
other/better way to do such things and I'm open to any _example_ how
it
can be done differently.
So, Dmitry, do you have anything to share?
I'm about to submit new version and since we at -rc5, I would really go
with it.
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
On Tue, Apr 04, 2017 at 07:11:17PM +0300, Andy Shevchenko wrote:
On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
quoted
On Wed, 2017-03-29 at 00:12 -0700, Dmitry Torokhov wrote:
quoted
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
quoted
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
quoted
quoted
+Using the _CRS fallback
+-----------------------
+
+If a device does not have _DSD or the driver does not create
ACPI
GPIO
+mapping, the Linux GPIO framework refuses to return any
GPIOs.
This
is
+because the driver does not know what it actually gets. For
example
if we
+have a device like below:
+
+ Device (BTH)
+ {
+ Name (_HID, ...)
+
+ Name (_CRS, ResourceTemplate () {
+ GpioIo (Exclusive, PullNone, 0, 0,
IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {15}
+ GpioIo (Exclusive, PullNone, 0, 0,
IoRestrictionNone,
+ "\\_SB.GPO0", 0, ResourceConsumer) {27}
+ })
+ }
+
+The driver might expect to get the right GPIO when it does:
+
+ desc = gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+
+but since there is no way to know the mapping between "reset"
and
+the GpioIo() in _CRS desc will hold ERR_PTR(-ENOENT).
+
+The driver author can solve this by passing the mapping
explictly
+(the recommended way and documented in the above chapter).
If the driver is not platform specific, then it would have no
idea
about
mapping between _CRS GPIOs and names. All such stuff should be
hidden
in
platform glue (i.e drivers/platform/x86/platform_crap.c).
It might be interpreted that all platform data from all the
drivers
should gone. While ideal case should be like this and I totally
agree
with you, we are living in non-ideal world, that's why we used to
and
continue using some ID-based quirks (PCI enumeration, I2C
enumeration,
ACPI enumeration, SPI enumeration, UART enumeration, an so on, so
on).
Moreover ACPI comes into ARM(64) world which might have its own
troubles
with generating correct tables and we might end up with quirks
there.
*gasp* I thought ACPI was the magic that would fix all issues with
cure
embedded hacks.
In which version of the spec? I think ACPI r6.2 (anticipating soon)
would have solved a lot of issues regarding GPIO and pin
configuration.
I also was and is thinking that ACPI has its own strong sides.
Sorry, that was a dig at someone else ;)
quoted
quoted
quoted
So, I disagree that here is possible to hide like you said "all
such
stuff in ...platform_crap.c".
Well, Hans already posted such patch for select x86 platforms with
Silead touchscreens. I am sure these platforms have more warts that
could be added to the same file in platform/x86/...
So, do we agree on the following paragraph will be added to this
documentation?
"GPIO ACPI mapping tables should not contaminate drivers that are not
knowing about which exact device they are servicing on. It implies
that
GPIO ACPI mapping tables are hardly linked to ACPI ID of the device in
question and their location is determined solely by location of the
ACPI
ID table."
I completely agree with the 1st sentence and disagree about the 2nd. The
ACPI ID usually describes a chip, much like DT compatible does, and
rarely is platform specific. For example, we have i2c-hid binding to
ACPI0C50 or PNP0C50. These are standard IDs shared between many
platforms. Same goes for various Silead IDs; Chromebooks use ATML0000
and ATML0001 for Atmel touch controllers, ELANXXXX for Elan controllers,
etc. All of them might have different connections, depending on platform
(i.e. on given platform firmware might be responsible for powering up
and resetting controller while on another it is done by the kernel). The
driver that is not platform specific should ideally not have
platform-specific knowledge in it, but instead get configuration from
elsewhere (device tree, acpi, board code, or platform driver augmenting
the former 3 sources, like we have with silead_dmi.c).
quoted
quoted
quoted
quoted
quoted
+
+Getting GPIO descriptor
+-----------------------
+
+There are two main approaches to get GPIO resource from ACPI:
+ desc = gpiod_get(dev, connection_id, flags);
+ desc = gpiod_get_index(dev, connection_id, index,
flags);
+
+We may consider two different cases here, i.e. when
connection
ID
is
+provided and otherwise.
+
+Case 1:
+ desc = gpiod_get(dev, "non-null-connection-id",
flags);
+ desc = gpiod_get_index(dev, "non-null-connection-id",
index, flags);
+
+Case 2:
+ desc = gpiod_get(dev, NULL, flags);
+ desc = gpiod_get_index(dev, NULL, index, flags);
+
+Case 1 assumes that corresponding ACPI device description
must
have
+defined device properties and will prevent to getting any
GPIO
resources
+otherwise.
+
+Case 2 explicitly tells GPIO core to look for resources in
_CRS.
+
+Be aware that gpiod_get_index() in cases 1 and 2, assuming
that
there
+are two versions of ACPI device description provided and no
mapping
is
+present in the driver, will return different resources.
That's
why
a
+certain driver has to handle them carefully as explained in
previous
+chapter.
I think that this wording is too x86-centric. We are talking
about
consumers of GPIOs here (i.e. drivers), which need unified
behavior
between ACPI, DT, and static board properties, they do not
really
care
about _CRS or _DSD.
If the certain driver cares about ACPI enumerated devices it might
care
about supporting it disregarding on how new firmware is used
(supporting
_DSD or not).
The drivers might care about ACPI enumerations, but they do not care
about warts of particular platform that chose to implement their
ACPI
tables with missing or invalid data. I say that such knowledge
should
not go into generic driver, but rather some other entity that woudl
fix
up whatever wrong the platform did. It could be an ACPI table
override,
or block of code in platform/x86/..., DT overlay, it does not really
matter as long as we do not litter drivers with hacks for random
boxes.
Yes, we used to do that (DMI tables, etc), because there was no
better
alternative. Now that we have generic device properties, we have
better
ways of addressing these issues.
See above.
Otherwise I'm reading something like this:
"If we have platform driverX.c which has DT/platform and ACPI
enumeration, we must split ACPI part out, duplicate a lot of code and
use platform driver as a library."
No. You need to split the part that augments incomplete ACPI data, and
move it somewhere (drivers/platform/x86/<platform>-crap.c; the driver
stays the same: a driver that is useful across multiple platforms.
quoted
Is that what you mean?
P.S. This all _CRS fallback shouldn't be allowed in the first place.
It does work in many cases. By disallowing it completely you force much
more platform stuff knowledge in the kernel, whereas before you needed
to deal with exceptions.
Thanks.
--
Dmitry
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: 2017-04-04 17:59:28
On Tue, 2017-04-04 at 10:31 -0700, Dmitry Torokhov wrote:
On Tue, Apr 04, 2017 at 07:11:17PM +0300, Andy Shevchenko wrote:
quoted
On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
quoted
On Wed, 2017-03-29 at 00:12 -0700, Dmitry Torokhov wrote:
quoted
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
quoted
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko
wrote:
quoted
quoted
Otherwise I'm reading something like this:
"If we have platform driverX.c which has DT/platform and ACPI
enumeration, we must split ACPI part out, duplicate a lot of code
and
use platform driver as a library."
No. You need to split the part that augments incomplete ACPI data, and
move it somewhere (drivers/platform/x86/<platform>-crap.c; the driver
stays the same: a driver that is useful across multiple platforms.
quoted
quoted
Is that what you mean?
So, it means to spread IDs in two places. Looking into silead_dmi.c I
can say it looks as a hack, we aren't supposed to use "ACPIXXXX:YY" in
the drivers AFAIK. Besides the fact of notifier and arch_initcall().
It indeed feels like a crap and looks like a crap.
Rafael, Mika, what are your opinions about proposed approach?
quoted
quoted
P.S. This all _CRS fallback shouldn't be allowed in the first
place.
It does work in many cases. By disallowing it completely you force
much
more platform stuff knowledge in the kernel, whereas before you needed
to deal with exceptions.
It works due to luck, not otherwise.
--
Andy Shevchenko [off-list ref]
Intel Finland Oy
On Tue, Apr 04, 2017 at 08:59:11PM +0300, Andy Shevchenko wrote:
On Tue, 2017-04-04 at 10:31 -0700, Dmitry Torokhov wrote:
quoted
On Tue, Apr 04, 2017 at 07:11:17PM +0300, Andy Shevchenko wrote:
quoted
On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
quoted
On Wed, 2017-03-29 at 00:12 -0700, Dmitry Torokhov wrote:
quoted
On Tue, Mar 28, 2017 at 07:39:23PM +0300, Andy Shevchenko wrote:
quoted
On Thu, 2017-03-23 at 13:28 -0700, Dmitry Torokhov wrote:
quoted
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko
wrote:
quoted
quoted
quoted
Otherwise I'm reading something like this:
"If we have platform driverX.c which has DT/platform and ACPI
enumeration, we must split ACPI part out, duplicate a lot of code
and
use platform driver as a library."
No. You need to split the part that augments incomplete ACPI data, and
move it somewhere (drivers/platform/x86/<platform>-crap.c; the driver
stays the same: a driver that is useful across multiple platforms.
quoted
quoted
quoted
Is that what you mean?
So, it means to spread IDs in two places.
For completely different purposes, yes. One takes DMI data to identify
platform, and ACPI _instance_ ID to identify the particular ACPI device;
there theoretically could be several of them. If you have a better
option to identify the instance, we can switch to them.
The driver uses HID or CID to bind to one or more devices.
Looking into silead_dmi.c I
can say it looks as a hack, we aren't supposed to use "ACPIXXXX:YY" in
the drivers AFAIK. Besides the fact of notifier and arch_initcall().
It indeed feels like a crap and looks like a crap.
It is supposed to be crap. We have a vendor that neglected to describe
the device in firmware properly and instead expects the driver to be
recompiled for each device shipped. Bad, bad vendor. So we have crap in
platform/x86/... At least we do not smear this shit all over generic
driver.
Rafael, Mika, what are your opinions about proposed approach?
quoted
quoted
quoted
P.S. This all _CRS fallback shouldn't be allowed in the first
place.
It does work in many cases. By disallowing it completely you force
much
more platform stuff knowledge in the kernel, whereas before you needed
to deal with exceptions.
It works due to luck, not otherwise.
As far as I see it works often enough.
Thanks.
--
Dmitry