| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZLqFuWsxhdiP4ZjA@smile.fi.intel.com>
Date: Fri, 21 Jul 2023 16:18:49 +0300
From: Andy Shevchenko <andy@...nel.org>
To: nikita.shubin@...uefel.me
Cc: Hartley Sweeten <hsweeten@...ionengravers.com>,
Lennert Buytenhek <kernel@...tstofly.org>,
Alexander Sverdlin <alexander.sverdlin@...il.com>,
Russell King <linux@...linux.org.uk>,
Lukasz Majewski <lukma@...x.de>,
Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Alessandro Zummo <a.zummo@...ertech.it>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Guenter Roeck <linux@...ck-us.net>,
Sebastian Reichel <sre@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Mark Brown <broonie@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Vinod Koul <vkoul@...nel.org>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
Damien Le Moal <dlemoal@...nel.org>,
Sergey Shtylyov <s.shtylyov@....ru>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Arnd Bergmann <arnd@...db.de>, Olof Johansson <olof@...om.net>,
soc@...nel.org, Liam Girdwood <lgirdwood@...il.com>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
Michael Peters <mpeters@...eddedts.com>,
Kris Bahnsen <kris@...eddedts.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
linux-clk@...r.kernel.org, linux-rtc@...r.kernel.org,
linux-watchdog@...r.kernel.org, linux-pm@...r.kernel.org,
linux-pwm@...r.kernel.org, linux-spi@...r.kernel.org,
netdev@...r.kernel.org, dmaengine@...r.kernel.org,
linux-mtd@...ts.infradead.org, linux-ide@...r.kernel.org,
linux-input@...r.kernel.org, alsa-devel@...a-project.org
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@...uefel.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
Powered by blists - more mailing lists