On Mon, Nov 12, 2018 at 04:15:09PM +0100, Johan Hovold wrote:
> On Fri, Nov 09, 2018 at 01:17:41PM +0530, Nishad Kamdar wrote:
> > Convert the GPIO driver to use the GPIO irqchip library
> > GPIOLIB_IRQCHIP instead of reimplementing the same.
>
> Thanks for picking this up. Linus W took a stab at it a couple of years
> ago, but it was never completed. You may want to take a look at that
> thread:
>
>
> https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.wall...@linaro.org
Thanks for the reference.
>
> > Signed-off-by: Nishad Kamdar <nishadkam...@gmail.com>
> > ---
> > drivers/staging/greybus/Kconfig | 1 +
> > drivers/staging/greybus/gpio.c | 123 ++++++--------------------------
> > 2 files changed, 21 insertions(+), 103 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/Kconfig
> > b/drivers/staging/greybus/Kconfig
> > index ab096bcef98c..b571e4e8060b 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 b1d4698019a1..32c228bad33a 100644
> > --- a/drivers/staging/greybus/gpio.c
> > +++ b/drivers/staging/greybus/gpio.c
> > @@ -9,9 +9,7 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/slab.h>
> > -#include <linux/gpio.h>
> > -#include <linux/irq.h>
>
> I think you should keep irq.h even if the gpio header currently provides
> it.
>
> > -#include <linux/irqdomain.h>
>
> Similarly for this one, if you still rely on it.
>
Ok i'll do that.
> > +#include <linux/gpio/driver.h>
> > #include <linux/mutex.h>
> >
> > #include "greybus.h"
> > @@ -40,8 +38,6 @@ struct gb_gpio_controller {
> > struct gpio_chip chip;
> > struct irq_chip irqc;
> > struct irq_chip *irqchip;
>
> This should not be needed any more either.
>
Just to confirm, by thism you mean only
struct irq_chip *irqchip; right?
> > - struct irq_domain *irqdomain;
> > - unsigned int irq_base;
> > irq_flow_handler_t irq_handler;
> > unsigned int irq_default_type;
>
> Neither should these two.
>
Ok.
> > struct mutex irq_lock;
> > @@ -365,6 +361,7 @@ static int gb_gpio_request_handler(struct gb_operation
> > *op)
> > {
> > struct gb_connection *connection = op->connection;
> > struct gb_gpio_controller *ggc = gb_connection_get_data(connection);
> > + struct gpio_chip *gc = &ggc->chip;
>
> Please name the pointer 'chip' as in the rest of the driver to avoid
> confusion with 'ggc'. But looks like you don't need it at all.
>
Yes.
> > struct device *dev = &ggc->gbphy_dev->dev;
> > struct gb_message *request;
> > struct gb_gpio_irq_event_request *event;
> > @@ -391,7 +388,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(gc->irq.domain, event->which);
> > if (!irq) {
> > dev_err(dev, "failed to find IRQ\n");
> > return -EINVAL;
> > @@ -506,68 +503,6 @@ static int gb_gpio_controller_setup(struct
> > gb_gpio_controller *ggc)
> > return ret;
> > }
>
> > -/**
> > - * 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
> > @@ -595,8 +530,7 @@ static int gb_gpio_irqchip_add(struct gpio_chip *chip,
> > unsigned int type)
> > {
> > struct gb_gpio_controller *ggc;
> > - unsigned int offset;
> > - unsigned int irq_base;
> > + unsigned int err;
> >
> > if (!chip || !irqchip)
> > return -EINVAL;
> > @@ -606,35 +540,21 @@ static int gb_gpio_irqchip_add(struct gpio_chip *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;
> > + err = gpiochip_irqchip_add(chip,
> > + irqchip,
> > + first_irq,
> > + ggc->irq_handler,
> > + type
>
> Don't break the line here.
>
Ok.
> > + );
> > + if (err) {
> > + ggc->irqchip = NULL;
> > + return err;
> > }
> >
> > return 0;
> > }
>
> Drop this function as well and call gpiochip_irqchip_add() from probe().
>
Ok. I'll do that.
> > -static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned int 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)
> > {
> > @@ -693,7 +613,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
> > gpio->get = gb_gpio_get;
> > gpio->set = gb_gpio_set;
> > gpio->set_config = gb_gpio_set_config;
> > - gpio->to_irq = gb_gpio_to_irq;
> > gpio->base = -1; /* Allocate base dynamically */
> > gpio->ngpio = ggc->line_max + 1;
> > gpio->can_sleep = true;
> > @@ -702,24 +621,23 @@ 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 = gpiochip_add(gpio);
> > if (ret) {
> > - dev_err(&gbphy_dev->dev, "failed to add irq chip: %d\n", ret);
> > + dev_err(&gbphy_dev->dev, "failed to add gpio chip: %d\n", ret);
> > goto exit_line_free;
> > }
> >
> > - ret = gpiochip_add(gpio);
> > + ret = gb_gpio_irqchip_add(gpio, irqc, 0,
> > + handle_level_irq, IRQ_TYPE_NONE);
>
> As you may have noted, we had the registration order reversed here to
> handle a (theoretical) race, which may or may not only have been an
> issue for the old 3.10 vendor kernel this was developed against. I've
> forgotten the details, but see:
>
> 88f6ba61f25b ("greybus: gpio: create irqdomain before registering gpio
> controller")
>
> It's possibly an issue also for mainline, but this is indeed the
> registration order all other drivers use (even if they tend not to be
> hotpluggable).
>
Yes, I noted that while taking reference of some of the existing
drivers. Thanks for the explanation though.
> > if (ret) {
> > - dev_err(&gbphy_dev->dev, "failed to add gpio chip: %d\n", ret);
> > - goto exit_gpio_irqchip_remove;
> > + dev_err(&gbphy_dev->dev, "failed to add irq chip: %d\n", ret);
> > + gpiochip_remove(gpio);
>
> Please use an error label for this.
>
Ok. I'll do that.
> > + 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:
>
> Thanks,
> Johan
Thanks a lot for the review comments.
Regards,
Nishad
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
