Messages in this thread | | | Date | Fri, 21 Jul 2023 16:18:49 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v3 01/42] gpio: ep93xx: split device in multiple |
| |
On Thu, Jul 20, 2023 at 02:29:01PM +0300, Nikita Shubin via B4 Relay wrote: > From: Nikita Shubin <nikita.shubin@maquefel.me> > > This prepares ep93xx SOC gpio to convert into device tree driver: > - dropped banks and legacy defines > - split AB IRQ and make it shared > > We are relying on IRQ number information A, B ports have single shared > IRQ, while F port have dedicated IRQ for each line. > > Also we had to split single ep93xx platform_device into multiple, one > for each port, without this we can't do a full working transition from > legacy platform code into device tree capable. All GPIO_LOOKUP were > change to match new chip namings.
...
> -static void ep93xx_gpio_ab_irq_handler(struct irq_desc *desc) > +static u32 ep93xx_gpio_ab_irq_handler(struct gpio_chip *gc) > { > - struct gpio_chip *gc = irq_desc_get_handler_data(desc); > - struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - struct irq_chip *irqchip = irq_desc_get_chip(desc); > + struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc); > unsigned long stat; > int offset; > > - chained_irq_enter(irqchip, desc); > - > - /* > - * Dispatch the IRQs to the irqdomain of each A and B > - * gpiochip irqdomains depending on what has fired. > - * The tricky part is that the IRQ line is shared > - * between bank A and B and each has their own gpiochip. > - */ > - stat = readb(epg->base + EP93XX_GPIO_A_INT_STATUS); > + stat = readb(eic->base + EP93XX_INT_STATUS_OFFSET); > for_each_set_bit(offset, &stat, 8) > - generic_handle_domain_irq(epg->gc[0].gc.irq.domain, > - offset); > + generic_handle_domain_irq(gc->irq.domain, offset); > > - stat = readb(epg->base + EP93XX_GPIO_B_INT_STATUS); > - for_each_set_bit(offset, &stat, 8) > - generic_handle_domain_irq(epg->gc[1].gc.irq.domain, > - offset); > + return stat; > +} > > - chained_irq_exit(irqchip, desc); > +static irqreturn_t ep93xx_ab_irq_handler(int irq, void *dev_id) > +{ > + return IRQ_RETVAL(ep93xx_gpio_ab_irq_handler(dev_id)); > } > > static void ep93xx_gpio_f_irq_handler(struct irq_desc *desc) > { > - /* > - * map discontiguous hw irq range to continuous sw irq range: > - * > - * IRQ_EP93XX_GPIO{0..7}MUX -> EP93XX_GPIO_LINE_F{0..7} > - */ > struct irq_chip *irqchip = irq_desc_get_chip(desc); > - unsigned int irq = irq_desc_get_irq(desc); > - int port_f_idx = (irq & 7) ^ 4; /* {20..23,48..51} -> {0..7} */ > - int gpio_irq = EP93XX_GPIO_F_IRQ_BASE + port_f_idx; > + struct gpio_chip *gc = irq_desc_get_handler_data(desc); > + struct gpio_irq_chip *gic = &gc->irq; > + unsigned int parent = irq_desc_get_irq(desc); > + unsigned int i; > > chained_irq_enter(irqchip, desc); > - generic_handle_irq(gpio_irq); > + for (i = 0; i < gic->num_parents; i++) > + if (gic->parents[i] == parent) > + break; > + > + if (i < gic->num_parents) > + generic_handle_irq(irq_find_mapping(gc->irq.domain, i));
Can we use
generic_handle_domain_irq(gc->irq.domain, i);
here as well?
> chained_irq_exit(irqchip, desc); > }
...
> - int offset = d->irq & 7; > + int offset = irqd_to_hwirq(d);
irq_hw_number_t ?
> irq_flow_handler_t handler;
...
> + int ret, irq, i = 0;
What do you need this assignment for?
...
> + ret = devm_request_irq(dev, irq, > + ep93xx_ab_irq_handler,
It can be located on the previous line.
> + IRQF_SHARED, gc->label, gc); > + if (ret) > + return dev_err_probe(dev, ret, "error requesting IRQ : %d\n", irq);
Drop duplicating word 'error' in the message. Space is not needed before colon.
...
> + /* TODO: replace with handle_bad_irq once we are fully hierarchical */
To be pedantic: handle_bad_irq()
> + gc->label = dev_name(&pdev->dev); > + if (platform_irq_count(pdev) > 0) { > + dev_dbg(&pdev->dev, "setting up irqs for %s\n", dev_name(&pdev->dev)); > + ret = ep93xx_setup_irqs(pdev, egc); > + if (ret)
> + dev_err(&pdev->dev, "setup irqs failed for %s\n", dev_name(&pdev->dev));
What's the point to print dev name twice? Esp. taking into account gc->label assignment above. Why not use dev_err_probe() to unify the format of the messages from ->probe()?
-- With Best Regards, Andy Shevchenko
| |