[PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[Linus Walleij]

Greybus GPIO seems to reimplement the already existing generic
gpiolib irqchip. Also use gpiochip_get_data(). Also use
devm_gpiochip_add_data().

Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Axel Haslam <haslam_axel@projectara.com>
Cc: Johan Hovold <johan@hovoldconsulting.com>
Cc: Sandeep Patil <patil_sandeep@projectara.com>
Cc: Rui Miguel Silva <rui.silva@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Add the hunk adding select GPIOLIB_IRQCHIP to Kconfig,
  sorry for sending patches too late in the night.

Greybus folks: please look at this. I expect something like this
to be applied before migrating from staging, I can't test it
obviously.
---
 drivers/staging/greybus/Kconfig |   1 +
 drivers/staging/greybus/gpio.c  | 183 +++++-----------------------------------
 2 files changed, 24 insertions(+), 160 deletions(-)

diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
index 50de2d72dde0..b05a8f4657e3 100644
--- a/drivers/staging/greybus/Kconfig
+++ b/drivers/staging/greybus/Kconfig
@@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
 config GREYBUS_GPIO
 	tristate "Greybus GPIO Bridged PHY driver"
 	depends on GPIOLIB
+	select GPIOLIB_IRQCHIP
 	---help---
 	  Select this option if you have a device that follows the
 	  Greybus GPIO Bridged PHY Class specification.
diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
index 5e06e4229e42..07ccd819ee1c 100644
--- a/drivers/staging/greybus/gpio.c
+++ b/drivers/staging/greybus/gpio.c
@@ -10,7 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/mutex.h>
@@ -41,14 +41,8 @@ struct gb_gpio_controller {
 	struct gpio_chip	chip;
 	struct irq_chip		irqc;
 	struct irq_chip		*irqchip;
-	struct irq_domain	*irqdomain;
-	unsigned int		irq_base;
-	irq_flow_handler_t	irq_handler;
-	unsigned int		irq_default_type;
 	struct mutex		irq_lock;
 };
-#define gpio_chip_to_gb_gpio_controller(chip) \
-	container_of(chip, struct gb_gpio_controller, chip)
 #define irq_data_to_gpio_chip(d) (d->domain->host_data)
 
 static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
@@ -277,7 +271,7 @@ static void _gb_gpio_irq_set_type(struct gb_gpio_controller *ggc,
 static void gb_gpio_irq_mask(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	line->masked = true;
@@ -287,7 +281,7 @@ static void gb_gpio_irq_mask(struct irq_data *d)
 static void gb_gpio_irq_unmask(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	line->masked = false;
@@ -297,7 +291,7 @@ static void gb_gpio_irq_unmask(struct irq_data *d)
 static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 	struct device *dev = &ggc->gbphy_dev->dev;
 	u8 irq_type;
@@ -335,7 +329,7 @@ static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 static void gb_gpio_irq_bus_lock(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	mutex_lock(&ggc->irq_lock);
 }
@@ -343,7 +337,7 @@ static void gb_gpio_irq_bus_lock(struct irq_data *d)
 static void gb_gpio_irq_bus_sync_unlock(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	if (line->irq_type_pending) {
@@ -392,7 +386,7 @@ static int gb_gpio_request_handler(struct gb_operation *op)
 		return -EINVAL;
 	}
 
-	irq = irq_find_mapping(ggc->irqdomain, event->which);
+	irq = irq_find_mapping(ggc->chip.irqdomain, event->which);
 	if (!irq) {
 		dev_err(dev, "failed to find IRQ\n");
 		return -EINVAL;
@@ -412,21 +406,21 @@ static int gb_gpio_request_handler(struct gb_operation *op)
 
 static int gb_gpio_request(struct gpio_chip *chip, unsigned offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_activate_operation(ggc, (u8)offset);
 }
 
 static void gb_gpio_free(struct gpio_chip *chip, unsigned offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	gb_gpio_deactivate_operation(ggc, (u8)offset);
 }
 
 static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u8 which;
 	int ret;
 
@@ -440,7 +434,7 @@ static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
 
 static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_direction_in_operation(ggc, (u8)offset);
 }
@@ -448,14 +442,14 @@ static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
 					int value)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_direction_out_operation(ggc, (u8)offset, !!value);
 }
 
 static int gb_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u8 which;
 	int ret;
 
@@ -469,7 +463,7 @@ static int gb_gpio_get(struct gpio_chip *chip, unsigned offset)
 
 static void gb_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	gb_gpio_set_value_operation(ggc, (u8)offset, !!value);
 }
@@ -477,7 +471,7 @@ static void gb_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 static int gb_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
 					unsigned debounce)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u16 usec;
 
 	if (debounce > U16_MAX)
@@ -504,134 +498,6 @@ static int gb_gpio_controller_setup(struct gb_gpio_controller *ggc)
 	return ret;
 }
 
