| Message ID | 20210127104617.1173-1-nikita.shubin@maquefel.me |
|---|---|
| Headers | show |
| Series | gpio: ep93xx: fixes series patch | expand |
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; > }
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];
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
Hi! 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") > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> Yes, it indeed fixes the warnigs mentioned above, Tested-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > 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.