| Message ID | 20201228150052.2633-1-nikita.shubin@maquefel.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | gpiolib: warning on gpiochip->to_irq defined | expand |
On Mon, Dec 28, 2020 at 4:02 PM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > > gpiochip->to_irq method is redefined in gpiochip_add_irqchip. > > A lot of gpiod driver's still define ->to_irq method, let's give > a gentle warning that they can no longer rely on it, so they can remove > it on ocassion. > > Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration") > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/gpio/gpiolib.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 5ce0c14c637b..44538d1a771a 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, > type = IRQ_TYPE_NONE; > } > > + if (gc->to_irq) > + chip_err(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__); > + > gc->to_irq = gpiochip_to_irq; > gc->irq.default_type = type; > gc->irq.lock_key = lock_key; > -- > 2.29.2 > I know Linus suggested using chip_err() here but since this doesn't cause the function to fail, I'd say this should be chip_warn(). Unless Linus has any objections, please change it. Bartosz
Series of patches to fix ep93xx gpio driver to make IRQ's working. The following are fix patches (these are enough to get gpio-ep93xx working with modern kernel): [PATCH v2 1/9] gpio: ep93xx: fix BUG_ON port F usage [PATCH v2 2/9] gpio: ep93xx: Fix single irqchip with multi gpiochips [PATCH v2 3/9] gpio: ep93xx: Fix wrong irq numbers in port F The following are cleanup and style patches: [PATCH v2 4/9] gpio: ep93xx: drop to_irq binding [PATCH v2 5/9] gpio: ep93xx: Fix typo s/hierarchial/hierarchical [PATCH v2 6/9] gpio: ep93xx: refactor ep93xx_gpio_add_bank [PATCH v2 7/9] gpio: ep93xx: separate IRQ's setup [PATCH v2 8/9] gpio: ep93xx: refactor base IRQ number [PATCH v2 9/9] gpio: ep93xx: replace bools with idx for IRQ ports The futher work can be done by removing ep93xx_gpio_port function and, maybe, ep93xx_gpio_update_int_params. But i think it's better to be done in conjunction with converting ep93xx to DT support.
Hello Nikita! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Replace boolean values used for determining if gpiochip is IRQ > capable > or not with index. What's the purpose of this patch? It neither makes the code more effective nor does it make the code more readable IMO. There are also some changes in the code which were not mentioned in the commit message. > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/gpio/gpio-ep93xx.c | 47 ++++++++++++++++++++---------------- > -- > 1 file changed, 25 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index 2aee13b5067d..f75a33b79504 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -44,6 +44,8 @@ struct ep93xx_gpio { > struct irq_chip ic[EP93XX_GPIO_IRQ_CHIPS_NUM]; > }; > > +#define EP93XX_GPIO_A_PORT_INDEX 0 > +#define EP93XX_GPIO_B_PORT_INDEX 1 > /* > * F Port index in GPIOCHIP'S array is 5 > * but we use index 2 for stored values and offsets > @@ -291,38 +293,36 @@ static struct irq_chip ep93xx_gpio_irq_chip = { > * gpiolib interface for EP93xx on-chip GPIOs > > ********************************************************************* > ****/ > struct ep93xx_gpio_bank { > + u8 idx; > const char *label; > int data; > int dir; > int base; > - bool has_irq; > - bool has_hierarchical_irq; > unsigned int irq_base; > }; > > -#define EP93XX_GPIO_BANK(_label, _data, _dir, _base, _has_irq, > _has_hier, _irq_base) \ > +#define EP93XX_GPIO_BANK(_index, _label, _data, _dir, _base, > _irq_base) \ > { \ > + .idx = _index, \ > .label = _label, \ > .data = _data, \ > .dir = _dir, \ > .base = _base, \ > - .has_irq = _has_irq, \ > - .has_hierarchical_irq = _has_hier, \ > .irq_base = _irq_base, \ > } > > static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = { > /* Bank A has 8 IRQs */ > - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, > EP93XX_GPIO_A_IRQ_BASE), > + EP93XX_GPIO_BANK(0, "A", 0x00, 0x10, 0, > EP93XX_GPIO_A_IRQ_BASE), > /* Bank B has 8 IRQs */ > - EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, > EP93XX_GPIO_B_IRQ_BASE), > - EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0), > - EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0), > - EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0), > + EP93XX_GPIO_BANK(1, "B", 0x04, 0x14, 8, > EP93XX_GPIO_B_IRQ_BASE), > + EP93XX_GPIO_BANK(2, "C", 0x08, 0x18, 40, 0), > + EP93XX_GPIO_BANK(3, "D", 0x0c, 0x1c, 24, 0), > + EP93XX_GPIO_BANK(4, "E", 0x20, 0x24, 32, 0), > /* Bank F has 8 IRQs */ > - EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, > EP93XX_GPIO_F_IRQ_BASE), > - EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0), > - EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0), > + EP93XX_GPIO_BANK(5, "F", 0x30, 0x34, 16, > EP93XX_GPIO_F_IRQ_BASE), > + EP93XX_GPIO_BANK(6, "G", 0x38, 0x3c, 48, 0), > + EP93XX_GPIO_BANK(7, "H", 0x40, 0x44, 56, 0), > }; > > static int ep93xx_gpio_set_config(struct gpio_chip *gc, unsigned > offset, > @@ -356,10 +356,11 @@ static void ep93xx_init_irq_chips(struct > ep93xx_gpio *epg) > } > > static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev, > - struct gpio_irq_chip *girq, > + struct gpio_chip *gc, > unsigned int irq_base) > { > struct device *dev = &pdev->dev; > + struct gpio_irq_chip *girq = &gc->irq; > int ab_parent_irq = platform_get_irq(pdev, 0); > > girq->parent_handler = ep93xx_gpio_ab_irq_handler; > @@ -378,12 +379,13 @@ static int ep93xx_gpio_add_ab_irq_chip(struct > platform_device *pdev, > } > > static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev, > - struct gpio_irq_chip *girq, > + struct gpio_chip *gc, > unsigned int irq_base) > { > int gpio_irq; > int i; > struct device *dev = &pdev->dev; > + struct gpio_irq_chip *girq = &gc->irq; > > /* > * FIXME: convert this to use hierarchical IRQ support! > @@ -397,10 +399,10 @@ static int ep93xx_gpio_add_f_irq_chip(struct > platform_device *pdev, > if (!girq->parents) > return -ENOMEM; > /* Pick resources 1..8 for these IRQs */ > - for (i = 0; i < ARRAY_SIZE(girq->parents); i++) { > + for (i = 0; i < girq->num_parents; i++) { > girq->parents[i] = platform_get_irq(pdev, i + 1); > gpio_irq = irq_base + i; > - irq_set_chip_data(gpio_irq, &epg->gc[5]); > + irq_set_chip_data(gpio_irq, gc); > irq_set_chip_and_handler(gpio_irq, > &ep93xx_gpio_irq_chip, > handle_level_irq); > @@ -433,21 +435,22 @@ static int ep93xx_gpio_add_bank(struct > gpio_chip *gc, > gc->base = bank->base; > > girq = &gc->irq; > - if (bank->has_irq || bank->has_hierarchical_irq) { > + if (bank->irq_base != 0) { > gc->set_config = ep93xx_gpio_set_config; > port = ep93xx_gpio_port(epg, gc); > girq->chip = &epg->ic[port]; > } > > - if (bank->has_irq) { > - err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank- > >irq_base); > + if (bank->idx == EP93XX_GPIO_A_PORT_INDEX || > + bank->idx == EP93XX_GPIO_B_PORT_INDEX) { > + err = ep93xx_gpio_add_ab_irq_chip(pdev, gc, bank- > >irq_base); > if (err) > return err; > } > > /* Only bank F has especially funky IRQ handling */ > - if (bank->has_hierarchical_irq) { > - err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank- > >irq_base); > + if (bank->idx == EP93XX_GPIO_F_PORT_INDEX) { > + err = ep93xx_gpio_add_f_irq_chip(pdev, gc, bank- > >irq_base); > if (err) > return err; > } -- Alexander Sverdlin.
Hi! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > - use predefined constants instead of plain numbers > - use provided bank IRQ number instead of defined constant > for port F > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/gpio/gpio-ep93xx.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index b212c007240e..2aee13b5067d 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -28,6 +28,8 @@ > /* Maximum value for irq capable line identifiers */ > #define EP93XX_GPIO_LINE_MAX_IRQ 23 > > +#define EP93XX_GPIO_A_IRQ_BASE 64 > +#define EP93XX_GPIO_B_IRQ_BASE 72 > /* > * Static mapping of GPIO bank F IRQS: > * F0..F7 (16..24) to irq 80..87. > @@ -311,14 +313,14 @@ struct ep93xx_gpio_bank { > > static struct ep93xx_gpio_bank ep93xx_gpio_banks[] = { > /* Bank A has 8 IRQs */ > - EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, 64), > + EP93XX_GPIO_BANK("A", 0x00, 0x10, 0, true, false, > EP93XX_GPIO_A_IRQ_BASE), > /* Bank B has 8 IRQs */ > - EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, 72), > + EP93XX_GPIO_BANK("B", 0x04, 0x14, 8, true, false, > EP93XX_GPIO_B_IRQ_BASE), > EP93XX_GPIO_BANK("C", 0x08, 0x18, 40, false, false, 0), > EP93XX_GPIO_BANK("D", 0x0c, 0x1c, 24, false, false, 0), > EP93XX_GPIO_BANK("E", 0x20, 0x24, 32, false, false, 0), > /* Bank F has 8 IRQs */ > - EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, 0), > + EP93XX_GPIO_BANK("F", 0x30, 0x34, 16, false, true, > EP93XX_GPIO_F_IRQ_BASE), > EP93XX_GPIO_BANK("G", 0x38, 0x3c, 48, false, false, 0), > EP93XX_GPIO_BANK("H", 0x40, 0x44, 56, false, false, 0), > }; > @@ -445,7 +447,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > > /* Only bank F has especially funky IRQ handling */ > if (bank->has_hierarchical_irq) { > - err = ep93xx_gpio_add_f_irq_chip(pdev, girq, > EP93XX_GPIO_F_IRQ_BASE); > + err = ep93xx_gpio_add_f_irq_chip(pdev, girq, bank- > >irq_base); > if (err) > return err; > } -- Alexander Sverdlin.
Hi! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > As ->to_irq is redefined in gpiochip_add_irqchip, having it defined > in > driver is useless, so let's drop it. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/gpio/gpio-ep93xx.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index 9c3d049e5af7..dee19372ebbd 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -337,11 +337,6 @@ static int ep93xx_gpio_set_config(struct > gpio_chip *gc, unsigned offset, > return 0; > } > > -static int ep93xx_gpio_f_to_irq(struct gpio_chip *gc, unsigned > offset) > -{ > - return EP93XX_GPIO_F_IRQ_BASE + offset; > -} > - > static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg) > { > int i; > @@ -429,7 +424,6 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > } > girq->default_type = IRQ_TYPE_NONE; > girq->handler = handle_level_irq; > - gc->to_irq = ep93xx_gpio_f_to_irq; > girq->first = EP93XX_GPIO_F_IRQ_BASE; > } > -- Alexander Sverdlin.
Hi! On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Fix typo in comment. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/gpio/gpio-ep93xx.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index dee19372ebbd..8f66e3ca0cfb 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -402,7 +402,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > > /* > * FIXME: convert this to use hierarchical IRQ > support! > - * this requires fixing the root irqchip to be > hierarchial. > + * this requires fixing the root irqchip to be > hierarchical. > */ > girq->parent_handler = ep93xx_gpio_f_irq_handler; > girq->num_parents = 8; -- Alexander Sverdlin.
Hello Nikita, On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > - replace plain numbers with girq->num_parents in devm_kcalloc > - replace plain numbers with ARRAY_SIZE(girq->parents) for port F > - refactor i - 1 to i + 1 to make loop more readable > - combine getting IRQ's loop and setting handler's into single loop > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/gpio/gpio-ep93xx.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index 8f66e3ca0cfb..e4270b4e5f26 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -384,7 +384,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > > girq->parent_handler = ep93xx_gpio_ab_irq_handler; > girq->num_parents = 1; > - girq->parents = devm_kcalloc(dev, 1, > + girq->parents = devm_kcalloc(dev, girq->num_parents, > sizeof(*girq->parents), > GFP_KERNEL); > if (!girq->parents) > @@ -406,15 +406,14 @@ static int ep93xx_gpio_add_bank(struct > gpio_chip *gc, > */ > girq->parent_handler = ep93xx_gpio_f_irq_handler; > girq->num_parents = 8; > - girq->parents = devm_kcalloc(dev, 8, > + girq->parents = devm_kcalloc(dev, girq->num_parents, > sizeof(*girq->parents), > GFP_KERNEL); > if (!girq->parents) > return -ENOMEM; > /* Pick resources 1..8 for these IRQs */ > - for (i = 1; i <= 8; i++) > - girq->parents[i - 1] = platform_get_irq(pdev, > i); > - for (i = 0; i < 8; i++) { > + for (i = 0; i < ARRAY_SIZE(girq->parents); i++) { Why do you use ARRAY_SIZE() here instead of ->num_parents like above? > + girq->parents[i] = platform_get_irq(pdev, i + > 1); > gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i; > irq_set_chip_data(gpio_irq, &epg->gc[5]); > irq_set_chip_and_handler(gpio_irq, -- Alexander Sverdlin.
Hello Nikita, On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Separate IRQ's setup for port A,B,F. Somehow I don't feel that moving "FIXME" code around makes much sense. Maybe the anticipated conversion to hierarhical IRQ chip will result in a cleanup automatically? > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/gpio/gpio-ep93xx.c | 104 +++++++++++++++++++++++------------ > -- > 1 file changed, 64 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index e4270b4e5f26..b212c007240e 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -353,6 +353,64 @@ static void ep93xx_init_irq_chips(struct > ep93xx_gpio *epg) > } > } > > +static int ep93xx_gpio_add_ab_irq_chip(struct platform_device *pdev, > + struct gpio_irq_chip *girq, > + unsigned int irq_base) > +{ > + struct device *dev = &pdev->dev; > + int ab_parent_irq = platform_get_irq(pdev, 0); > + > + girq->parent_handler = ep93xx_gpio_ab_irq_handler; > + girq->num_parents = 1; > + girq->parents = devm_kcalloc(dev, girq->num_parents, > + sizeof(*girq->parents), > + GFP_KERNEL); > + if (!girq->parents) > + return -ENOMEM; > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_level_irq; > + girq->parents[0] = ab_parent_irq; > + girq->first = irq_base; > + > + return 0; > +} > + > +static int ep93xx_gpio_add_f_irq_chip(struct platform_device *pdev, > + struct gpio_irq_chip *girq, > + unsigned int irq_base) > +{ > + int gpio_irq; > + int i; > + struct device *dev = &pdev->dev; > + > + /* > + * FIXME: convert this to use hierarchical IRQ support! > + * this requires fixing the root irqchip to be hierarchical. > + */ > + girq->parent_handler = ep93xx_gpio_f_irq_handler; > + girq->num_parents = 8; > + girq->parents = devm_kcalloc(dev, girq->num_parents, > + sizeof(*girq->parents), > + GFP_KERNEL); > + if (!girq->parents) > + return -ENOMEM; > + /* Pick resources 1..8 for these IRQs */ > + for (i = 0; i < ARRAY_SIZE(girq->parents); i++) { > + girq->parents[i] = platform_get_irq(pdev, i + 1); > + gpio_irq = irq_base + i; > + irq_set_chip_data(gpio_irq, &epg->gc[5]); > + irq_set_chip_and_handler(gpio_irq, > + &ep93xx_gpio_irq_chip, > + handle_level_irq); > + irq_clear_status_flags(gpio_irq, IRQ_NOREQUEST); > + } > + girq->default_type = IRQ_TYPE_NONE; > + girq->handler = handle_level_irq; > + girq->first = irq_base; > + > + return 0; > +} > + > static int ep93xx_gpio_add_bank(struct gpio_chip *gc, > struct platform_device *pdev, > struct ep93xx_gpio *epg, > @@ -380,50 +438,16 @@ static int ep93xx_gpio_add_bank(struct > gpio_chip *gc, > } > > if (bank->has_irq) { > - int ab_parent_irq = platform_get_irq(pdev, 0); > - > - girq->parent_handler = ep93xx_gpio_ab_irq_handler; > - girq->num_parents = 1; > - girq->parents = devm_kcalloc(dev, girq->num_parents, > - sizeof(*girq->parents), > - GFP_KERNEL); > - if (!girq->parents) > - return -ENOMEM; > - girq->default_type = IRQ_TYPE_NONE; > - girq->handler = handle_level_irq; > - girq->parents[0] = ab_parent_irq; > - girq->first = bank->irq_base; > + err = ep93xx_gpio_add_ab_irq_chip(pdev, girq, bank- > >irq_base); > + if (err) > + return err; > } > > /* Only bank F has especially funky IRQ handling */ > if (bank->has_hierarchical_irq) { > - int gpio_irq; > - int i; > - > - /* > - * FIXME: convert this to use hierarchical IRQ > support! > - * this requires fixing the root irqchip to be > hierarchical. > - */ > - girq->parent_handler = ep93xx_gpio_f_irq_handler; > - girq->num_parents = 8; > - girq->parents = devm_kcalloc(dev, girq->num_parents, > - sizeof(*girq->parents), > - GFP_KERNEL); > - if (!girq->parents) > - return -ENOMEM; > - /* Pick resources 1..8 for these IRQs */ > - for (i = 0; i < ARRAY_SIZE(girq->parents); i++) { > - girq->parents[i] = platform_get_irq(pdev, i + > 1); > - gpio_irq = EP93XX_GPIO_F_IRQ_BASE + i; > - irq_set_chip_data(gpio_irq, &epg->gc[5]); > - irq_set_chip_and_handler(gpio_irq, > - > &ep93xx_gpio_irq_chip, > - handle_level_irq); > - irq_clear_status_flags(gpio_irq, > IRQ_NOREQUEST); > - } > - girq->default_type = IRQ_TYPE_NONE; > - girq->handler = handle_level_irq; > - girq->first = EP93XX_GPIO_F_IRQ_BASE; > + err = ep93xx_gpio_add_f_irq_chip(pdev, girq, > EP93XX_GPIO_F_IRQ_BASE); > + if (err) > + return err; > } > > return devm_gpiochip_add_data(dev, gc, epg); -- Alexander Sverdlin.
Hello Nikita, On Wed, 2021-01-27 at 13:46 +0300, Nikita Shubin wrote: > Fixes the following warnings which results in interrupts disabled on > port B/F: > > gpio gpiochip1: (B): detected irqchip that is shared with multiple > gpiochips: please fix the driver. > gpio gpiochip5: (F): detected irqchip that is shared with multiple > gpiochips: please fix the driver. > > - added separate irqchip for each interrupt capable gpiochip > - provided unique names for each irqchip > - reworked ep93xx_gpio_port to make it usable before chip_add_data > in ep93xx_init_irq_chips > > Fixes: a8173820f441 ("gpio: gpiolib: Allow GPIO IRQs to lazy > disable") I'd rather say, it fixes commit d2b091961510 ("gpio: ep93xx: Pass irqchip when adding gpiochip"). But for sure, not the gpiolib code as above tag claims. > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/gpio/gpio-ep93xx.c | 45 ++++++++++++++++++++++++++++++------ > -- > 1 file changed, 36 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpio-ep93xx.c b/drivers/gpio/gpio-ep93xx.c > index 0d0435c07a5a..2eea02c906e0 100644 > --- a/drivers/gpio/gpio-ep93xx.c > +++ b/drivers/gpio/gpio-ep93xx.c > @@ -34,9 +34,12 @@ > */ > #define EP93XX_GPIO_F_IRQ_BASE 80 > > +#define EP93XX_GPIO_IRQ_CHIPS_NUM 3 > + > struct ep93xx_gpio { > void __iomem *base; > struct gpio_chip gc[8]; > + struct irq_chip ic[EP93XX_GPIO_IRQ_CHIPS_NUM]; > }; > > /* > @@ -55,6 +58,11 @@ static unsigned char gpio_int_type2[3]; > static unsigned char gpio_int_debounce[3]; > > /* Port ordering is: A B F */ > +static const char * const irq_chip_names[] = { > + "gpio-irq-a", > + "gpio-irq-b", > + "gpio-irq-f" > +}; > static const u8 int_type1_register_offset[3] = { 0x90, 0xac, 0x4c > }; > static const u8 int_type2_register_offset[3] = { 0x94, 0xb0, 0x50 > }; > static const u8 eoi_register_offset[3] = { 0x98, 0xb4, 0x54 > }; > @@ -77,9 +85,8 @@ static void ep93xx_gpio_update_int_params(struct > ep93xx_gpio *epg, unsigned port > epg->base + int_en_register_offset[port]); > } > > -static int ep93xx_gpio_port(struct gpio_chip *gc) > +static int ep93xx_gpio_port(struct ep93xx_gpio *epg, struct > gpio_chip *gc) > { > - struct ep93xx_gpio *epg = gpiochip_get_data(gc); > int port = 0; > > while (port < ARRAY_SIZE(epg->gc) && gc != &epg->gc[port]) > @@ -101,7 +108,7 @@ static void ep93xx_gpio_int_debounce(struct > gpio_chip *gc, > unsigned int offset, bool > enable) > { > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > int port_mask = BIT(offset); > > if (enable) > @@ -163,7 +170,7 @@ static void ep93xx_gpio_irq_ack(struct irq_data > *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > int port_mask = BIT(d->irq & 7); > > if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) { > @@ -178,7 +185,7 @@ static void ep93xx_gpio_irq_mask_ack(struct > irq_data *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > int port_mask = BIT(d->irq & 7); > > if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) > @@ -194,7 +201,7 @@ static void ep93xx_gpio_irq_mask(struct irq_data > *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > > gpio_int_unmasked[port] &= ~BIT(d->irq & 7); > ep93xx_gpio_update_int_params(epg, port); > @@ -204,7 +211,7 @@ static void ep93xx_gpio_irq_unmask(struct > irq_data *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > > gpio_int_unmasked[port] |= BIT(d->irq & 7); > ep93xx_gpio_update_int_params(epg, port); > @@ -219,7 +226,7 @@ static int ep93xx_gpio_irq_type(struct irq_data > *d, unsigned int type) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port = ep93xx_gpio_port(gc); > + int port = ep93xx_gpio_port(epg, gc); > int offset = d->irq & 7; > int port_mask = BIT(offset); > irq_flow_handler_t handler; > @@ -335,6 +342,22 @@ static int ep93xx_gpio_f_to_irq(struct gpio_chip > *gc, unsigned offset) > return EP93XX_GPIO_F_IRQ_BASE + offset; > } > > +static void ep93xx_init_irq_chips(struct ep93xx_gpio *epg) > +{ > + int i; > + > + for (i = 0; i < EP93XX_GPIO_IRQ_CHIPS_NUM; i++) { > + struct irq_chip *ic = &epg->ic[i]; > + > + ic->name = irq_chip_names[i]; > + ic->irq_ack = ep93xx_gpio_irq_ack; > + ic->irq_mask_ack = ep93xx_gpio_irq_mask_ack; > + ic->irq_mask = ep93xx_gpio_irq_mask; > + ic->irq_unmask = ep93xx_gpio_irq_unmask; > + ic->irq_set_type = ep93xx_gpio_irq_type; > + } > +} > + > static int ep93xx_gpio_add_bank(struct gpio_chip *gc, > struct platform_device *pdev, > struct ep93xx_gpio *epg, > @@ -345,6 +368,7 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > struct device *dev = &pdev->dev; > struct gpio_irq_chip *girq; > int err; > + int port; > > err = bgpio_init(gc, dev, 1, data, NULL, NULL, dir, NULL, 0); > if (err) > @@ -356,7 +380,8 @@ static int ep93xx_gpio_add_bank(struct gpio_chip > *gc, > girq = &gc->irq; > if (bank->has_irq || bank->has_hierarchical_irq) { > gc->set_config = ep93xx_gpio_set_config; > - girq->chip = &ep93xx_gpio_irq_chip; > + port = ep93xx_gpio_port(epg, gc); > + girq->chip = &epg->ic[port]; > } > > if (bank->has_irq) { > @@ -423,6 +448,8 @@ static int ep93xx_gpio_probe(struct > platform_device *pdev) > if (IS_ERR(epg->base)) > return PTR_ERR(epg->base); > > + ep93xx_init_irq_chips(epg); > + > for (i = 0; i < ARRAY_SIZE(ep93xx_gpio_banks); i++) { > struct gpio_chip *gc = &epg->gc[i]; > struct ep93xx_gpio_bank *bank = > &ep93xx_gpio_banks[i]; -- Alexander Sverdlin.
On Wed, Jan 27, 2021 at 11:46 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > Port F irq's should be statically mapped to EP93XX_GPIO_F_IRQ_BASE. > > So we need to specify girq->first otherwise: > > "If device tree is used, then first_irq will be 0 and > irqs get mapped dynamically on the fly" > > And that's not the thing we want. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> True, to satisfy old board file-type machines we unfortunately have to do this. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Jan 27, 2021 at 11:46 AM Nikita Shubin <nikita.shubin@maquefel.me> wrote: > Series of patches to fix ep93xx gpio driver to make IRQ's working. > > The following are fix patches (these are enough to get gpio-ep93xx > working with modern kernel): I see that there is a strange level of attention to patches to this platform! Since you fix all my mistakes made in converting this driver in the past I will just say: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> For all of them. There are some nitpicks from the reviewers to fix up but overall this looks very very good. Yours, Linus Walleij
Hello Linus, On Wed, 2021-01-27 at 22:54 +0100, Linus Walleij wrote: > > Series of patches to fix ep93xx gpio driver to make IRQ's working. > > > > The following are fix patches (these are enough to get gpio-ep93xx > > working with modern kernel): > > I see that there is a strange level of attention to patches to this > platform! > > Since you fix all my mistakes made in converting this driver > in the past I will just say: > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > For all of them. > > There are some nitpicks from the reviewers to fix up but > overall this looks very very good. as we don't have a dedicated EP93xx tree, would you like to take the series in one of your trees? -- Alexander Sverdlin.
On Thu, Jan 28, 2021 at 11:19 AM Alexander Sverdlin <alexander.sverdlin@gmail.com> wrote: > as we don't have a dedicated EP93xx tree, would you like to take > the series in one of your trees? Bartosz is managing the GPIO tree right now and I think he will queue it once all reviews are finished. Yours, Linus Walleij
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 5ce0c14c637b..44538d1a771a 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1489,6 +1489,9 @@ static int gpiochip_add_irqchip(struct gpio_chip *gc, type = IRQ_TYPE_NONE; } + if (gc->to_irq) + chip_err(gc, "to_irq is redefined in %s and you shouldn't rely on it\n", __func__); + gc->to_irq = gpiochip_to_irq; gc->irq.default_type = type; gc->irq.lock_key = lock_key;
gpiochip->to_irq method is redefined in gpiochip_add_irqchip. A lot of gpiod driver's still define ->to_irq method, let's give a gentle warning that they can no longer rely on it, so they can remove it on ocassion. Fixes: e0d8972898139 ("gpio: Implement tighter IRQ chip integration") Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> --- drivers/gpio/gpiolib.c | 3 +++ 1 file changed, 3 insertions(+)