-/**
- * gb_gpio_irq_map() - maps an IRQ into a GB gpio irqchip
- * @d: the irqdomain used by this irqchip
- * @irq: the global irq number used by this GB gpio irqchip irq
- * @hwirq: the local IRQ/GPIO line offset on this GB gpio
- *
- * This function will set up the mapping for a certain IRQ line on a
- * GB gpio by assigning the GB gpio as chip data, and using the irqchip
- * stored inside the GB gpio.
- */
-static int gb_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
-			   irq_hw_number_t hwirq)
-{
-	struct gpio_chip *chip = domain->host_data;
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	irq_set_chip_data(irq, ggc);
-	irq_set_chip_and_handler(irq, ggc->irqchip, ggc->irq_handler);
-	irq_set_noprobe(irq);
-	/*
-	 * No set-up of the hardware will happen if IRQ_TYPE_NONE
-	 * is passed as default type.
-	 */
-	if (ggc->irq_default_type != IRQ_TYPE_NONE)
-		irq_set_irq_type(irq, ggc->irq_default_type);
-
-	return 0;
-}
-
-static void gb_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
-{
-	irq_set_chip_and_handler(irq, NULL, NULL);
-	irq_set_chip_data(irq, NULL);
-}
-
-static const struct irq_domain_ops gb_gpio_domain_ops = {
-	.map	= gb_gpio_irq_map,
-	.unmap	= gb_gpio_irq_unmap,
-};
-
-/**
- * gb_gpio_irqchip_remove() - removes an irqchip added to a gb_gpio_controller
- * @ggc: the gb_gpio_controller to remove the irqchip from
- *
- * This is called only from gb_gpio_remove()
- */
-static void gb_gpio_irqchip_remove(struct gb_gpio_controller *ggc)
-{
-	unsigned int offset;
-
-	/* Remove all IRQ mappings and delete the domain */
-	if (ggc->irqdomain) {
-		for (offset = 0; offset < (ggc->line_max + 1); offset++)
-			irq_dispose_mapping(irq_find_mapping(ggc->irqdomain, offset));
-		irq_domain_remove(ggc->irqdomain);
-	}
-
-	if (ggc->irqchip)
-		ggc->irqchip = NULL;
-}
-
-/**
- * gb_gpio_irqchip_add() - adds an irqchip to a gpio chip
- * @chip: the gpio chip to add the irqchip to
- * @irqchip: the irqchip to add to the adapter
- * @first_irq: if not dynamically assigned, the base (first) IRQ to
- * allocate gpio irqs from
- * @handler: the irq handler to use (often a predefined irq core function)
- * @type: the default type for IRQs on this irqchip, pass IRQ_TYPE_NONE
- * to have the core avoid setting up any default type in the hardware.
- *
- * This function closely associates a certain irqchip with a certain
- * gpio chip, providing an irq domain to translate the local IRQs to
- * global irqs, and making sure that the gpio chip
- * is passed as chip data to all related functions. Driver callbacks
- * need to use container_of() to get their local state containers back
- * from the gpio chip passed as chip data. An irqdomain will be stored
- * in the gpio chip that shall be used by the driver to handle IRQ number
- * translation. The gpio chip will need to be initialized and registered
- * before calling this function.
- */
-static int gb_gpio_irqchip_add(struct gpio_chip *chip,
-			 struct irq_chip *irqchip,
-			 unsigned int first_irq,
-			 irq_flow_handler_t handler,
-			 unsigned int type)
-{
-	struct gb_gpio_controller *ggc;
-	unsigned int offset;
-	unsigned irq_base;
-
-	if (!chip || !irqchip)
-		return -EINVAL;
-
-	ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	ggc->irqchip = irqchip;
-	ggc->irq_handler = handler;
-	ggc->irq_default_type = type;
-	ggc->irqdomain = irq_domain_add_simple(NULL,
-					ggc->line_max + 1, first_irq,
-					&gb_gpio_domain_ops, chip);
-	if (!ggc->irqdomain) {
-		ggc->irqchip = NULL;
-		return -EINVAL;
-	}
-
-	/*
-	 * Prepare the mapping since the irqchip shall be orthogonal to
-	 * any gpio calls. If the first_irq was zero, this is
-	 * necessary to allocate descriptors for all IRQs.
-	 */
-	for (offset = 0; offset < (ggc->line_max + 1); offset++) {
-		irq_base = irq_create_mapping(ggc->irqdomain, offset);
-		if (offset == 0)
-			ggc->irq_base = irq_base;
-	}
-
-	return 0;
-}
-
-static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
-{
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	return irq_find_mapping(ggc->irqdomain, offset);
-}
-
 static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 			 const struct gbphy_device_id *id)
 {
@@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 	gpio->get = gb_gpio_get;
 	gpio->set = gb_gpio_set;
 	gpio->set_debounce = gb_gpio_set_debounce;
-	gpio->to_irq = gb_gpio_to_irq;
 	gpio->base = -1;		/* Allocate base dynamically */
 	gpio->ngpio = ggc->line_max + 1;
 	gpio->can_sleep = true;
@@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 	if (ret)
 		goto exit_line_free;
 
-	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
-				   handle_level_irq, IRQ_TYPE_NONE);
+	ret = devm_gpiochip_add_data(&connection->bundle->dev,
+				     gpio, ggc);
 	if (ret) {
 		dev_err(&connection->bundle->dev,
-			"failed to add irq chip: %d\n", ret);
+			"failed to add gpio chip: %d\n", ret);
 		goto exit_line_free;
 	}
 
