| Message ID | 1379085280-2211-2-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, Sep 13, 2013 at 6:14 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > GPIO operation regions is a new feature introduced in ACPI 5.0 > specification. In practise it means that now ASL code can toggle GPIOs with > the help of the OS GPIO driver. [] > void acpi_gpiochip_add(struct gpio_chip *chip) > { > + struct acpi_gpio_chip_data *data; > + acpi_handle handle; > + acpi_status status; > + > + handle = ACPI_HANDLE(chip->dev); > + if (!handle) > + return; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); May we use devm_kzalloc here?
On Fri, Sep 13, 2013 at 06:55:11PM +0300, Andy Shevchenko wrote: > On Fri, Sep 13, 2013 at 6:14 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > GPIO operation regions is a new feature introduced in ACPI 5.0 > > specification. In practise it means that now ASL code can toggle GPIOs with > > the help of the OS GPIO driver. > > [] > > > void acpi_gpiochip_add(struct gpio_chip *chip) > > { > > + struct acpi_gpio_chip_data *data; > > + acpi_handle handle; > > + acpi_status status; > > + > > + handle = ACPI_HANDLE(chip->dev); > > + if (!handle) > > + return; > > + > > + data = kzalloc(sizeof(*data), GFP_KERNEL); > > May we use devm_kzalloc here? The ACPI event handling code still uses kzalloc() and we need to call acpi_gpiolib_remove() anyway (which undoes this), so I think we should stick with kzalloc() now. -- 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
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Mika Westerberg > Sent: Friday, September 13, 2013 11:15 PM > > GPIO operation regions is a new feature introduced in ACPI 5.0 > specification. In practise it means that now ASL code can toggle GPIOs with > the help of the OS GPIO driver. > > An imaginary example of such ASL code: > > Device (\SB.GPIO) > { > // _REG is called when the operation region handler is > // installed. > Method (_REG) > { > ... > } > > OperationRegion (GPOP, GeneralPurposeIo, 0, 0xc) > Field (GPOP, ByteAcc, NoLock, Preserve) > { > Connection > ( > GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, > IoRestrictionOutputOnly, "\\_SB.GPIO", > 0x00, ResourceConsumer,,) > { > 0x0005 > } > ), > PWR0, 1 > } > > ... > } > > Device (\SB.DEV0) > { > ... > > Method (_PS0, 0, Serialized) > { > // Toggle the GPIO > Store (1, \SB.GPIO.PWR0) > } > > Method (_PS3, 0, Serialized) > { > Store (0, \SB.GPIO.PRW0) > } > } > > The device \SB.DEV0 is powered on by asserting \SB.GPIO.PWR0 GPIO line. So > when the OS calls _PS0 or _PS3, that GPIO line should be set to 1 or 0 by > the actual GPIO driver. > > In order to support GPIO operation regions we install ACPI address space > handler for the device (provided that it has an ACPI handle). This handler > uses the standard GPIO APIs to toggle the pin according to what the ASL > code does. > > Since there is need to attach more data to the ACPI object, we create a new > structure acpi_gpio_chip_data that contains data for both GPIO operation > regions and for ACPI event handling and make the event handling code to use > this new structure. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/gpio/gpiolib-acpi.c | 131 ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 126 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index e12a08e..f52536a 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -24,6 +24,17 @@ struct acpi_gpio_evt_pin { > unsigned int irq; > }; > > +struct acpi_gpio_chip_data { > + /* > + * acpi_install_address_space_handler() wants to put the connection > + * information at the start of the context structure we pass it, in > + * case we are dealing with GPIO operation regions. > + */ > + struct acpi_connection_info ci; > + struct gpio_chip *chip; > + struct list_head *evt_pins; > +}; > + > static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) > { > if (!gc->dev) > @@ -86,7 +97,7 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data) > return IRQ_HANDLED; > } > > -static void acpi_gpio_evt_dh(acpi_handle handle, void *data) > +static void acpi_gpio_chip_dh(acpi_handle handle, void *data) > { > /* The address of this function is used as a key. */ > } > @@ -127,12 +138,16 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) > if (ACPI_SUCCESS(status)) { > evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL); > if (evt_pins) { > + struct acpi_gpio_chip_data *data; > + > INIT_LIST_HEAD(evt_pins); > - status = acpi_attach_data(handle, acpi_gpio_evt_dh, > - evt_pins); > + status = acpi_get_data(handle, acpi_gpio_chip_dh, > + (void **)&data); > if (ACPI_FAILURE(status)) { > kfree(evt_pins); > evt_pins = NULL; > + } else { > + data->evt_pins = evt_pins; > } > } > } > @@ -293,6 +308,7 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > acpi_status status; > struct list_head *evt_pins; > struct acpi_gpio_evt_pin *evt_pin, *ep; > + struct acpi_gpio_chip_data *data; > > if (!chip->dev || !chip->to_irq) > return; > @@ -301,28 +317,133 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > if (!handle) > return; > > - status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data); > if (ACPI_FAILURE(status)) > return; > > + evt_pins = data->evt_pins; > + data->evt_pins = NULL; > + > list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > list_del(&evt_pin->node); > kfree(evt_pin); > } > > - acpi_detach_data(handle, acpi_gpio_evt_dh); > kfree(evt_pins); > } > > +static acpi_status > +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, > + u32 bits, u64 *value, void *handler_context, > + void *reqion_context) > +{ > + struct acpi_gpio_chip_data *data = handler_context; > + struct gpio_chip *chip = data->chip; > + struct acpi_resource *ares; > + int ret, gpio = -EINVAL; > + unsigned long flags = 0; > + acpi_status status; > + > + status = acpi_buffer_to_resource(data->ci.connection, data->ci.length, > + &ares); > + if (ACPI_FAILURE(status)) > + return status; > + > + if (ares->type == ACPI_RESOURCE_TYPE_GPIO) { > + const struct acpi_resource_gpio *agpio = &ares->data.gpio; > + > + switch (agpio->io_restriction) { > + case ACPI_IO_RESTRICT_NONE: > + flags = GPIOF_DIR_IN | GPIOF_DIR_OUT; > + break; > + case ACPI_IO_RESTRICT_INPUT: > + flags = GPIOF_DIR_IN; > + break; > + case ACPI_IO_RESTRICT_OUTPUT: > + flags = GPIOF_DIR_OUT; > + break; > + } > + > + gpio = acpi_get_gpio(agpio->resource_source.string_ptr, > + agpio->pin_table[0]); > + } > + > + ACPI_FREE(ares); > + > + if (!gpio_is_valid(gpio)) { > + dev_err(chip->dev, "GpioOpReg: invalid GPIO resource\n"); > + return AE_ERROR; > + } > + > + ret = gpio_request_one(gpio, flags, "acpi_gpio_adr_space"); > + if (ret) { > + dev_err(chip->dev, "GpioOpReg: failed to request GPIO\n"); > + return AE_ERROR; > + } > + > + if (function == ACPI_WRITE) > + gpio_set_value(gpio, (int)*value); > + else > + *value = gpio_get_value(gpio); > + > + gpio_free(gpio); > + return AE_OK; > +} > + > void acpi_gpiochip_add(struct gpio_chip *chip) > { > + struct acpi_gpio_chip_data *data; > + acpi_handle handle; > + acpi_status status; > + > + handle = ACPI_HANDLE(chip->dev); > + if (!handle) > + return; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return; > + > + status = acpi_attach_data(handle, acpi_gpio_chip_dh, data); > + if (ACPI_FAILURE(status)) { > + kfree(data); > + return; > + } > + > + data->chip = chip; > + > + status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, > + acpi_gpio_adr_space_handler, > + NULL, data); Is it possible to install the handler for ACPI_ROOT_OBJECT? Can it be achieved by implementing a setup callback? Maybe you can also eliminate acpi_attach_data usages by doing so. > + if (ACPI_FAILURE(status)) { > + acpi_detach_data(handle, acpi_gpio_chip_dh); > + kfree(data); > + return; > + } > + > acpi_gpiochip_request_interrupts(chip); > } > EXPORT_SYMBOL_GPL(acpi_gpiochip_add); > > void acpi_gpiochip_remove(struct gpio_chip *chip) > { > + struct acpi_gpio_chip_data *data; > + acpi_handle handle; > + acpi_status status; > + > + handle = ACPI_HANDLE(chip->dev); > + if (!handle) > + return; > + > + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data); > + if (ACPI_FAILURE(status)) > + return; > + > acpi_gpiochip_free_interrupts(chip); > + acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, > + acpi_gpio_adr_space_handler); > + acpi_detach_data(handle, acpi_gpio_chip_dh); > + kfree(data); > } > EXPORT_SYMBOL_GPL(acpi_gpiochip_remove); > -- > 1.8.4.rc3 > > -- > 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 -- 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 Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote: > Is it possible to install the handler for ACPI_ROOT_OBJECT? > Can it be achieved by implementing a setup callback? Yes that can be done. However, that would mean that we always install the operation region handler even if there is no suitable GPIO driver loaded. With this patch we install the handler once the GPIO driver for this device is registered. If nothing is registered no handlers will be installed. What would be the advantage in doing what you propose? > Maybe you can also eliminate acpi_attach_data usages by doing so. I think we still need that for ACPI _EVT handling. -- 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
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Sunday, September 15, 2013 2:52 PM > > On Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote: > > Is it possible to install the handler for ACPI_ROOT_OBJECT? > > Can it be achieved by implementing a setup callback? > > Yes that can be done. However, that would mean that we always install the > operation region handler even if there is no suitable GPIO driver loaded. > With this patch we install the handler once the GPIO driver for this device > is registered. If nothing is registered no handlers will be installed. > > What would be the advantage in doing what you propose? A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to access other ACPI provided value-adds. So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions have nothing to do with the GPIO devices' ACPI handle. We cannot imply that the BIOS writers won't create such Frankenstein in the future. So it's better to install address space handlers from ACPI_ROOT_OBJECT. > > Maybe you can also eliminate acpi_attach_data usages by doing so. > > I think we still need that for ACPI _EVT handling. It could be good if we can find a way to eliminate all acpi_attach_data usages and make this function deprecated. But that's fine. :-) Thanks and best regards -Lv -- 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
> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Zheng, Lv > Sent: Monday, September 16, 2013 8:47 AM > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Sent: Sunday, September 15, 2013 2:52 PM > > > > On Sat, Sep 14, 2013 at 12:10:37AM +0000, Zheng, Lv wrote: > > > Is it possible to install the handler for ACPI_ROOT_OBJECT? > > > Can it be achieved by implementing a setup callback? > > > > Yes that can be done. However, that would mean that we always install the > > operation region handler even if there is no suitable GPIO driver loaded. > > With this patch we install the handler once the GPIO driver for this device > > is registered. If nothing is registered no handlers will be installed. > > > > What would be the advantage in doing what you propose? > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to > access other ACPI provided value-adds. > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions > have nothing to do with the GPIO devices' ACPI handle. Sorry for the wording. It's better to say the GPIO operation region users haven't strict relationship to the GPIO operation region providers. As the installation is to provide GPIO operation regions to the users, it shouldn't relate to the providers' ACPI handle. > We cannot imply that the BIOS writers won't create such Frankenstein in the future. > So it's better to install address space handlers from ACPI_ROOT_OBJECT. If we didn't do this and such a pseudo device was created, then the error message: "Region XXX has no handler" would be prompted. Thanks -Lv > > > > Maybe you can also eliminate acpi_attach_data usages by doing so. > > > > I think we still need that for ACPI _EVT handling. > > It could be good if we can find a way to eliminate all acpi_attach_data usages and make this function deprecated. > But that's fine. :-) > > Thanks and best regards > -Lv > -- > 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 -- 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 Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote: > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse to > > access other ACPI provided value-adds. > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation regions > > have nothing to do with the GPIO devices' ACPI handle. > > Sorry for the wording. > It's better to say the GPIO operation region users haven't strict > relationship to the GPIO operation region providers. > As the installation is to provide GPIO operation regions to the users, it > shouldn't relate to the providers' ACPI handle. If I understand you correctly you mean that there might be multiple users (different devices) for the same GPIO operation region, right? That shouldn't be a problem as far as I can tell. What comes to the hierarchy you refer, I'm not sure if that is a problem either (unless I'm missing something). The GPIO can be used anywhere in the ASL, it doesn't have to be descendant of the GPIO device. You only need to do something like this: // Assert the GPIO Store(1, \_SB.PCI0.SHD3) In other words, use the fully qualified name. Typically when the GPIO device _REG() is called it sets some variable like AVBL to true which is then checked in the code that uses the GPIO: If (LEqual (\_SB.PCI0.GPO0.AVBL, One)) { Store (Zero, \_SB.PCI0..SHD3) } So if there is no driver, this part of the code is never called. -- 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
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Monday, September 16, 2013 4:11 PM > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote: > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse > to > > > access other ACPI provided value-adds. > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation > regions > > > have nothing to do with the GPIO devices' ACPI handle. > > > > Sorry for the wording. > > It's better to say the GPIO operation region users haven't strict > > relationship to the GPIO operation region providers. > > As the installation is to provide GPIO operation regions to the users, it > > shouldn't relate to the providers' ACPI handle. > > If I understand you correctly you mean that there might be multiple users > (different devices) for the same GPIO operation region, right? No, this is not what I meant. Can one vendor device uses two or more GPIO pins from different GPIO ports? > That shouldn't be a problem as far as I can tell. > > What comes to the hierarchy you refer, I'm not sure if that is a problem > either (unless I'm missing something). The GPIO can be used anywhere in the > ASL, it doesn't have to be descendant of the GPIO device. You only need to > do something like this: > > // Assert the GPIO > Store(1, \_SB.PCI0.SHD3) > > In other words, use the fully qualified name. Yes, which means this line of code can be everywhere in the namespace. It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope. The problem is the installation, the first parameter for acpi_install_address_space_handler() is a namespace node, the function will call _REG for all OperationRegions below the scope whose top most node is the specified node. The nodes out of this scope are not affected. So if an OperationRegion is defined out of this scope, problem happens. > Typically when the GPIO device _REG() is called it sets some variable like > AVBL to true which is then checked in the code that uses the GPIO: > > If (LEqual (\_SB.PCI0.GPO0.AVBL, One)) > { > Store (Zero, \_SB.PCI0..SHD3) > } > > So if there is no driver, this part of the code is never called. This can trigger exceptions, which can be used to load the GPIO driver. This seems to be another topic. Thanks and best regards -Lv -- 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 Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote: > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Sent: Monday, September 16, 2013 4:11 PM > > > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote: > > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even worse > > to > > > > access other ACPI provided value-adds. > > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation > > regions > > > > have nothing to do with the GPIO devices' ACPI handle. > > > > > > Sorry for the wording. > > > It's better to say the GPIO operation region users haven't strict > > > relationship to the GPIO operation region providers. > > > As the installation is to provide GPIO operation regions to the users, it > > > shouldn't relate to the providers' ACPI handle. > > > > If I understand you correctly you mean that there might be multiple users > > (different devices) for the same GPIO operation region, right? > > No, this is not what I meant. > Can one vendor device uses two or more GPIO pins from different GPIO ports? Yes. > > That shouldn't be a problem as far as I can tell. > > > > What comes to the hierarchy you refer, I'm not sure if that is a problem > > either (unless I'm missing something). The GPIO can be used anywhere in the > > ASL, it doesn't have to be descendant of the GPIO device. You only need to > > do something like this: > > > > // Assert the GPIO > > Store(1, \_SB.PCI0.SHD3) > > > > In other words, use the fully qualified name. > > Yes, which means this line of code can be everywhere in the namespace. > It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope. > > The problem is the installation, the first parameter for > acpi_install_address_space_handler() is a namespace node, the function > will call _REG for all OperationRegions below the scope whose top most > node is the specified node. > The nodes out of this scope are not affected. So if an OperationRegion > is defined out of this scope, problem happens. I suppose for that operation region another GPIO device should be introduced then, no? So if we don't have a GPIO driver for the given operation region, what can we do? We don't want the ASL code to erroneusly think that there is an operation region available when it is not. -- 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, Sep 13, 2013 at 5:14 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c (...) > +struct acpi_gpio_chip_data { > + /* > + * acpi_install_address_space_handler() wants to put the connection > + * information at the start of the context structure we pass it, in > + * case we are dealing with GPIO operation regions. > + */ > + struct acpi_connection_info ci; > + struct gpio_chip *chip; > + struct list_head *evt_pins; > +}; Consider just naming this acpi_gpio_chip, as it is obviously some generic container that you will keep adding to. I'm uncertain how things work, it wouldn't add something to have struct gpio_chip be a true member (not a pointer) so you can allocate one thing from the drivers, and e.g. use container_of() to get from the gpio_chip to the acpi_gpio_chip[_data]? 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, Sep 20, 2013 at 09:21:37PM +0200, Linus Walleij wrote: > On Fri, Sep 13, 2013 at 5:14 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > (...) > > +struct acpi_gpio_chip_data { > > + /* > > + * acpi_install_address_space_handler() wants to put the connection > > + * information at the start of the context structure we pass it, in > > + * case we are dealing with GPIO operation regions. > > + */ > > + struct acpi_connection_info ci; > > + struct gpio_chip *chip; > > + struct list_head *evt_pins; > > +}; > > Consider just naming this acpi_gpio_chip, as it is obviously some > generic container that you will keep adding to. Sure. > I'm uncertain how things work, it wouldn't add something to have > struct gpio_chip be a true member (not a pointer) so you can > allocate one thing from the drivers, and e.g. use container_of() > to get from the gpio_chip to the acpi_gpio_chip[_data]? The drivers are just normal platform drivers (for example gpio-lynxpoint.c) and they shouldn't care if they got enumerated from ACPI. Allocating acpi_gpio_chip from a driver would make it depend on ACPI, if I'm understanding correctly. -- 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
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > Sent: Tuesday, September 17, 2013 4:37 PM > > On Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote: > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > > Sent: Monday, September 16, 2013 4:11 PM > > > > > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote: > > > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > > > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even > worse > > > to > > > > > access other ACPI provided value-adds. > > > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation > > > regions > > > > > have nothing to do with the GPIO devices' ACPI handle. > > > > > > > > Sorry for the wording. > > > > It's better to say the GPIO operation region users haven't strict > > > > relationship to the GPIO operation region providers. > > > > As the installation is to provide GPIO operation regions to the users, it > > > > shouldn't relate to the providers' ACPI handle. > > > > > > If I understand you correctly you mean that there might be multiple users > > > (different devices) for the same GPIO operation region, right? > > > > No, this is not what I meant. > > Can one vendor device uses two or more GPIO pins from different GPIO ports? > > Yes. So how can such a device under one of these GPIO ports? It can only be under one particular GPIO port device, right? Thus I believe such device will appear below other devices rather than below a GPIO port device in the ACPI namespace. > > > > That shouldn't be a problem as far as I can tell. > > > > > > What comes to the hierarchy you refer, I'm not sure if that is a problem > > > either (unless I'm missing something). The GPIO can be used anywhere in the > > > ASL, it doesn't have to be descendant of the GPIO device. You only need to > > > do something like this: > > > > > > // Assert the GPIO > > > Store(1, \_SB.PCI0.SHD3) > > > > > > In other words, use the fully qualified name. > > > > Yes, which means this line of code can be everywhere in the namespace. > > It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope. > > > > The problem is the installation, the first parameter for > > acpi_install_address_space_handler() is a namespace node, the function > > will call _REG for all OperationRegions below the scope whose top most > > node is the specified node. > > The nodes out of this scope are not affected. So if an OperationRegion > > is defined out of this scope, problem happens. > > I suppose for that operation region another GPIO device should be > introduced then, no? I believe the answer is no. > So if we don't have a GPIO driver for the given operation region, what can > we do? We don't want the ASL code to erroneusly think that there is an > operation region available when it is not. I think GPIO "address_space_handler" should be provided by an ACPI GPIO address space module rather than provided by a particular GPIO driver. Actually, without the readiness of the GPIO driver, currently ASL code can still access the GPIO fields, though it just results in "handler not found" ACPICA error message. If the "setup" callback is implemented, then the proper GPIO driver can be matched in this "setup" callback. If the GPIO driver hasn't been loaded by Linux and thus not matched in the "setup" callback, from users' point of view, the result is just changing the error message from "handler not found" to "driver not found". Thanks and best regards -Lv -- 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 Tue, Sep 24, 2013 at 12:47:56AM +0000, Zheng, Lv wrote: > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > Sent: Tuesday, September 17, 2013 4:37 PM > > > > On Mon, Sep 16, 2013 at 11:35:56PM +0000, Zheng, Lv wrote: > > > > From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] > > > > Sent: Monday, September 16, 2013 4:11 PM > > > > > > > > On Mon, Sep 16, 2013 at 01:21:53AM +0000, Zheng, Lv wrote: > > > > > > A pseudo device may be created to access the GPIO operation region fields provided by one GPIO device. > > > > > > The pseudo device may have other functions to access other GPIO operation region fields provided by other GPIO devices, or even > > worse > > > > to > > > > > > access other ACPI provided value-adds. > > > > > > So hierarchically the pseudo device only requires CPU, thus should not be under the GPIO device, which means the GPIO operation > > > > regions > > > > > > have nothing to do with the GPIO devices' ACPI handle. > > > > > > > > > > Sorry for the wording. > > > > > It's better to say the GPIO operation region users haven't strict > > > > > relationship to the GPIO operation region providers. > > > > > As the installation is to provide GPIO operation regions to the users, it > > > > > shouldn't relate to the providers' ACPI handle. > > > > > > > > If I understand you correctly you mean that there might be multiple users > > > > (different devices) for the same GPIO operation region, right? > > > > > > No, this is not what I meant. > > > Can one vendor device uses two or more GPIO pins from different GPIO ports? > > > > Yes. > > So how can such a device under one of these GPIO ports? > It can only be under one particular GPIO port device, right? > > Thus I believe such device will appear below other devices rather than > below a GPIO port device in the ACPI namespace. I'm sorry, but I can't follow what you mean. > > > > That shouldn't be a problem as far as I can tell. > > > > > > > > What comes to the hierarchy you refer, I'm not sure if that is a problem > > > > either (unless I'm missing something). The GPIO can be used anywhere in the > > > > ASL, it doesn't have to be descendant of the GPIO device. You only need to > > > > do something like this: > > > > > > > > // Assert the GPIO > > > > Store(1, \_SB.PCI0.SHD3) > > > > > > > > In other words, use the fully qualified name. > > > > > > Yes, which means this line of code can be everywhere in the namespace. > > > It is not required to be under one particular GPIO device as long as there is an operation region defined in that scope. > > > > > > The problem is the installation, the first parameter for > > > acpi_install_address_space_handler() is a namespace node, the function > > > will call _REG for all OperationRegions below the scope whose top most > > > node is the specified node. > > > The nodes out of this scope are not affected. So if an OperationRegion > > > is defined out of this scope, problem happens. > > > > I suppose for that operation region another GPIO device should be > > introduced then, no? > > I believe the answer is no. Consider a situation, where you have two different GPIO controllers and the ASL code needs to access GPIOs on both controllers. That requires introducing two GPIO devices in ACPI namespace and two separate drivers as well. > > > So if we don't have a GPIO driver for the given operation region, what can > > we do? We don't want the ASL code to erroneusly think that there is an > > operation region available when it is not. > > I think GPIO "address_space_handler" should be provided by an ACPI GPIO > address space module rather than provided by a particular GPIO driver. > > Actually, without the readiness of the GPIO driver, currently ASL code > can still access the GPIO fields, though it just results in "handler not > found" ACPICA error message. If the "setup" callback is implemented, > then the proper GPIO driver can be matched in this "setup" callback. If > the GPIO driver hasn't been loaded by Linux and thus not matched in the > "setup" callback, from users' point of view, the result is just changing > the error message from "handler not found" to "driver not found". Well, I'm fine implementing a single global GPIO region handler (instead of what is done in this series). Rafael, do you have any comments on this? -- 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 e12a08e..f52536a 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -24,6 +24,17 @@ struct acpi_gpio_evt_pin { unsigned int irq; }; +struct acpi_gpio_chip_data { + /* + * acpi_install_address_space_handler() wants to put the connection + * information at the start of the context structure we pass it, in + * case we are dealing with GPIO operation regions. + */ + struct acpi_connection_info ci; + struct gpio_chip *chip; + struct list_head *evt_pins; +}; + static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) { if (!gc->dev) @@ -86,7 +97,7 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data) return IRQ_HANDLED; } -static void acpi_gpio_evt_dh(acpi_handle handle, void *data) +static void acpi_gpio_chip_dh(acpi_handle handle, void *data) { /* The address of this function is used as a key. */ } @@ -127,12 +138,16 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) if (ACPI_SUCCESS(status)) { evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL); if (evt_pins) { + struct acpi_gpio_chip_data *data; + INIT_LIST_HEAD(evt_pins); - status = acpi_attach_data(handle, acpi_gpio_evt_dh, - evt_pins); + status = acpi_get_data(handle, acpi_gpio_chip_dh, + (void **)&data); if (ACPI_FAILURE(status)) { kfree(evt_pins); evt_pins = NULL; + } else { + data->evt_pins = evt_pins; } } } @@ -293,6 +308,7 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) acpi_status status; struct list_head *evt_pins; struct acpi_gpio_evt_pin *evt_pin, *ep; + struct acpi_gpio_chip_data *data; if (!chip->dev || !chip->to_irq) return; @@ -301,28 +317,133 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) if (!handle) return; - status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data); if (ACPI_FAILURE(status)) return; + evt_pins = data->evt_pins; + data->evt_pins = NULL; + list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { devm_free_irq(chip->dev, evt_pin->irq, evt_pin); list_del(&evt_pin->node); kfree(evt_pin); } - acpi_detach_data(handle, acpi_gpio_evt_dh); kfree(evt_pins); } +static acpi_status +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, + u32 bits, u64 *value, void *handler_context, + void *reqion_context) +{ + struct acpi_gpio_chip_data *data = handler_context; + struct gpio_chip *chip = data->chip; + struct acpi_resource *ares; + int ret, gpio = -EINVAL; + unsigned long flags = 0; + acpi_status status; + + status = acpi_buffer_to_resource(data->ci.connection, data->ci.length, + &ares); + if (ACPI_FAILURE(status)) + return status; + + if (ares->type == ACPI_RESOURCE_TYPE_GPIO) { + const struct acpi_resource_gpio *agpio = &ares->data.gpio; + + switch (agpio->io_restriction) { + case ACPI_IO_RESTRICT_NONE: + flags = GPIOF_DIR_IN | GPIOF_DIR_OUT; + break; + case ACPI_IO_RESTRICT_INPUT: + flags = GPIOF_DIR_IN; + break; + case ACPI_IO_RESTRICT_OUTPUT: + flags = GPIOF_DIR_OUT; + break; + } + + gpio = acpi_get_gpio(agpio->resource_source.string_ptr, + agpio->pin_table[0]); + } + + ACPI_FREE(ares); + + if (!gpio_is_valid(gpio)) { + dev_err(chip->dev, "GpioOpReg: invalid GPIO resource\n"); + return AE_ERROR; + } + + ret = gpio_request_one(gpio, flags, "acpi_gpio_adr_space"); + if (ret) { + dev_err(chip->dev, "GpioOpReg: failed to request GPIO\n"); + return AE_ERROR; + } + + if (function == ACPI_WRITE) + gpio_set_value(gpio, (int)*value); + else + *value = gpio_get_value(gpio); + + gpio_free(gpio); + return AE_OK; +} + void acpi_gpiochip_add(struct gpio_chip *chip) { + struct acpi_gpio_chip_data *data; + acpi_handle handle; + acpi_status status; + + handle = ACPI_HANDLE(chip->dev); + if (!handle) + return; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return; + + status = acpi_attach_data(handle, acpi_gpio_chip_dh, data); + if (ACPI_FAILURE(status)) { + kfree(data); + return; + } + + data->chip = chip; + + status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, + acpi_gpio_adr_space_handler, + NULL, data); + if (ACPI_FAILURE(status)) { + acpi_detach_data(handle, acpi_gpio_chip_dh); + kfree(data); + return; + } + acpi_gpiochip_request_interrupts(chip); } EXPORT_SYMBOL_GPL(acpi_gpiochip_add); void acpi_gpiochip_remove(struct gpio_chip *chip) { + struct acpi_gpio_chip_data *data; + acpi_handle handle; + acpi_status status; + + handle = ACPI_HANDLE(chip->dev); + if (!handle) + return; + + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&data); + if (ACPI_FAILURE(status)) + return; + acpi_gpiochip_free_interrupts(chip); + acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO, + acpi_gpio_adr_space_handler); + acpi_detach_data(handle, acpi_gpio_chip_dh); + kfree(data); } EXPORT_SYMBOL_GPL(acpi_gpiochip_remove);
GPIO operation regions is a new feature introduced in ACPI 5.0 specification. In practise it means that now ASL code can toggle GPIOs with the help of the OS GPIO driver. An imaginary example of such ASL code: Device (\SB.GPIO) { // _REG is called when the operation region handler is // installed. Method (_REG) { ... } OperationRegion (GPOP, GeneralPurposeIo, 0, 0xc) Field (GPOP, ByteAcc, NoLock, Preserve) { Connection ( GpioIo (Exclusive, PullDefault, 0x0000, 0x0000, IoRestrictionOutputOnly, "\\_SB.GPIO", 0x00, ResourceConsumer,,) { 0x0005 } ), PWR0, 1 } ... } Device (\SB.DEV0) { ... Method (_PS0, 0, Serialized) { // Toggle the GPIO Store (1, \SB.GPIO.PWR0) } Method (_PS3, 0, Serialized) { Store (0, \SB.GPIO.PRW0) } } The device \SB.DEV0 is powered on by asserting \SB.GPIO.PWR0 GPIO line. So when the OS calls _PS0 or _PS3, that GPIO line should be set to 1 or 0 by the actual GPIO driver. In order to support GPIO operation regions we install ACPI address space handler for the device (provided that it has an ACPI handle). This handler uses the standard GPIO APIs to toggle the pin according to what the ASL code does. Since there is need to attach more data to the ACPI object, we create a new structure acpi_gpio_chip_data that contains data for both GPIO operation regions and for ACPI event handling and make the event handling code to use this new structure. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/gpio/gpiolib-acpi.c | 131 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 126 insertions(+), 5 deletions(-)