[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <301c5f90-0307-4c23-b867-6677d41dce47@lunn.ch>
Date: Sat, 10 Aug 2024 19:44:02 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
devicetree@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>,
Vladimir Oltean <olteanv@...il.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Marek Vasut <marex@...x.de>, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts
in KSZ9477/KSZ9893 switch families
> +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> + u8 *data)
> +{
> + struct dsa_switch *ds = dev->ds;
> + struct phy_device *phydev;
> + int cnt = 0;
> +
> + phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> + if (*data & PORT_PHY_INT) {
> + /* Handle the interrupt if there is no PHY device or its
> + * interrupt is not registered yet.
> + */
> + if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> + u8 phy_status;
> +
> + ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> + &phy_status);
> + if (phydev)
> + phy_trigger_machine(phydev);
> + ++cnt;
> + *data &= ~PORT_PHY_INT;
> + }
> + }
This looks like a layering violation. Why is this needed? An interrupt
controller generally has no idea what the individual interrupt is
about. It just calls into the interrupt core to get the handler
called, and then clears the interrupt. Why does that not work here?
What other DSA drivers do if they need to handle some of the
interrupts is just request the interrupt like any other driver:
https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-639x.c#L95
> +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data)
> +{
> + irqreturn_t ret = IRQ_NONE;
> + u32 data32;
> +
> + if (port > 0)
> + return ksz9477_handle_port_irq(dev, port - 1, data);
> +
> + ksz_read32(dev, REG_SW_INT_STATUS__4, &data32);
> + if (data32 & LUE_INT) {
> + u8 lue;
> +
> + ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue);
> + ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue);
> + if (lue & LEARN_FAIL_INT)
> + dev_info_ratelimited(dev->dev, "lue learn fail\n");
> + if (lue & WRITE_FAIL_INT)
> + dev_info_ratelimited(dev->dev, "lue write fail\n");
> + ret = IRQ_HANDLED;
> + }
https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/global1_atu.c#L474
Andrew
Powered by blists - more mailing lists