-	ret = gpiochip_add(gpio);
+	ret = gpiochip_irqchip_add(gpio, irqc,
+				   0, handle_level_irq,
+				   IRQ_TYPE_NONE);
 	if (ret) {
 		dev_err(&connection->bundle->dev,
-			"failed to add gpio chip: %d\n", ret);
-		goto exit_gpio_irqchip_remove;
+			"failed to add gpio irqchip: %d\n", ret);
+		goto exit_line_free;
 	}
 
 	gbphy_runtime_put_autosuspend(gbphy_dev);
 	return 0;
 
-exit_gpio_irqchip_remove:
-	gb_gpio_irqchip_remove(ggc);
 exit_line_free:
 	kfree(ggc->lines);
 exit_connection_disable:
@@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
 		gbphy_runtime_get_noresume(gbphy_dev);
 
 	gb_connection_disable_rx(connection);
-	gpiochip_remove(&ggc->chip);
-	gb_gpio_irqchip_remove(ggc);
 	gb_connection_disable(connection);
 	gb_connection_destroy(connection);
 	kfree(ggc->lines);
-- 
2.7.4

Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[Rui Miguel Silva]

Hi Linus,
On Mon, Oct 10, 2016 at 10:39:32AM +0200, Linus Walleij wrote:
>Greybus GPIO seems to reimplement the already existing generic
>gpiolib irqchip. Also use gpiochip_get_data(). Also use
>devm_gpiochip_add_data().
>
>Cc: Viresh Kumar <viresh.kumar@linaro.org>
>Cc: Axel Haslam <haslam_axel@projectara.com>
>Cc: Johan Hovold <johan@hovoldconsulting.com>
>Cc: Sandeep Patil <patil_sandeep@projectara.com>
>Cc: Rui Miguel Silva <rui.silva@linaro.org>
>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

This was a necessary change for recent kernels.

Looks good to me. Thanks.
Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

