Re: [PATCH v7 2/5] net: stmmac: platform: read channels irq

Jan Petrous
2026-02-27T12:27:45Z

On Thu, Feb 26, 2026 at 07:10:22PM +0000, Russell King (Oracle) wrote:
> On Thu, Feb 26, 2026 at 09:54:07AM +0100, Jan Petrous via B4 Relay wrote:
> > From: "Jan Petrous (OSS)" <jan.petrous@oss.nxp.com>
> >
> > Read IRQ resources for all rx/tx channels, to allow Multi-IRQ mode
> > for platform glue drivers.
> >
> > Reviewed-by: Matthias Brugger <mbrugger@suse.com>
> > Signed-off-by: Jan Petrous (OSS) <jan.petrous@oss.nxp.com>
> > ---
> > .../net/ethernet/stmicro/stmmac/stmmac_platform.c | 46 +++++++++++++++++++++-
> > 1 file changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > index 5c9fd91a1db9..93bd915ab6eb 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> > @@ -697,9 +697,40 @@ struct clk *stmmac_pltfr_find_clk(struct plat_stmmacenet_data *plat_dat,
> > }
> > EXPORT_SYMBOL_GPL(stmmac_pltfr_find_clk);
> >
> > +static int stmmac_pltfr_get_queue_irqs(struct platform_device *pdev,
> > + struct stmmac_resources *stmmac_res,
> > + bool tx)
> > +{
> > + int *irqs = tx ? &stmmac_res->tx_irq[0] : &stmmac_res->rx_irq[0];
> > + char name[16];
> > + int i;
> > +
> > + /* RX channels irq */
> > + STMMAC_FOREACH_MTL_QUEUE(i, MTL_MAX_RX_QUEUES) {
>
> You've missed that there are two separate definitions for tx and rx
> queues - while they are currently the same number, code shouldn't
> make that assumption.

Oh, yes. My fault. Next time I shall review my code better.

>
> > + scnprintf(name, sizeof(name), "%cx-queue-%d",
> > + tx ? 't' : 'r', i);
>
> I'm not happy with this method of combining the two loops.
>
> Maybe instead:
>
> static int stmmac_pltfr_get_irq_array(struct platform_device *pdev,
> const char *fmt, int *irqs,
> size_t num)
> {
> char name[16];
> size_t i;
>
> for (i = 0; i < num; i++) {
> if (snprintf(name, sizeof(name), fmt, i) >= sizeof(name))
> return -EINVAL;
>
> irqs[i] = platform_get_irq_byname_optional(pdev, name);
> if (irqs[i] == -EPROBE_DEFER) {
> return irqs[i];
> } else if (irqs[i] <= 0) {
> dev_dbg(&pdev->dev, "IRQ %s not found\n", name);
>
> irqs[i] = 0;
> break;
> }
> }
>
> return 0;
> }
>
> which has the advantage that it becomes a generic helper for getting an
> any array of IRQs.
>
> > int stmmac_get_platform_resources(struct platform_device *pdev,
> > struct stmmac_resources *stmmac_res)
> > {
> > + int ret;
> > +
> > memset(stmmac_res, 0, sizeof(*stmmac_res));
> >
> > /* Get IRQ information early to have an ability to ask for deferred
> > @@ -735,7 +766,20 @@ int stmmac_get_platform_resources(struct platform_device *pdev,
> >
> > stmmac_res->addr = devm_platform_ioremap_resource(pdev, 0);
> >
> > - return PTR_ERR_OR_ZERO(stmmac_res->addr);
> > + if (IS_ERR(stmmac_res->addr))
> > + return PTR_ERR(stmmac_res->addr);
> > +
> > + /* TX channels irq */
> > + ret = stmmac_pltfr_get_queue_irqs(pdev, stmmac_res, true);
> > + if (ret)
> > + return ret;
> > +
> > + /* RX channels irq */
> > + ret = stmmac_pltfr_get_queue_irqs(pdev, stmmac_res, false);
> > + if (ret)
> > + return ret;
>
> These then become:
>
> ret = stmmac_pltfr_get_irq_array(pdev, "tx-queue-%d",
> stmmac_res->tx_irq,
> MTL_MAX_TX_QUEUES);
> if (ret)
> return ret;
>
> ret = stmmac_pltfr_get_irq_array(pdev, "rx-queue-%d",
> stmmac_res->rx_irq,
> MTL_MAX_RX_QUEUES);
> if (ret)
> return ret;
>
> This has the advantage that one can grep for rx-queue to find it,
> and we also use the correct limit for each queue type.
>

Agree, your code looks better. Applied in v8 :) Thanks.

BTW, I would prefer to use sizeof() instead of constant for array size,
but this is the style used in stmmac, so I reuse the same approach.

BR.
/Jan