On Monday, February 24, 2014 06:00:07 PM Mika Westerberg wrote:
> We are going to add more ACPI specific data to accompany GPIO chip so
> instead of allocating it per each use-case we allocate it once when
> acpi_gpiochip_add() is called and release it when acpi_gpiochip_remove() is
> called.
>
> Doing this allows us to add more ACPI specific data by merely adding new
> fields to struct acpi_gpio_chip.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-acpi.c | 83 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 59 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index b7db098ba060..5f5f107c2099 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -26,6 +26,11 @@ struct acpi_gpio_evt_pin {
> unsigned int irq;
> };
>
> +struct acpi_gpio_chip {
> + struct gpio_chip *chip;
> + struct list_head *evt_pins;
Hmm. Why exactly evt_pins has to be a pointer?
> +};
> +
> static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> {
> if (!gc->dev)
> @@ -81,14 +86,14 @@ 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. */
> }
>
> /**
> * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
> - * @chip: gpio chip
> + * @achip: ACPI GPIO chip
> *
> * ACPI5 platforms can use GPIO signaled ACPI events. These GPIO interrupts are
> * handled by ACPI event methods which need to be called from the GPIO
> @@ -96,9 +101,10 @@ static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
> * gpio pins have acpi event methods and assigns interrupt handlers that calls
> * the acpi event methods for those pins.
> */
> -static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
> +static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
I would call the argument "acpi_gpio" instead of achip (and analogously below),
because the structure is a "chip plus some additional info".
> {
> struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> + struct gpio_chip *chip = achip->chip;
> struct acpi_resource *res;
> acpi_handle handle, evt_handle;
> struct list_head *evt_pins = NULL;
> @@ -123,12 +129,7 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
> evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL);
> if (evt_pins) {
> INIT_LIST_HEAD(evt_pins);
> - status = acpi_attach_data(handle, acpi_gpio_evt_dh,
> - evt_pins);
> - if (ACPI_FAILURE(status)) {
> - kfree(evt_pins);
> - evt_pins = NULL;
> - }
> + achip->evt_pins = evt_pins;
What about doing INIT_LIST_HEAD(&acpi_gpio->evt_pins) instead (if it's not a
pointer)?
> }
> }
>
> @@ -197,30 +198,24 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>
> /**
> * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> - * @chip: gpio chip
> + * @achip: ACPI GPIO chip
> *
> * Free interrupts associated with the _EVT method for the given GPIO chip.
> *
> * The remaining ACPI event interrupts associated with the chip are freed
> * automatically.
> */
> -static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> +static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> {
> - acpi_handle handle;
> - acpi_status status;
> struct list_head *evt_pins;
> struct acpi_gpio_evt_pin *evt_pin, *ep;
> + struct gpio_chip *chip = achip->chip;
>
> - if (!chip->dev || !chip->to_irq)
> - return;
> -
> - handle = ACPI_HANDLE(chip->dev);
> - if (!handle)
> + if (!chip->dev || !chip->to_irq || !achip->evt_pins)
> return;
>
> - status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
> - if (ACPI_FAILURE(status))
> - return;
> + evt_pins = achip->evt_pins;
> + achip->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);
> @@ -228,7 +223,6 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> kfree(evt_pin);
> }
>
> - acpi_detach_data(handle, acpi_gpio_evt_dh);
> kfree(evt_pins);
> }
>
> @@ -312,10 +306,51 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
>
> void acpi_gpiochip_add(struct gpio_chip *chip)
> {
> - acpi_gpiochip_request_interrupts(chip);
> + struct acpi_gpio_chip *achip;
> + acpi_handle handle;
> + acpi_status status;
> +
> + handle = ACPI_HANDLE(chip->dev);
> + if (!handle)
> + return;
> +
> + achip = kzalloc(sizeof(*achip), GFP_KERNEL);
> + if (!achip) {
> + dev_err(chip->dev,
> + "Failed to allocate memory for ACPI GPIO chip\n");
> + return;
> + }
> +
> + achip->chip = chip;
> +
> + status = acpi_attach_data(handle, acpi_gpio_chip_dh, achip);
To be honest, I'd prefer that to be associated with struct acpi_device rather
than with the handle, but that's not a big deal for now.
> + if (ACPI_FAILURE(status)) {
> + dev_err(chip->dev, "Failed to attach ACPI GPIO chip\n");
> + kfree(achip);
> + return;
> + }
> +
> + acpi_gpiochip_request_interrupts(achip);
> }
>
> void acpi_gpiochip_remove(struct gpio_chip *chip)
> {
> - acpi_gpiochip_free_interrupts(chip);
> + struct acpi_gpio_chip *achip;
> + acpi_handle handle;
> + acpi_status status;
> +
> + handle = ACPI_HANDLE(chip->dev);
> + if (!handle)
> + return;
> +
> + status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&achip);
> + if (ACPI_FAILURE(status)) {
> + dev_warn(chip->dev, "Failed to retrieve ACPI GPIO chip\n");
> + return;
> + }
> +
> + acpi_gpiochip_free_interrupts(achip);
> +
> + acpi_detach_data(handle, acpi_gpio_chip_dh);
> + kfree(achip);
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