>---
>ChangeLog v1->v2:
>- Add the hunk adding select GPIOLIB_IRQCHIP to Kconfig,
>  sorry for sending patches too late in the night.
>
>Greybus folks: please look at this. I expect something like this
>to be applied before migrating from staging, I can't test it
>obviously.
>---
> drivers/staging/greybus/Kconfig |   1 +
> drivers/staging/greybus/gpio.c  | 183 +++++-----------------------------------
> 2 files changed, 24 insertions(+), 160 deletions(-)
>
>diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
>index 50de2d72dde0..b05a8f4657e3 100644
>--- a/drivers/staging/greybus/Kconfig
>+++ b/drivers/staging/greybus/Kconfig
>@@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
> config GREYBUS_GPIO
> 	tristate "Greybus GPIO Bridged PHY driver"
> 	depends on GPIOLIB
>+	select GPIOLIB_IRQCHIP
> 	---help---
> 	  Select this option if you have a device that follows the
> 	  Greybus GPIO Bridged PHY Class specification.
>diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
>index 5e06e4229e42..07ccd819ee1c 100644
>--- a/drivers/staging/greybus/gpio.c
>+++ b/drivers/staging/greybus/gpio.c
>@@ -10,7 +10,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
>-#include <linux/gpio.h>
>+#include <linux/gpio/driver.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> #include <linux/mutex.h>
>@@ -41,14 +41,8 @@ struct gb_gpio_controller {
> 	struct gpio_chip	chip;
> 	struct irq_chip		irqc;
> 	struct irq_chip		*irqchip;
>-	struct irq_domain	*irqdomain;
>-	unsigned int		irq_base;
>-	irq_flow_handler_t	irq_handler;
>-	unsigned int		irq_default_type;
> 	struct mutex		irq_lock;
> };
>-#define gpio_chip_to_gb_gpio_controller(chip) \
>-	container_of(chip, struct gb_gpio_controller, chip)
> #define irq_data_to_gpio_chip(d) (d->domain->host_data)
>
> static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
>@@ -277,7 +271,7 @@ static void _gb_gpio_irq_set_type(struct gb_gpio_controller *ggc,
> static void gb_gpio_irq_mask(struct irq_data *d)
> {
> 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
>
> 	line->masked = true;
>@@ -287,7 +281,7 @@ static void gb_gpio_irq_mask(struct irq_data *d)
> static void gb_gpio_irq_unmask(struct irq_data *d)
> {
> 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
>
> 	line->masked = false;
>@@ -297,7 +291,7 @@ static void gb_gpio_irq_unmask(struct irq_data *d)
> static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> {
> 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
> 	struct device *dev = &ggc->gbphy_dev->dev;
> 	u8 irq_type;
>@@ -335,7 +329,7 @@ static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> static void gb_gpio_irq_bus_lock(struct irq_data *d)
> {
> 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	mutex_lock(&ggc->irq_lock);
> }
>@@ -343,7 +337,7 @@ static void gb_gpio_irq_bus_lock(struct irq_data *d)
> static void gb_gpio_irq_bus_sync_unlock(struct irq_data *d)
> {
> 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
>
> 	if (line->irq_type_pending) {
>@@ -392,7 +386,7 @@ static int gb_gpio_request_handler(struct gb_operation *op)
> 		return -EINVAL;
> 	}
>
>-	irq = irq_find_mapping(ggc->irqdomain, event->which);
>+	irq = irq_find_mapping(ggc->chip.irqdomain, event->which);
> 	if (!irq) {
> 		dev_err(dev, "failed to find IRQ\n");
> 		return -EINVAL;
>@@ -412,21 +406,21 @@ static int gb_gpio_request_handler(struct gb_operation *op)
>
> static int gb_gpio_request(struct gpio_chip *chip, unsigned offset)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	return gb_gpio_activate_operation(ggc, (u8)offset);
> }
>
> static void gb_gpio_free(struct gpio_chip *chip, unsigned offset)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	gb_gpio_deactivate_operation(ggc, (u8)offset);
> }
>
> static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	u8 which;
> 	int ret;
>
>@@ -440,7 +434,7 @@ static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
>
> static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	return gb_gpio_direction_in_operation(ggc, (u8)offset);
> }
>@@ -448,14 +442,14 @@ static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
> 					int value)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	return gb_gpio_direction_out_operation(ggc, (u8)offset, !!value);
> }
>
> static int gb_gpio_get(struct gpio_chip *chip, unsigned offset)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	u8 which;
> 	int ret;
>
>@@ -469,7 +463,7 @@ static int gb_gpio_get(struct gpio_chip *chip, unsigned offset)
>
> static void gb_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
>
> 	gb_gpio_set_value_operation(ggc, (u8)offset, !!value);
> }
>@@ -477,7 +471,7 @@ static void gb_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> static int gb_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
> 					unsigned debounce)
> {
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 	u16 usec;
>
> 	if (debounce > U16_MAX)
>@@ -504,134 +498,6 @@ static int gb_gpio_controller_setup(struct gb_gpio_controller *ggc)
> 	return ret;
> }
>
>-/**
>- * gb_gpio_irq_map() - maps an IRQ into a GB gpio irqchip
>- * @d: the irqdomain used by this irqchip
>- * @irq: the global irq number used by this GB gpio irqchip irq
>- * @hwirq: the local IRQ/GPIO line offset on this GB gpio
>- *
>- * This function will set up the mapping for a certain IRQ line on a
>- * GB gpio by assigning the GB gpio as chip data, and using the irqchip
>- * stored inside the GB gpio.
>- */
>-static int gb_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
>-			   irq_hw_number_t hwirq)
>-{
>-	struct gpio_chip *chip = domain->host_data;
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>-
>-	irq_set_chip_data(irq, ggc);
>-	irq_set_chip_and_handler(irq, ggc->irqchip, ggc->irq_handler);
>-	irq_set_noprobe(irq);
>-	/*
>-	 * No set-up of the hardware will happen if IRQ_TYPE_NONE
>-	 * is passed as default type.
>-	 */
>-	if (ggc->irq_default_type != IRQ_TYPE_NONE)
>-		irq_set_irq_type(irq, ggc->irq_default_type);
>-
>-	return 0;
>-}
>-
>-static void gb_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
>-{
>-	irq_set_chip_and_handler(irq, NULL, NULL);
>-	irq_set_chip_data(irq, NULL);
>-}
>-
>-static const struct irq_domain_ops gb_gpio_domain_ops = {
>-	.map	= gb_gpio_irq_map,
>-	.unmap	= gb_gpio_irq_unmap,
>-};
>-
>-/**
>- * gb_gpio_irqchip_remove() - removes an irqchip added to a gb_gpio_controller
>- * @ggc: the gb_gpio_controller to remove the irqchip from
>- *
>- * This is called only from gb_gpio_remove()
>- */
>-static void gb_gpio_irqchip_remove(struct gb_gpio_controller *ggc)
>-{
>-	unsigned int offset;
>-
>-	/* Remove all IRQ mappings and delete the domain */
>-	if (ggc->irqdomain) {
>-		for (offset = 0; offset < (ggc->line_max + 1); offset++)
>-			irq_dispose_mapping(irq_find_mapping(ggc->irqdomain, offset));
>-		irq_domain_remove(ggc->irqdomain);
>-	}
>-
>-	if (ggc->irqchip)
>-		ggc->irqchip = NULL;
>-}
>-
>-/**
>- * gb_gpio_irqchip_add() - adds an irqchip to a gpio chip
>- * @chip: the gpio chip to add the irqchip to
>- * @irqchip: the irqchip to add to the adapter
>- * @first_irq: if not dynamically assigned, the base (first) IRQ to
>- * allocate gpio irqs from
>- * @handler: the irq handler to use (often a predefined irq core function)
>- * @type: the default type for IRQs on this irqchip, pass IRQ_TYPE_NONE
>- * to have the core avoid setting up any default type in the hardware.
>- *
>- * This function closely associates a certain irqchip with a certain
>- * gpio chip, providing an irq domain to translate the local IRQs to
>- * global irqs, and making sure that the gpio chip
>- * is passed as chip data to all related functions. Driver callbacks
>- * need to use container_of() to get their local state containers back
>- * from the gpio chip passed as chip data. An irqdomain will be stored
>- * in the gpio chip that shall be used by the driver to handle IRQ number
>- * translation. The gpio chip will need to be initialized and registered
>- * before calling this function.
>- */
>-static int gb_gpio_irqchip_add(struct gpio_chip *chip,
>-			 struct irq_chip *irqchip,
>-			 unsigned int first_irq,
>-			 irq_flow_handler_t handler,
>-			 unsigned int type)
>-{
>-	struct gb_gpio_controller *ggc;
>-	unsigned int offset;
>-	unsigned irq_base;
>-
>-	if (!chip || !irqchip)
>-		return -EINVAL;
>-
>-	ggc = gpio_chip_to_gb_gpio_controller(chip);
>-
>-	ggc->irqchip = irqchip;
>-	ggc->irq_handler = handler;
>-	ggc->irq_default_type = type;
>-	ggc->irqdomain = irq_domain_add_simple(NULL,
>-					ggc->line_max + 1, first_irq,
>-					&gb_gpio_domain_ops, chip);
>-	if (!ggc->irqdomain) {
>-		ggc->irqchip = NULL;
>-		return -EINVAL;
>-	}
>-
>-	/*
>-	 * Prepare the mapping since the irqchip shall be orthogonal to
>-	 * any gpio calls. If the first_irq was zero, this is
>-	 * necessary to allocate descriptors for all IRQs.
>-	 */
>-	for (offset = 0; offset < (ggc->line_max + 1); offset++) {
>-		irq_base = irq_create_mapping(ggc->irqdomain, offset);
>-		if (offset == 0)
>-			ggc->irq_base = irq_base;
>-	}
>-
>-	return 0;
>-}
>-
>-static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
>-{
>-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
>-
>-	return irq_find_mapping(ggc->irqdomain, offset);
>-}
>-
> static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
> 			 const struct gbphy_device_id *id)
> {
>@@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
> 	gpio->get = gb_gpio_get;
> 	gpio->set = gb_gpio_set;
> 	gpio->set_debounce = gb_gpio_set_debounce;
>-	gpio->to_irq = gb_gpio_to_irq;
> 	gpio->base = -1;		/* Allocate base dynamically */
> 	gpio->ngpio = ggc->line_max + 1;
> 	gpio->can_sleep = true;
>@@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
> 	if (ret)
> 		goto exit_line_free;
>
>-	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
>-				   handle_level_irq, IRQ_TYPE_NONE);
>+	ret = devm_gpiochip_add_data(&connection->bundle->dev,
>+				     gpio, ggc);
> 	if (ret) {
> 		dev_err(&connection->bundle->dev,
>-			"failed to add irq chip: %d\n", ret);
>+			"failed to add gpio chip: %d\n", ret);
> 		goto exit_line_free;
> 	}
>
>-	ret = gpiochip_add(gpio);
>+	ret = gpiochip_irqchip_add(gpio, irqc,
>+				   0, handle_level_irq,
>+				   IRQ_TYPE_NONE);
> 	if (ret) {
> 		dev_err(&connection->bundle->dev,
>-			"failed to add gpio chip: %d\n", ret);
>-		goto exit_gpio_irqchip_remove;
>+			"failed to add gpio irqchip: %d\n", ret);
>+		goto exit_line_free;
> 	}
>
> 	gbphy_runtime_put_autosuspend(gbphy_dev);
> 	return 0;
>
>-exit_gpio_irqchip_remove:
>-	gb_gpio_irqchip_remove(ggc);
> exit_line_free:
> 	kfree(ggc->lines);
> exit_connection_disable:
>@@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
> 		gbphy_runtime_get_noresume(gbphy_dev);
>
> 	gb_connection_disable_rx(connection);
>-	gpiochip_remove(&ggc->chip);
>-	gb_gpio_irqchip_remove(ggc);
> 	gb_connection_disable(connection);
> 	gb_connection_destroy(connection);
> 	kfree(ggc->lines);
>-- 
>2.7.4
>
>_______________________________________________
>devel mailing list
>devel@linuxdriverproject.org
>http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[Viresh Kumar]

