diff mbox

[2/6] gpio / ACPI: Allocate ACPI specific data directly in acpi_gpiochip_add()

Message ID 1393257611-18031-3-git-send-email-mika.westerberg@linux.intel.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Mika Westerberg Feb. 24, 2014, 4 p.m. UTC 
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@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 83 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 59 insertions(+), 24 deletions(-)

Comments

Rafael J. Wysocki Feb. 25, 2014, 2:21 p.m. UTC | #1 
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@linux.intel.com>
> ---
>  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);
>  }
>
Mika Westerberg Feb. 26, 2014, 9:08 a.m. UTC | #2 
On Tue, Feb 25, 2014 at 03:21:55PM +0100, Rafael J. Wysocki wrote:
> 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@linux.intel.com>
> > ---
> >  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".

OK.

> 
> >  {
> >  	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.

OK, we can do that later if needed.
--
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 mbox

Patch

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;
+};
+
 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)
 {
 	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;
 		}
 	}
 
@@ -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);
+	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);
 }