| Message ID | 1394456094-21671-6-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: Here: > +static acpi_status > +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > + u32 bits, u64 *value, void *handler_context, > + void *region_context) > +{ > + struct acpi_gpio_chip *achip = region_context; > + struct gpio_chip *chip = achip->chip; > + struct acpi_resource_gpio *agpio; > + struct acpi_resource *ares; > + acpi_status status; > + bool pull; Should be named pull_up, right? > + int i; > + > + status = acpi_buffer_to_resource(achip->conn_info.connection, > + achip->conn_info.length, &ares); > + if (ACPI_FAILURE(status)) > + return status; > + > + if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) { > + ACPI_FREE(ares); > + return AE_BAD_PARAMETER; > + } > + > + agpio = &ares->data.gpio; > + pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP; So here ACPI tells us that the pin is pulled up. And I am suspicious since this is strictly speaking pin control business and not GPIO, and I won't let pin control stuff sneak into the GPIO subsystem under the radar just because I'm not paying close enough attention. I have been told that these things are not dynamic (i.e. we only get informed that a pin is pulled up/down, we cannot actively change the config) is this correct? If any sort of advanced pin control business is going to come into ACPI a sibling driver in drivers/pinctrl/pinctrl-acpi.c needs to be created that can select CONFIG_PINCONF properly reflect this stuff in debugfs etc. (Maybe also give proper names on the pins since I hear this is coming to ACPI!) > + if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT && > + function == ACPI_WRITE)) { > + ACPI_FREE(ares); > + return AE_BAD_PARAMETER; > + } > + > + for (i = 0; i < agpio->pin_table_length; i++) { > + unsigned pin = agpio->pin_table[i]; > + struct acpi_gpio_connection *conn; > + struct gpio_desc *desc; > + bool found; > + > + desc = gpiochip_get_desc(chip, pin); > + if (IS_ERR(desc)) { > + status = AE_ERROR; > + goto out; > + } > + > + mutex_lock(&achip->conn_lock); > + > + found = false; > + list_for_each_entry(conn, &achip->conns, node) { > + if (conn->desc == desc) { > + found = true; > + break; > + } > + } > + if (!found) { > + int ret; > + > + ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion"); > + if (ret) { > + 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: > + gpiod_direction_output(desc, pull); Can you explain why the fact that the line is pulled down affects the default output value like this? I don't get it. A comment in the code would be needed I think. This looks more like a typical piece of code for open collector/drain (which is already handled by gpiolib) not pull up/down. > + break; > + default: > + /* > + * Assume that the BIOS has configured the > + * direction and pull accordingly. > + */ > + break; > + } > + > + conn = kzalloc(sizeof(*conn), GFP_KERNEL); > + if (!conn) { > + status = AE_NO_MEMORY; > + mutex_unlock(&achip->conn_lock); > + goto out; > + } > + > + conn->desc = desc; > + > + list_add_tail(&conn->node, &achip->conns); > + } > + > + mutex_unlock(&achip->conn_lock); > + > + > + if (function == ACPI_WRITE) > + gpiod_set_raw_value(desc, !!((1 << i) & *value)); What is this? How can the expression !!((1 << i) possibly evaluate to anything else than "true"? I don't get it. Just (desc, *value) seem more apropriate. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiA+ICsgICAgICAgICAgICAgICAgICAgICAgIGdwaW9kX3NldF9yYXdfdmFsdWUoZGVzYywgISEo KDEgPDwgaSkgJiAqdmFsdWUpKTsNCj4gDQo+IFdoYXQgaXMgdGhpcz8gSG93IGNhbiB0aGUgZXhw cmVzc2lvbiAhISgoMSA8PCBpKSBwb3NzaWJseSBldmFsdWF0ZSB0bw0KPiBhbnl0aGluZyBlbHNl IHRoYW4gInRydWUiPyBJIGRvbid0IGdldCBpdC4gSnVzdCAoZGVzYywgKnZhbHVlKSBzZWVtIG1v cmUNCj4gYXByb3ByaWF0ZS4NCg0KDQpUaGUgZXhwcmVzc2lvbiBpcyAhISgoMSA8PCBpKSAmICp2 YWx1ZSkNCg0Kc28gaXRzIHRoZSBzdGFuZGFyZCBDIHNlbWF0aWMgZm9yICdsb2dpY2FsIGFuZCcg PyAxIDogMA0KDQoNCi0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQpJbnRlbCBDb3Jwb3JhdGlvbiAoVUspIExpbWl0ZWQK UmVnaXN0ZXJlZCBOby4gMTEzNDk0NSAoRW5nbGFuZCkKUmVnaXN0ZXJlZCBPZmZpY2U6IFBpcGVy cyBXYXksIFN3aW5kb24gU04zIDFSSgpWQVQgTm86IDg2MCAyMTczIDQ3CgpUaGlzIGUtbWFpbCBh bmQgYW55IGF0dGFjaG1lbnRzIG1heSBjb250YWluIGNvbmZpZGVudGlhbCBtYXRlcmlhbCBmb3IK dGhlIHNvbGUgdXNlIG9mIHRoZSBpbnRlbmRlZCByZWNpcGllbnQocykuIEFueSByZXZpZXcgb3Ig ZGlzdHJpYnV0aW9uCmJ5IG90aGVycyBpcyBzdHJpY3RseSBwcm9oaWJpdGVkLiBJZiB5b3UgYXJl IG5vdCB0aGUgaW50ZW5kZWQKcmVjaXBpZW50LCBwbGVhc2UgY29udGFjdCB0aGUgc2VuZGVyIGFu ZCBkZWxldGUgYWxsIGNvcGllcy4K -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote: > On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > Here: > > > +static acpi_status > > +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > > + u32 bits, u64 *value, void *handler_context, > > + void *region_context) > > +{ > > + struct acpi_gpio_chip *achip = region_context; > > + struct gpio_chip *chip = achip->chip; > > + struct acpi_resource_gpio *agpio; > > + struct acpi_resource *ares; > > + acpi_status status; > > + bool pull; > > Should be named pull_up, right? Yes. > > > + int i; > > + > > + status = acpi_buffer_to_resource(achip->conn_info.connection, > > + achip->conn_info.length, &ares); > > + if (ACPI_FAILURE(status)) > > + return status; > > + > > + if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) { > > + ACPI_FREE(ares); > > + return AE_BAD_PARAMETER; > > + } > > + > > + agpio = &ares->data.gpio; > > + pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP; > > So here ACPI tells us that the pin is pulled up. > > And I am suspicious since this is strictly speaking pin control business > and not GPIO, and I won't let pin control stuff sneak into the GPIO > subsystem under the radar just because I'm not paying close enough > attention. This has more to do in finding out what will be the initial value of the GPIO when we set it to output (given that it is output). > I have been told that these things are not dynamic (i.e. we only get > informed that a pin is pulled up/down, we cannot actively change the > config) is this correct? As far as I can tell, yes. > If any sort of advanced pin control business is going to come into > ACPI a sibling driver in drivers/pinctrl/pinctrl-acpi.c needs to be > created that can select CONFIG_PINCONF properly reflect this > stuff in debugfs etc. (Maybe also give proper names on the pins > since I hear this is coming to ACPI!) No advanced pin control business, just GPIO stuff :) > > + if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT && > > + function == ACPI_WRITE)) { > > + ACPI_FREE(ares); > > + return AE_BAD_PARAMETER; > > + } > > + > > + for (i = 0; i < agpio->pin_table_length; i++) { > > + unsigned pin = agpio->pin_table[i]; > > + struct acpi_gpio_connection *conn; > > + struct gpio_desc *desc; > > + bool found; > > + > > + desc = gpiochip_get_desc(chip, pin); > > + if (IS_ERR(desc)) { > > + status = AE_ERROR; > > + goto out; > > + } > > + > > + mutex_lock(&achip->conn_lock); > > + > > + found = false; > > + list_for_each_entry(conn, &achip->conns, node) { > > + if (conn->desc == desc) { > > + found = true; > > + break; > > + } > > + } > > + if (!found) { > > + int ret; > > + > > + ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion"); > > + if (ret) { > > + 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: > > + gpiod_direction_output(desc, pull); > > Can you explain why the fact that the line is pulled down affects the > default output value like this? I don't get it. That's the thing - ACPI doesn't tell us what is the initial value of the pin. There is no such field in GpioIo() resource. So I'm assuming here that if it says that the pin is pulled up, the default value will be high. > A comment in the code would be needed I think. > > This looks more like a typical piece of code for open collector/drain > (which is already handled by gpiolib) not pull up/down. > > > > + break; > > + default: > > + /* > > + * Assume that the BIOS has configured the > > + * direction and pull accordingly. > > + */ > > + break; > > + } > > + > > + conn = kzalloc(sizeof(*conn), GFP_KERNEL); > > + if (!conn) { > > + status = AE_NO_MEMORY; > > + mutex_unlock(&achip->conn_lock); > > + goto out; > > + } > > + > > + conn->desc = desc; > > + > > + list_add_tail(&conn->node, &achip->conns); > > + } > > + > > + mutex_unlock(&achip->conn_lock); > > + > > + > > + if (function == ACPI_WRITE) > > + gpiod_set_raw_value(desc, !!((1 << i) & *value)); > > What is this? How can the expression !!((1 << i) possibly evaluate to > anything else than "true"? I don't get it. Just (desc, *value) seem more > apropriate. We are dealing with multiple pins here. So for example if agpio->pin_table_length == 2 and *value == 0x2 we get: i == 0: !!((1 << 0) & 0x2) --> false i == 1: !!((1 << 1) & 0x2) --> true Maybe gpiod_set_raw_value(desc, (*value >> i) & 1); is cleaner? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 13, 2014 at 4:05 PM, Cox, Alan <alan.cox@intel.com> wrote: >> > + gpiod_set_raw_value(desc, !!((1 << i) & *value)); >> >> What is this? How can the expression !!((1 << i) possibly evaluate to >> anything else than "true"? I don't get it. Just (desc, *value) seem more >> apropriate. > > > The expression is !!((1 << i) & *value) > > so its the standard C sematic for 'logical and' ? 1 : 0 Hm I missed the first paranthesis when parsing in my head, all wrong, too bad. Thanks Alan. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 13, 2014 at 4:18 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote: >> > + case ACPI_IO_RESTRICT_OUTPUT: >> > + gpiod_direction_output(desc, pull); >> >> Can you explain why the fact that the line is pulled down affects the >> default output value like this? I don't get it. > > That's the thing - ACPI doesn't tell us what is the initial value of the > pin. There is no such field in GpioIo() resource. > > So I'm assuming here that if it says that the pin is pulled up, the default > value will be high. OK! So exactly that statement is what I want to see as a comment in this switch case. >> > + if (function == ACPI_WRITE) >> > + gpiod_set_raw_value(desc, !!((1 << i) & *value)); >> >> What is this? How can the expression !!((1 << i) possibly evaluate to >> anything else than "true"? I don't get it. Just (desc, *value) seem more >> apropriate. > > We are dealing with multiple pins here. So for example if > agpio->pin_table_length == 2 and *value == 0x2 we get: > > i == 0: !!((1 << 0) & 0x2) --> false > i == 1: !!((1 << 1) & 0x2) --> true Yeah, Alan already pointed out my parse error... this is OK. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 14, 2014 at 11:53:30AM +0100, Linus Walleij wrote: > On Thu, Mar 13, 2014 at 4:18 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote: > > >> > + case ACPI_IO_RESTRICT_OUTPUT: > >> > + gpiod_direction_output(desc, pull); > >> > >> Can you explain why the fact that the line is pulled down affects the > >> default output value like this? I don't get it. > > > > That's the thing - ACPI doesn't tell us what is the initial value of the > > pin. There is no such field in GpioIo() resource. > > > > So I'm assuming here that if it says that the pin is pulled up, the default > > value will be high. > > OK! So exactly that statement is what I want to see as a comment > in this switch case. Sure. I'll add that comment to the next version of this patch. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 092ea4e5c9a8..b81272b7c63b 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -16,6 +16,7 @@ #include <linux/export.h> #include <linux/acpi.h> #include <linux/interrupt.h> +#include <linux/mutex.h> #include "gpiolib.h" @@ -26,7 +27,20 @@ struct acpi_gpio_event { unsigned int irq; }; +struct acpi_gpio_connection { + struct list_head node; + struct gpio_desc *desc; +}; + struct acpi_gpio_chip { + /* + * ACPICA requires that the first field of the context parameter + * passed to acpi_install_address_space_handler() is large enough + * to hold struct acpi_connection_info. + */ + struct acpi_connection_info conn_info; + struct list_head conns; + struct mutex conn_lock; struct gpio_chip *chip; struct list_head events; }; @@ -334,6 +348,146 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index, return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT); } +static acpi_status +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, + u32 bits, u64 *value, void *handler_context, + void *region_context) +{ + struct acpi_gpio_chip *achip = region_context; + struct gpio_chip *chip = achip->chip; + struct acpi_resource_gpio *agpio; + struct acpi_resource *ares; + acpi_status status; + bool pull; + int i; + + status = acpi_buffer_to_resource(achip->conn_info.connection, + achip->conn_info.length, &ares); + if (ACPI_FAILURE(status)) + return status; + + if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) { + ACPI_FREE(ares); + return AE_BAD_PARAMETER; + } + + agpio = &ares->data.gpio; + pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP; + + if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT && + function == ACPI_WRITE)) { + ACPI_FREE(ares); + return AE_BAD_PARAMETER; + } + + for (i = 0; i < agpio->pin_table_length; i++) { + unsigned pin = agpio->pin_table[i]; + struct acpi_gpio_connection *conn; + struct gpio_desc *desc; + bool found; + + desc = gpiochip_get_desc(chip, pin); + if (IS_ERR(desc)) { + status = AE_ERROR; + goto out; + } + + mutex_lock(&achip->conn_lock); + + found = false; + list_for_each_entry(conn, &achip->conns, node) { + if (conn->desc == desc) { + found = true; + break; + } + } + if (!found) { + int ret; + + ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion"); + if (ret) { + 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: + gpiod_direction_output(desc, pull); + break; + default: + /* + * Assume that the BIOS has configured the + * direction and pull accordingly. + */ + break; + } + + conn = kzalloc(sizeof(*conn), GFP_KERNEL); + if (!conn) { + status = AE_NO_MEMORY; + mutex_unlock(&achip->conn_lock); + goto out; + } + + conn->desc = desc; + + list_add_tail(&conn->node, &achip->conns); + } + + mutex_unlock(&achip->conn_lock); + + + if (function == ACPI_WRITE) + gpiod_set_raw_value(desc, !!((1 << i) & *value)); + else + *value |= gpiod_get_raw_value(desc) << i; + } + +out: + ACPI_FREE(ares); + return status; +} + +static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip) +{ + struct gpio_chip *chip = achip->chip; + acpi_handle handle = ACPI_HANDLE(chip->dev); + acpi_status status; + + INIT_LIST_HEAD(&achip->conns); + mutex_init(&achip->conn_lock); + status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, + acpi_gpio_adr_space_handler, + NULL, achip); + if (ACPI_FAILURE(status)) + dev_err(chip->dev, "Failed to install GPIO OpRegion handler\n"); +} + +static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip) +{ + struct gpio_chip *chip = achip->chip; + acpi_handle handle = ACPI_HANDLE(chip->dev); + struct acpi_gpio_connection *conn, *tmp; + acpi_status status; + + status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, + acpi_gpio_adr_space_handler); + if (ACPI_FAILURE(status)) { + dev_err(chip->dev, "Failed to remove GPIO OpRegion handler\n"); + return; + } + + list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) { + gpiochip_free_own_desc(conn->desc); + list_del(&conn->node); + kfree(conn); + } +} + void acpi_gpiochip_add(struct gpio_chip *chip) { struct acpi_gpio_chip *acpi_gpio; @@ -361,6 +515,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip) } acpi_gpiochip_request_interrupts(acpi_gpio); + acpi_gpiochip_request_regions(acpi_gpio); } void acpi_gpiochip_remove(struct gpio_chip *chip) @@ -379,6 +534,7 @@ void acpi_gpiochip_remove(struct gpio_chip *chip) return; } + acpi_gpiochip_free_regions(acpi_gpio); acpi_gpiochip_free_interrupts(acpi_gpio); acpi_detach_data(handle, acpi_gpio_chip_dh);
GPIO operation regions is a new feature introduced in ACPI 5.0 specification. This feature adds a way for platform ASL code to call back to OS GPIO driver and toggle GPIO pins. An example ASL code from Lenovo Miix 2 tablet with only relevant part listed: Device (\_SB.GPO0) { Name (AVBL, Zero) Method (_REG, 2, NotSerialized) { If (LEqual (Arg0, 0x08)) { // Marks the region available Store (Arg1, AVBL) } } OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0C) Field (GPOP, ByteAcc, NoLock, Preserve) { Connection ( GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionOutputOnly, "\\_SB.GPO0", 0x00, ResourceConsumer,,) { 0x003B } ), SHD3, 1, } } Device (SHUB) { Method (_PS0, 0, Serialized) { If (LEqual (\_SB.GPO0.AVBL, One)) { Store (One, \_SB.GPO0.SHD3) Sleep (0x32) } } Method (_PS3, 0, Serialized) { If (LEqual (\_SB.GPO0.AVBL, One)) { Store (Zero, \_SB.GPO0.SHD3) } } } How this works is that whenever _PS0 or _PS3 method is run (typically when SHUB device is transitioned to D0 or D3 respectively), ASL code checks if the GPIO operation region is available (\_SB.GPO0.AVBL). If it is we go and store either 0 or 1 to \_SB.GPO0.SHD3. Now, when ACPICA notices ACPI GPIO operation region access (the store above) it will call acpi_gpio_adr_space_handler() that then toggles the GPIO accordingly using standard gpiolib interfaces. Implement the support by registering GPIO operation region handlers for all GPIO devices that have an ACPI handle. First time the GPIO is used by the ASL code we make sure that the GPIO stays requested until the GPIO chip driver itself is unloaded. If we find out that the GPIO is already requested we just toggle it according to the value got from ASL code. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/gpio/gpiolib-acpi.c | 156 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 156 insertions(+)