I was looking to exactly do thanks, thanks for doing it before us :)

On 10-10-16, 10:39, Linus Walleij wrote:
>  static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  			 const struct gbphy_device_id *id)
>  {
> @@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	gpio->get = gb_gpio_get;
>  	gpio->set = gb_gpio_set;
>  	gpio->set_debounce = gb_gpio_set_debounce;
> -	gpio->to_irq = gb_gpio_to_irq;
>  	gpio->base = -1;		/* Allocate base dynamically */
>  	gpio->ngpio = ggc->line_max + 1;
>  	gpio->can_sleep = true;
> @@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	if (ret)
>  		goto exit_line_free;
>  
> -	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
> -				   handle_level_irq, IRQ_TYPE_NONE);
> +	ret = devm_gpiochip_add_data(&connection->bundle->dev,

We should be using gbphy_dev->dev here instead.

> +				     gpio, ggc);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,

And also at these places. I will send a patch to fix these separately though.

> -			"failed to add irq chip: %d\n", ret);
> +			"failed to add gpio chip: %d\n", ret);
>  		goto exit_line_free;
>  	}
>  
> -	ret = gpiochip_add(gpio);
> +	ret = gpiochip_irqchip_add(gpio, irqc,
> +				   0, handle_level_irq,
> +				   IRQ_TYPE_NONE);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add gpio chip: %d\n", ret);
> -		goto exit_gpio_irqchip_remove;
> +			"failed to add gpio irqchip: %d\n", ret);
> +		goto exit_line_free;
>  	}
>  
>  	gbphy_runtime_put_autosuspend(gbphy_dev);
>  	return 0;
>  
> -exit_gpio_irqchip_remove:
> -	gb_gpio_irqchip_remove(ggc);
>  exit_line_free:
>  	kfree(ggc->lines);
>  exit_connection_disable:
> @@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
>  		gbphy_runtime_get_noresume(gbphy_dev);
>  
>  	gb_connection_disable_rx(connection);
> -	gpiochip_remove(&ggc->chip);

