Messages in this thread | | | Date | Mon, 18 Sep 2023 10:37:55 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH v4 01/42] gpio: ep93xx: split device in multiple |
| |
On Fri, Sep 15, 2023 at 11:10:43AM +0300, Nikita Shubin via B4 Relay wrote: > From: Nikita Shubin <nikita.shubin@maquefel.me> > > Prepare 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.
...
> #include <linux/reboot.h> > #include <linux/usb/ohci_pdriver.h> > #include <linux/random.h> > +#include <linux/ioport.h>
Can it be squeezed to the most ordered part?
...
> +static struct resource ep93xx_a_gpio_resources[] = { > + DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE, 0x04, "data"),
You can also use " + 0x00", but it's up to you.
> + DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE + 0x10, 0x04, "dir"), > + DEFINE_RES_MEM_NAMED(EP93XX_GPIO_PHYS_BASE + 0x90, 0x1c, "intr"), > DEFINE_RES_IRQ(IRQ_EP93XX_GPIO_AB), > +};
...
> .dev_id = "i2c-gpio.0", > .table = { > /* Use local offsets on gpiochip/port "G" */ > - GPIO_LOOKUP_IDX("G", 1, NULL, 0, > + GPIO_LOOKUP_IDX("gpio-ep93xx.6", 1, NULL, 0, > GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN), > - GPIO_LOOKUP_IDX("G", 0, NULL, 1, > + GPIO_LOOKUP_IDX("gpio-ep93xx.6", 0, NULL, 1, > GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN),
Here the terminator is missing, I believe you need to have a separate fix patch done first to prepend this one.
> },
...
> #include <linux/seq_file.h> > +#include <linux/interrupt.h>
Please, squeeze this to the most sorted part.
...
> 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_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc); > - struct ep93xx_gpio *epg = gpiochip_get_data(gc); > - int port_mask = BIT(d->irq & 7);
irq_hw_number_t hwirq = irqd_to_hwirq(d);
> + int port_mask = BIT(hwirq); > > if (irqd_get_trigger_type(d) == IRQ_TYPE_EDGE_BOTH) > eic->int_type2 ^= port_mask; /* switch edge direction */ > > eic->int_unmasked &= ~port_mask; > - ep93xx_gpio_update_int_params(epg, eic); > + ep93xx_gpio_update_int_params(eic); > > - writeb(port_mask, epg->base + eic->irq_offset + EP93XX_INT_EOI_OFFSET); > + writeb(port_mask, eic->base + EP93XX_INT_EOI_OFFSET);
> gpiochip_disable_irq(gc, irqd_to_hwirq(d));
gpiochip_disable_irq(gc, hwirq);
> }
...
> { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc); > - struct ep93xx_gpio *epg = gpiochip_get_data(gc); > > - eic->int_unmasked &= ~BIT(d->irq & 7); > - ep93xx_gpio_update_int_params(epg, eic); > + eic->int_unmasked &= ~BIT(irqd_to_hwirq(d)); > + ep93xx_gpio_update_int_params(eic); > gpiochip_disable_irq(gc, irqd_to_hwirq(d)); > }
As per above.
...
> @@ -203,11 +164,10 @@ static void ep93xx_gpio_irq_unmask(struct irq_data *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > struct ep93xx_gpio_irq_chip *eic = to_ep93xx_gpio_irq_chip(gc); > - struct ep93xx_gpio *epg = gpiochip_get_data(gc); > > gpiochip_enable_irq(gc, irqd_to_hwirq(d)); > - eic->int_unmasked |= BIT(d->irq & 7); > - ep93xx_gpio_update_int_params(epg, eic); > + eic->int_unmasked |= BIT(irqd_to_hwirq(d)); > + ep93xx_gpio_update_int_params(eic); > }
As per above.
...
> + irq_hw_number_t offset = irqd_to_hwirq(d);
(or use hwirq name to be consistent in the code).
...
> + girq->parents = devm_kcalloc(dev, girq->num_parents, > + sizeof(*girq->parents),
I would put this one to the previous line.
> + GFP_KERNEL); > + if (!girq->parents) > + return -ENOMEM;
...
> + irq = platform_get_irq(pdev, i);
This will issue a warning or error message if IRQ is not found...
> + if (irq < 0)
> + continue;
...but this suggests that that is optional. Hence
irq = platform_get_irq_optional(pdev, i);
?
...
> + 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_probe(&pdev->dev, ret, "setup irqs failed");
Hmm... Missing return?
> }
-- With Best Regards, Andy Shevchenko
| |