Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753076AbcJJOT6 (ORCPT ); Mon, 10 Oct 2016 10:19:58 -0400 Received: from mail-lf0-f66.google.com ([209.85.215.66]:33003 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752657AbcJJOTp (ORCPT ); Mon, 10 Oct 2016 10:19:45 -0400 Date: Mon, 10 Oct 2016 15:18:56 +0100 From: Rui Miguel Silva To: Linus Walleij Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org, Sandeep Patil , Viresh Kumar , linux-kernel@vger.kernel.org, Rui Miguel Silva , Axel Haslam , Johan Hovold Subject: Re: [PATCH v2] RFC: staging: greybus: shape up greybus GPIO Message-ID: <20161010141856.GA5156@arch-late.localdomain> References: <1476088772-6834-1-git-send-email-linus.walleij@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <1476088772-6834-1-git-send-email-linus.walleij@linaro.org> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13919 Lines: 389 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 >Cc: Axel Haslam >Cc: Johan Hovold >Cc: Sandeep Patil >Cc: Rui Miguel Silva >Cc: Greg Kroah-Hartman >Signed-off-by: Linus Walleij This was a necessary change for recent kernels. Looks good to me. Thanks. Reviewed-by: Rui Miguel Silva >--- >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 > #include > #include >-#include >+#include > #include > #include > #include >@@ -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