This should stay as is, right?

> -	gb_gpio_irqchip_remove(ggc);
>  	gb_connection_disable(connection);
>  	gb_connection_destroy(connection);
>  	kfree(ggc->lines);

-- 
viresh

Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO

[Johan Hovold]

On Mon, Oct 10, 2016 at 10:39:32AM +0200, Linus Walleij wrote:
> Greybus GPIO seems to reimplement the already existing generic
> gpiolib irqchip. Also use gpiochip_get_data(). Also use
> devm_gpiochip_add_data().
> 
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Axel Haslam <haslam_axel@projectara.com>
> Cc: Johan Hovold <johan@hovoldconsulting.com>
> Cc: Sandeep Patil <patil_sandeep@projectara.com>
> Cc: Rui Miguel Silva <rui.silva@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Add the hunk adding select GPIOLIB_IRQCHIP to Kconfig,
>   sorry for sending patches too late in the night.
> 
> Greybus folks: please look at this. I expect something like this
> to be applied before migrating from staging, I can't test it
> obviously.

Yeah, ripping out the inline gpiolib-irqchip backport has been on the
TODO list for quite some time now. This was used to support some ancient
vendor kernels, which we no longer need to worry about. Thanks for
taking a stab at it.

But please split this up in two patches for the gpiolib-irqchip
conversion and the switch to using the new gpiochip_add_data()
interface.

