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
This allows ACPI GPIO code to modify flags based on
ACPI GpioIo() / GpioInt() resources.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 50 +++++++++++++++++++++++++++++++++++++++++++--
drivers/gpio/gpiolib.c | 8 ++++++--
drivers/gpio/gpiolib.h | 15 ++++++++++++--
3 files changed, 67 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d4de84670c5b..5506b0736aba 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -442,6 +442,34 @@ acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
}
}
+int
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+{
+ int ret = 0;
+
+ /*
+ * Check if the BIOS has IoRestriction with explicitly set direction
+ * and update @flags accordingly. Otherwise use whatever caller asked
+ * for.
+ */
+ if (update & GPIOD_FLAGS_BIT_DIR_SET) {
+ enum gpiod_flags diff = *flags ^ update;
+
+ /*
+ * Check if caller supplied incompatible GPIO initialization
+ * flags.
+ *
+ * Return %-EINVAL to notify that firmware has different
+ * settings and we are going to use them.
+ */
+ if (((*flags & GPIOD_FLAGS_BIT_DIR_SET) && (diff & GPIOD_FLAGS_BIT_DIR_OUT)) ||
+ ((*flags & GPIOD_FLAGS_BIT_DIR_OUT) && (diff & GPIOD_FLAGS_BIT_DIR_VAL)))
+ ret = -EINVAL;
+ *flags = update;
+ }
+ return ret;
+}
+
struct acpi_gpio_lookup {
struct acpi_gpio_info info;
int index;
@@ -479,8 +507,11 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
* - ACPI_ACTIVE_HIGH == GPIO_ACTIVE_HIGH
*/
if (lookup->info.gpioint) {
+ lookup->info.flags = GPIOD_IN;
lookup->info.polarity = agpio->polarity;
lookup->info.triggering = agpio->triggering;
+ } else {
+ lookup->info.flags = acpi_gpio_to_gpiod_flags(agpio);
}
}
@@ -607,13 +638,14 @@ static struct gpio_desc *acpi_get_gpiod_by_index(struct acpi_device *adev,
struct gpio_desc *acpi_find_gpio(struct device *dev,
const char *con_id,
unsigned int idx,
- enum gpiod_flags flags,
+ enum gpiod_flags *dflags,
enum gpio_lookup_flags *lookupflags)
{
struct acpi_device *adev = ACPI_COMPANION(dev);
struct acpi_gpio_info info;
struct gpio_desc *desc;
char propname[32];
+ int err;
int i;
/* Try first from _DSD */
@@ -642,7 +674,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
}
if (info.gpioint &&
- (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
+ (*dflags == GPIOD_OUT_LOW || *dflags == GPIOD_OUT_HIGH)) {
dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
return ERR_PTR(-ENOENT);
}
@@ -650,6 +682,10 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
if (info.polarity == GPIO_ACTIVE_LOW)
*lookupflags |= GPIO_ACTIVE_LOW;
+ err = acpi_gpio_update_gpiod_flags(dflags, info.flags);
+ if (err)
+ dev_dbg(dev, "Override GPIO initialization flags\n");
+
return desc;
}
@@ -703,12 +739,16 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
* used to translate from the GPIO offset in the resource to the Linux IRQ
* number.
*
+ * The function is idempotent, though each time it runs it will configure GPIO
+ * pin direction according to the flags in GpioInt resource.
+ *
* Return: Linux IRQ number (>%0) on success, negative errno on failure.
*/
int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
{
int idx, i;
unsigned int irq_flags;
+ int ret;
for (i = 0, idx = 0; idx <= index; i++) {
struct acpi_gpio_info info;
@@ -721,6 +761,7 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
return PTR_ERR(desc);
if (info.gpioint && idx++ == index) {
+ char label[32];
int irq;
if (IS_ERR(desc))
@@ -730,6 +771,11 @@ int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
if (irq < 0)
return irq;
+ snprintf(label, sizeof(label), "GpioInt() %d", index);
+ ret = gpiod_configure_flags(desc, label, 0, info.flags);
+ if (ret < 0)
+ return ret;
+
irq_flags = acpi_dev_get_irq_type(info.triggering,
info.polarity);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 3752785d31ab..6110fd62277a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3272,7 +3272,7 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
desc = of_find_gpio(dev, con_id, idx, &lookupflags);
} else if (ACPI_COMPANION(dev)) {
dev_dbg(dev, "using ACPI for GPIO lookup\n");
- desc = acpi_find_gpio(dev, con_id, idx, flags, &lookupflags);
+ desc = acpi_find_gpio(dev, con_id, idx, &flags, &lookupflags);
}
}
@@ -3351,8 +3351,12 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
struct acpi_gpio_info info;
desc = acpi_node_get_gpiod(fwnode, propname, index, &info);
- if (!IS_ERR(desc))
+ if (!IS_ERR(desc)) {
active_low = info.polarity == GPIO_ACTIVE_LOW;
+ ret = acpi_gpio_update_gpiod_flags(&dflags, info.flags);
+ if (ret)
+ pr_debug("Override GPIO initialization flags\n");
+ }
}
if (IS_ERR(desc))
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index e36a0bdc7740..cff398cbb545 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -75,11 +75,13 @@ struct gpio_device {
/**
* struct acpi_gpio_info - ACPI GPIO specific information
+ * @flags: GPIO initialization flags
* @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
* @polarity: interrupt polarity as provided by ACPI
* @triggering: triggering type as provided by ACPI
*/
struct acpi_gpio_info {
+ enum gpiod_flags flags;
bool gpioint;
int polarity;
int triggering;
@@ -121,10 +123,13 @@ void acpi_gpiochip_remove(struct gpio_chip *chip);
void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
+int acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags,
+ enum gpiod_flags update);
+
struct gpio_desc *acpi_find_gpio(struct device *dev,
const char *con_id,
unsigned int idx,
- enum gpiod_flags flags,
+ enum gpiod_flags *dflags,
enum gpio_lookup_flags *lookupflags);
struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
const char *propname, int index,
@@ -143,9 +148,15 @@ acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
static inline void
acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
+static inline int
+acpi_gpio_update_gpiod_flags(enum gpiod_flags *flags, enum gpiod_flags update)
+{
+ return 0;
+}
+
static inline struct gpio_desc *
acpi_find_gpio(struct device *dev, const char *con_id,
- unsigned int idx, enum gpiod_flags flags,
+ unsigned int idx, enum gpiod_flags *dflags,
enum gpio_lookup_flags *lookupflags)
{
return ERR_PTR(-ENOENT);
--
2.11.0
This is preparatory patch for enabling GPIO ACPI to configure a pin
accordingly.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib.c | 2 +-
drivers/gpio/gpiolib.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4aa1e78f0f0b..3752785d31ab 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3212,7 +3212,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional);
* requested function and/or index, or another IS_ERR() code if an error
* occurred while trying to acquire the GPIO.
*/
-static int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
unsigned long lflags, enum gpiod_flags dflags)
{
int status;
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 2495b7ee1b42..e36a0bdc7740 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -199,6 +199,8 @@ struct gpio_desc {
int gpiod_request(struct gpio_desc *desc, const char *label);
void gpiod_free(struct gpio_desc *desc);
+int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
+ unsigned long lflags, enum gpiod_flags dflags);
int gpiod_hog(struct gpio_desc *desc, const char *name,
unsigned long lflags, enum gpiod_flags dflags);
--
2.11.0
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 <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 61 ++++++++++++++++++++++++++-------------------
1 file changed, 36 insertions(+), 25 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 61518273e360..d4de84670c5b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -417,6 +417,31 @@ static bool acpi_get_driver_gpio_data(struct acpi_device *adev,
return false;
}
+static enum gpiod_flags
+acpi_gpio_to_gpiod_flags(const struct acpi_resource_gpio *agpio)
+{
+ bool pull_up = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
+
+ switch (agpio->io_restriction) {
+ case ACPI_IO_RESTRICT_INPUT:
+ return GPIOD_IN;
+ case ACPI_IO_RESTRICT_OUTPUT:
+ /*
+ * ACPI GPIO resources don't contain an initial value for the
+ * GPIO. Therefore we deduce that value from the pull field
+ * instead. If the pin is pulled up we assume default to be
+ * high, otherwise low.
+ */
+ return pull_up ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW;
+ default:
+ /*
+ * Assume that the BIOS has configured the direction and pull
+ * accordingly.
+ */
+ return GPIOD_ASIS;
+ }
+}
+
struct acpi_gpio_lookup {
struct acpi_gpio_info info;
int index;
@@ -732,7 +757,6 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
struct acpi_resource *ares;
int pin_index = (int)address;
acpi_status status;
- bool pull_up;
int length;
int i;
@@ -747,7 +771,6 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
}
agpio = &ares->data.gpio;
- pull_up = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
function == ACPI_WRITE)) {
@@ -798,35 +821,23 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
}
if (!found) {
- desc = gpiochip_request_own_desc(chip, pin,
- "ACPI:OpRegion");
+ enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio);
+ const char *label = "ACPI:OpRegion";
+ int err;
+
+ desc = gpiochip_request_own_desc(chip, pin, label);
if (IS_ERR(desc)) {
status = AE_ERROR;
mutex_unlock(&achip->conn_lock);
goto out;
}
- switch (agpio->io_restriction) {
- case ACPI_IO_RESTRICT_INPUT:
- gpiod_direction_input(desc);
- break;
- case ACPI_IO_RESTRICT_OUTPUT:
- /*
- * ACPI GPIO resources don't contain an
- * initial value for the GPIO. Therefore we
- * deduce that value from the pull field
- * instead. If the pin is pulled up we
- * assume default to be high, otherwise
- * low.
- */
- gpiod_direction_output(desc, pull_up);
- break;
- default:
- /*
- * Assume that the BIOS has configured the
- * direction and pull accordingly.
- */
- break;
+ err = gpiod_configure_flags(desc, label, 0, flags);
+ if (err < 0) {
+ status = AE_NOT_CONFIGURED;
+ gpiochip_free_own_desc(desc);
+ mutex_unlock(&achip->conn_lock);
+ goto out;
}
conn = kzalloc(sizeof(*conn), GFP_KERNEL);
--
2.11.0
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
index 2aff0349facd..07954b7c3a12 100644
--- a/Documentation/acpi/gpio-properties.txt
+++ b/Documentation/acpi/gpio-properties.txt
@@ -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.
--
2.11.0
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 <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e516b7a0cc50..61518273e360 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1111,6 +1111,9 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
struct list_head resource_list;
unsigned int crs_count = 0;
+ if (!acpi_can_fallback_to_crs(adev, con_id))
+ return count;
+
INIT_LIST_HEAD(&resource_list);
acpi_dev_get_resources(adev, &resource_list,
acpi_find_gpio_count, &crs_count);
--
2.11.0
Check that we don't ask for output direction on GpioInt resource in cases with
or without _DSD defined.
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 3bda3166d418..21e4930ca2db 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -614,12 +614,12 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
if (IS_ERR(desc))
return desc;
+ }
- if ((flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH) &&
- info.gpioint) {
- dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
- return ERR_PTR(-ENOENT);
- }
+ if (info.gpioint &&
+ (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
+ dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
+ return ERR_PTR(-ENOENT);
}
if (info.polarity == GPIO_ACTIVE_LOW)
--
2.11.0
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 <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 56a6b1be3a17..3bda3166d418 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -593,7 +593,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
/* Try first from _DSD */
for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
- if (con_id && strcmp(con_id, "gpios")) {
+ if (con_id) {
snprintf(propname, sizeof(propname), "%s-%s",
con_id, gpio_suffixes[i]);
} else {
@@ -1081,7 +1081,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
/* Try first from _DSD */
for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
- if (con_id && strcmp(con_id, "gpios"))
+ if (con_id)
snprintf(propname, sizeof(propname), "%s-%s",
con_id, gpio_suffixes[i]);
else
--
2.11.0
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 <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpio/gpiolib-acpi.c | 36 +-----------------------------------
1 file changed, 1 insertion(+), 35 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 21e4930ca2db..e516b7a0cc50 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
return count ? count : -ENOENT;
}
-struct acpi_crs_lookup {
- struct list_head node;
- struct acpi_device *adev;
- const char *con_id;
-};
-
-static DEFINE_MUTEX(acpi_crs_lookup_lock);
-static LIST_HEAD(acpi_crs_lookup_list);
-
bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
{
- struct acpi_crs_lookup *l, *lookup = NULL;
-
/* Never allow fallback if the device has properties */
if (adev->data.properties || adev->driver_gpios)
return false;
- 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);
-
- return lookup &&
- ((!lookup->con_id && !con_id) ||
- (lookup->con_id && con_id &&
- strcmp(lookup->con_id, con_id) == 0));
+ return con_id == NULL;
}
--
2.11.0
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.
>
> [1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers")
> [2] https://patchwork.kernel.org/patch/9283745/
>
> Cc: Dmitry Torokhov <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpio/gpiolib-acpi.c | 36 +-----------------------------------
> 1 file changed, 1 insertion(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 21e4930ca2db..e516b7a0cc50 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
> return count ? count : -ENOENT;
> }
>
> -struct acpi_crs_lookup {
> - struct list_head node;
> - struct acpi_device *adev;
> - const char *con_id;
> -};
> -
> -static DEFINE_MUTEX(acpi_crs_lookup_lock);
> -static LIST_HEAD(acpi_crs_lookup_list);
> -
> bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
> {
> - struct acpi_crs_lookup *l, *lookup = NULL;
> -
> /* Never allow fallback if the device has properties */
> if (adev->data.properties || adev->driver_gpios)
> return false;
>
> - 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);
> -
> - return lookup &&
> - ((!lookup->con_id && !con_id) ||
> - (lookup->con_id && con_id &&
> - strcmp(lookup->con_id, con_id) == 0));
> + return con_id == NULL;
> }
> --
> 2.11.0
>
--
Dmitry
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 <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/gpio/gpiolib-acpi.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 56a6b1be3a17..3bda3166d418 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -593,7 +593,7 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
>
> /* Try first from _DSD */
> for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> - if (con_id && strcmp(con_id, "gpios")) {
> + if (con_id) {
> snprintf(propname, sizeof(propname), "%s-%s",
> con_id, gpio_suffixes[i]);
> } else {
> @@ -1081,7 +1081,7 @@ int acpi_gpio_count(struct device *dev, const char *con_id)
>
> /* Try first from _DSD */
> for (i = 0; i < ARRAY_SIZE(gpio_suffixes); i++) {
> - if (con_id && strcmp(con_id, "gpios"))
> + if (con_id)
> snprintf(propname, sizeof(propname), "%s-%s",
> con_id, gpio_suffixes[i]);
> else
> --
> 2.11.0
>
--
Dmitry
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 <[email protected]>
Reviewed-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/gpio/gpiolib-acpi.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 3bda3166d418..21e4930ca2db 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -614,12 +614,12 @@ struct gpio_desc *acpi_find_gpio(struct device *dev,
> desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
> if (IS_ERR(desc))
> return desc;
> + }
>
> - if ((flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH) &&
> - info.gpioint) {
> - dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
> - return ERR_PTR(-ENOENT);
> - }
> + if (info.gpioint &&
> + (flags == GPIOD_OUT_LOW || flags == GPIOD_OUT_HIGH)) {
> + dev_dbg(dev, "refusing GpioInt() entry when doing GPIOD_OUT_* lookup\n");
> + return ERR_PTR(-ENOENT);
> }
>
> if (info.polarity == GPIO_ACTIVE_LOW)
> --
> 2.11.0
>
--
Dmitry
On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
> 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 <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> Documentation/acpi/gpio-properties.txt | 60 ++++++++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/Documentation/acpi/gpio-properties.txt b/Documentation/acpi/gpio-properties.txt
> index 2aff0349facd..07954b7c3a12 100644
> --- a/Documentation/acpi/gpio-properties.txt
> +++ b/Documentation/acpi/gpio-properties.txt
> @@ -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).
> +
> +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>
> > 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.
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 <[email protected]>
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
^^^^^^^^^
resources
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 <[email protected]>
On Thu, Mar 23, 2017 at 8:46 PM, Andy Shevchenko
<[email protected]> 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
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:
> > 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.
Oh, yeah, though it didn't fix a potential issue described below.
> > 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)
> > 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 <[email protected]>
Intel Finland Oy
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:
> > +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".
> > +
> > +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 <[email protected]>
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:
> > 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.
>
> Oh, yeah, though it didn't fix a potential issue described below.
>
> > > 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?
Yes, and it uses named GPIOs.
--
Dmitry
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:
> > On Thu, Mar 23, 2017 at 09:46:16PM +0200, Andy Shevchenko wrote:
>
>
> > > +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/...
>
> > > +
> > > +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
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:
> > 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:
> >
> >
> > > > +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.
> >
> > 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."
> > > > +
> > > > +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 <[email protected]>
Intel Finland Oy
On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
> 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:
> > > 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:
> > >
> > >
> > > > > +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.
>
> > >
> > > 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."
>
> > > > > +
> > > > > +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 <[email protected]>
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:
> > 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:
> > > > 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:
> > > >
> > > >
> > > > > > +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 ;)
> >
> > > >
> > > > 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).
> >
> > > > > > +
> > > > > > +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.
> >
> > 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
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:
> > On Wed, 2017-03-29 at 18:04 +0300, Andy Shevchenko wrote:
> > > 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:
> > > > > 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:
> > > 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.
> > > 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?
> > > 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 <[email protected]>
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:
> > 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:
> > > > 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:
> > > > > > 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:
>
> > > > 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.
>
> > > > 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?
>
> > > > 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