| Message ID | 20200730135536.19747-1-Sergey.Semin@baikalelectronics.ru |
|---|---|
| Headers | show |
| Series | gpio: dwapb: Refactor GPIO resources initialization | expand |
On Thu, Jul 30, 2020 at 04:55:30PM +0300, Serge Semin wrote: > Add a new macro DWAPB_MAX_GPIOS which defines the maximum possible number > of GPIO lines corresponding to the maximum DW APB GPIO controller port > width. Use the new macro instead of number literal 32 where it's > applicable. Since it's a modified version of what I sent earlier, perhaps Suggested-by?
On Thu, Jul 30, 2020 at 05:05:26PM +0300, Andy Shevchenko wrote: > On Thu, Jul 30, 2020 at 04:55:30PM +0300, Serge Semin wrote: > > Add a new macro DWAPB_MAX_GPIOS which defines the maximum possible number > > of GPIO lines corresponding to the maximum DW APB GPIO controller port > > width. Use the new macro instead of number literal 32 where it's > > applicable. > > Since it's a modified version of what I sent earlier, perhaps Suggested-by? Could you point out to the message with that change? I must have missed that...( -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
Wou, I've confused my SOB tag here. Linus, if no additional patchset revision is required, could you please replace it with: Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> ? Alternatively I could resend the series with correct version of the tag. -Sergey On Thu, Jul 30, 2020 at 04:55:26PM +0300, Serge Semin wrote: > This series is about the DW APB GPIO device initialization procedure > cleaning up. First of all it has been discovered that having a > vendor-specific "snps,nr-gpios" property isn't only redundant but also > might be dangerous (see the commit log for details). Instead we suggest to > use the generic "ngpios" property to define a number of GPIOs each DW APB > GPIO controller port supports. Secondly seeing a tendency of the other > GPIO drivers getting converted to using the GPIO-lib-based IRQ-chip > interface this series provides a patch, which replaces the DW APB GPIO > driver Generic IRQ-chip implementation with the GPIO-lib IRQ-chip one. > Finally the DW APB GPIO device probe procedure is simplified by > converting the code to be using the device managed resources for the > reference clocks initialization, reset control assertion/de-assertion > and GPIO-chip registration. > > Some additional cleanups like replacing a number of GPIOs literal with a > corresponding macro and grouping the IRQ handlers up in a single place of > the driver are also introduced in this patchset. > > Link: https://lore.kernel.org/linux-gpio/20200723013858.10766-1-Sergey.Semin@baikalelectronics.ru/ > Changelog v2: > - Replace gc->to_irq() with irq_find_mapping() method. > - Refactor dwapb_irq_set_type() method to directly set IRQ flow handler > instead of using a temporary variable. > - Initialize GPIOlib IRQ-chip settings before calling request_irq() > method. > - Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio: > dwapb: use a second irq chip"). > - Move the acpi_gpiochip_{request,free}_interrupts() methods invocation > removal to a dedicated patch. > - Move GPIO-chip to_irq callback removal to a dedicated patch. > - Add a patch which replaces a max number of GPIO literals with a macro. > - Introduce dwapb_convert_irqs() method to convert the sparse parental > IRQs array into an array of linearly distributed IRQs correctly > perceived by GPIO-lib. > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> > Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: linux-gpio@vger.kernel.org > Cc: devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > > Serge Semin (10): > dt-bindings: gpio: dwapb: Add ngpios property support > gpio: dwapb: Add ngpios DT-property support > gpio: dwapb: Move MFD-specific IRQ handler > gpio: dwapb: Add max GPIOs macro > gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip > gpio: dwapb: Discard GPIO-to-IRQ mapping function > gpio: dwapb: Discard ACPI GPIO-chip IRQs request > gpio: dwapb: Get reset control by means of resource managed interface > gpio: dwapb: Get clocks by means of resource managed interface > gpio: dwapb: Use resource managed GPIO-chip add data method > > .../bindings/gpio/snps,dw-apb-gpio.yaml | 6 + > drivers/gpio/Kconfig | 2 +- > drivers/gpio/gpio-dwapb.c | 340 +++++++++--------- > include/linux/platform_data/gpio-dwapb.h | 4 +- > 4 files changed, 178 insertions(+), 174 deletions(-) > > -- > 2.27.0 >
On Thu, Jul 30, 2020 at 05:05:26PM +0300, Andy Shevchenko wrote: > On Thu, Jul 30, 2020 at 04:55:30PM +0300, Serge Semin wrote: > > Add a new macro DWAPB_MAX_GPIOS which defines the maximum possible number > > of GPIO lines corresponding to the maximum DW APB GPIO controller port > > width. Use the new macro instead of number literal 32 where it's > > applicable. > > Since it's a modified version of what I sent earlier, perhaps Suggested-by? Oh, found it [PATCH v1 6/6] gpio: dwapb: Define magic number for IRQ and GPIO lines Sorry for missing your suggested-by tag. I'll add it if new revision is required. Otherwise, Linus, could you add it when you apply the series? -Sergey > > -- > With Best Regards, > Andy Shevchenko > >
On Thu, Jul 30, 2020 at 04:55:31PM +0300, Serge Semin wrote: > GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on > top of a GPIO chip. It's better from maintainability and readability > point of view to use one instead of supporting a hand-written Generic > IRQ-chip-based implementation. Moreover the new implementation won't > cause much functional overhead but will provide a cleaner driver code. > All of that makes the DW APB GPIO driver conversion pretty much justified > especially seeing a tendency of the other GPIO drivers getting converted > too. > > Here is what we do in the framework of this commit to convert the driver > to using the GPIO-lib-based IRQ-chip interface: > 1) IRQ ack, mask and unmask callbacks are locally defined instead of > using the Generic IRQ-chip ones. Easy to read if you put blank lines in between of items. > 2) An irq_chip structure instance is embedded into the dwapb_gpio > private data. Note we can't have a static instance of that structure since > GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks(). > A warning about that would have been printed by the GPIO-lib code if we > used a single irq_chip structure instance for multiple DW APB GPIO > controllers. > 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip > descriptor. By default there is no IRQ enabled so any event raised will be > handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core > is synthesized to have non-shared reference IRQ-lines, then as before the > hierarchical and cascaded cases are distinguished by checking how many > parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't > initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP > is used on a platform with shared IRQ line, then we simply won't let the > GPIO-lib to initialize the parental IRQs, but will handle them locally in > the driver. > 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since > GPIO-lib IRQ-chip interface will create a new domain and accept a standard > IRQ-chip structure pointer based on the setting we provided in the > gpio_irq_chip structure. > 5) Manually select a proper IRQ flow handler directly in the > irq_set_type() callback by calling irq_set_handler_locked() method, since > an ordinary (not Generic) irq_chip descriptor is now utilized. Note this > shalln't give any regression > 6) Alter CONFIG_GPIO_DWAPB kernel config to select > CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP. > > Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5 > ("gpio: dwapb: use a second irq chip"), since the later isn't properly > used here anyway. ... > struct dwapb_gpio_port { > struct gpio_chip gc; > + unsigned int nr_irqs; > + unsigned int irq[DWAPB_MAX_GPIOS]; > + struct irq_chip irqchip; > bool is_registered; > struct dwapb_gpio *gpio; Isn't it too much wasted memory (imagine 4 port controller)? What if we have it in another structure and allocate dynamically? struct dwapb_gpio_port_irqchip { struct irq_chip irqchip; unsigned int nr_irqs; unsigned int irq[DWAPB_MAX_GPIOS]; }; ... struct dwapb_gpio_port_irqchip *pirq; ... (I agree that IRQ chip is rather property of a port than controller)
On Thu, Jul 30, 2020 at 04:55:32PM +0300, Serge Semin wrote: > Since GPIOlib-based IRQ-chip interface is now utilized there is no need in > setting up a custom GPIO-to-IRQ mapping method. GPIO-lib defines the > standard mapping method - gpiochip_to_irq(), which will be used anyway no > matter whether the custom to_irq callback is specified or not. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > Changelog v2: > - This is a new patch detached from commit > "gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip". > --- > drivers/gpio/gpio-dwapb.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 327333fbc750..f7acc5abbf5c 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -150,14 +150,6 @@ static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset, > gc->write_reg(reg_base + gpio_reg_convert(gpio, offset), val); > } > > -static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset) > -{ > - struct dwapb_gpio_port *port = gpiochip_get_data(gc); > - struct dwapb_gpio *gpio = port->gpio; > - > - return irq_find_mapping(gpio->domain, offset); > -} > - > static struct dwapb_gpio_port *dwapb_offs_to_port(struct dwapb_gpio *gpio, unsigned int offs) > { > struct dwapb_gpio_port *port; > @@ -466,8 +458,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio, > } > > girq->chip = &port->irqchip; > - > - port->gc.to_irq = dwapb_gpio_to_irq; > } > > static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > -- > 2.27.0 >
On Thu, Jul 30, 2020 at 04:55:33PM +0300, Serge Semin wrote: > Since GPIOlib-based IRQ-chip interface is now utilized there is no need > in calling the methods acpi_gpiochip_{request,free}_interrupts() here. > They will be called from gpiochip_add_irqchip()/gpiochip_irqchip_remove() > anyway. Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > Changelog v2: > - This is a new patch detached from commit > "gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip". > --- > drivers/gpio/gpio-dwapb.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index f7acc5abbf5c..226d9c2d9493 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -512,9 +512,6 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio, > return err; > } > > - /* Add GPIO-signaled ACPI event support */ > - acpi_gpiochip_request_interrupts(&port->gc); > - > port->is_registered = true; > > return 0; > @@ -530,7 +527,6 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio) > if (!port->is_registered) > continue; > > - acpi_gpiochip_free_interrupts(&port->gc); > gpiochip_remove(&port->gc); > } > } > -- > 2.27.0 >
On Thu, Jul 30, 2020 at 05:26:18PM +0300, Andy Shevchenko wrote: > On Thu, Jul 30, 2020 at 04:55:31PM +0300, Serge Semin wrote: > > GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on > > top of a GPIO chip. It's better from maintainability and readability > > point of view to use one instead of supporting a hand-written Generic > > IRQ-chip-based implementation. Moreover the new implementation won't > > cause much functional overhead but will provide a cleaner driver code. > > All of that makes the DW APB GPIO driver conversion pretty much justified > > especially seeing a tendency of the other GPIO drivers getting converted > > too. > > > > Here is what we do in the framework of this commit to convert the driver > > to using the GPIO-lib-based IRQ-chip interface: > > 1) IRQ ack, mask and unmask callbacks are locally defined instead of > > using the Generic IRQ-chip ones. > > Easy to read if you put blank lines in between of items. Ok. > > > 2) An irq_chip structure instance is embedded into the dwapb_gpio > > private data. Note we can't have a static instance of that structure since > > GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks(). > > A warning about that would have been printed by the GPIO-lib code if we > > used a single irq_chip structure instance for multiple DW APB GPIO > > controllers. > > 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip > > descriptor. By default there is no IRQ enabled so any event raised will be > > handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core > > is synthesized to have non-shared reference IRQ-lines, then as before the > > hierarchical and cascaded cases are distinguished by checking how many > > parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't > > initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP > > is used on a platform with shared IRQ line, then we simply won't let the > > GPIO-lib to initialize the parental IRQs, but will handle them locally in > > the driver. > > 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since > > GPIO-lib IRQ-chip interface will create a new domain and accept a standard > > IRQ-chip structure pointer based on the setting we provided in the > > gpio_irq_chip structure. > > 5) Manually select a proper IRQ flow handler directly in the > > irq_set_type() callback by calling irq_set_handler_locked() method, since > > an ordinary (not Generic) irq_chip descriptor is now utilized. Note this > > shalln't give any regression > > 6) Alter CONFIG_GPIO_DWAPB kernel config to select > > CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP. > > > > Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5 > > ("gpio: dwapb: use a second irq chip"), since the later isn't properly > > used here anyway. > > ... > > > struct dwapb_gpio_port { > > struct gpio_chip gc; > > + unsigned int nr_irqs; > > + unsigned int irq[DWAPB_MAX_GPIOS]; > > + struct irq_chip irqchip; > > bool is_registered; > > struct dwapb_gpio *gpio; > > Isn't it too much wasted memory (imagine 4 port controller)? > > What if we have it in another structure and allocate dynamically? > > struct dwapb_gpio_port_irqchip { > struct irq_chip irqchip; > unsigned int nr_irqs; > unsigned int irq[DWAPB_MAX_GPIOS]; > }; Agree. I have to send a new revision of the series anyway. I'll do that shortly. -Sergey > > ... > struct dwapb_gpio_port_irqchip *pirq; > ... > > (I agree that IRQ chip is rather property of a port than controller) > > -- > With Best Regards, > Andy Shevchenko > >
This series is about the DW APB GPIO device initialization procedure cleaning up. First of all it has been discovered that having a vendor-specific "snps,nr-gpios" property isn't only redundant but also might be dangerous (see the commit log for details). Instead we suggest to use the generic "ngpios" property to define a number of GPIOs each DW APB GPIO controller port supports. Secondly seeing a tendency of the other GPIO drivers getting converted to using the GPIO-lib-based IRQ-chip interface this series provides a patch, which replaces the DW APB GPIO driver Generic IRQ-chip implementation with the GPIO-lib IRQ-chip one. Finally the DW APB GPIO device probe procedure is simplified by converting the code to be using the device managed resources for the reference clocks initialization, reset control assertion/de-assertion and GPIO-chip registration. Some additional cleanups like replacing a number of GPIOs literal with a corresponding macro and grouping the IRQ handlers up in a single place of the driver are also introduced in this patchset. Link: https://lore.kernel.org/linux-gpio/20200723013858.10766-1-Sergey.Semin@baikalelectronics.ru/ Changelog v2: - Replace gc->to_irq() with irq_find_mapping() method. - Refactor dwapb_irq_set_type() method to directly set IRQ flow handler instead of using a temporary variable. - Initialize GPIOlib IRQ-chip settings before calling request_irq() method. - Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio: dwapb: use a second irq chip"). - Move the acpi_gpiochip_{request,free}_interrupts() methods invocation removal to a dedicated patch. - Move GPIO-chip to_irq callback removal to a dedicated patch. - Add a patch which replaces a max number of GPIO literals with a macro. - Introduce dwapb_convert_irqs() method to convert the sparse parental IRQs array into an array of linearly distributed IRQs correctly perceived by GPIO-lib. Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> Cc: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru> Cc: Rob Herring <robh+dt@kernel.org> Cc: linux-gpio@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Serge Semin (10): dt-bindings: gpio: dwapb: Add ngpios property support gpio: dwapb: Add ngpios DT-property support gpio: dwapb: Move MFD-specific IRQ handler gpio: dwapb: Add max GPIOs macro gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip gpio: dwapb: Discard GPIO-to-IRQ mapping function gpio: dwapb: Discard ACPI GPIO-chip IRQs request gpio: dwapb: Get reset control by means of resource managed interface gpio: dwapb: Get clocks by means of resource managed interface gpio: dwapb: Use resource managed GPIO-chip add data method .../bindings/gpio/snps,dw-apb-gpio.yaml | 6 + drivers/gpio/Kconfig | 2 +- drivers/gpio/gpio-dwapb.c | 340 +++++++++--------- include/linux/platform_data/gpio-dwapb.h | 4 +- 4 files changed, 178 insertions(+), 174 deletions(-)