>  static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  			 const struct gbphy_device_id *id)
>  {
> @@ -690,7 +556,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	gpio->get = gb_gpio_get;
>  	gpio->set = gb_gpio_set;
>  	gpio->set_debounce = gb_gpio_set_debounce;
> -	gpio->to_irq = gb_gpio_to_irq;
>  	gpio->base = -1;		/* Allocate base dynamically */
>  	gpio->ngpio = ggc->line_max + 1;
>  	gpio->can_sleep = true;
> @@ -699,26 +564,26 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
>  	if (ret)
>  		goto exit_line_free;
>  
> -	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
> -				   handle_level_irq, IRQ_TYPE_NONE);
> +	ret = devm_gpiochip_add_data(&connection->bundle->dev,

As Viresh already noted, this should use the gbphy_dev device that is
being probed rather than the parent bundle device.

> +				     gpio, ggc);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add irq chip: %d\n", ret);
> +			"failed to add gpio chip: %d\n", ret);
>  		goto exit_line_free;
>  	}
>  
> -	ret = gpiochip_add(gpio);
> +	ret = gpiochip_irqchip_add(gpio, irqc,
> +				   0, handle_level_irq,
> +				   IRQ_TYPE_NONE);
>  	if (ret) {
>  		dev_err(&connection->bundle->dev,
> -			"failed to add gpio chip: %d\n", ret);
> -		goto exit_gpio_irqchip_remove;
> +			"failed to add gpio irqchip: %d\n", ret);
> +		goto exit_line_free;
>  	}
>  
>  	gbphy_runtime_put_autosuspend(gbphy_dev);
>  	return 0;
>  
> -exit_gpio_irqchip_remove:
> -	gb_gpio_irqchip_remove(ggc);

You need to deregister the gpiochip here, before parts of our state is
freed.

>  exit_line_free:
>  	kfree(ggc->lines);
>  exit_connection_disable:

And the connection should really have been disabled before freeing lines
as well (separate prior bug).

> @@ -741,8 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
>  		gbphy_runtime_get_noresume(gbphy_dev);
>  
>  	gb_connection_disable_rx(connection);
> -	gpiochip_remove(&ggc->chip);
> -	gb_gpio_irqchip_remove(ggc);

This is similarly problematic, as again parts of our private state is
released below while the gpiochip is still registered.

I suggest simply not using the devres interface in probe.

>  	gb_connection_disable(connection);
>  	gb_connection_destroy(connection);
>  	kfree(ggc->lines);

Thanks,
Johan