On Sun, Feb 28, 2021 at 12:00:27PM -0800, Jakub Kicinski wrote:
> On Fri, 26 Feb 2021 17:30:20 +0200 Ioana Ciornei wrote:
> > diff --git a/drivers/net/phy/dp83822.c b/drivers/net/phy/dp83822.c
> > index be1224b4447b..f7a2ec150e54 100644
> > --- a/drivers/net/phy/dp83822.c
> > +++ b/drivers/net/phy/dp83822.c
> > @@ -290,6 +290,7 @@ static int dp83822_config_intr(struct phy_device
> > *phydev)
> >
> > static irqreturn_t dp83822_handle_interrupt(struct phy_device *phydev)
> > {
> > + bool trigger_machine = false;
> > int irq_status;
> >
> > /* The MISR1 and MISR2 registers are holding the interrupt status in
> > @@ -305,7 +306,7 @@ static irqreturn_t dp83822_handle_interrupt(struct
> > phy_device *phydev)
> > return IRQ_NONE;
> > }
> > if (irq_status & ((irq_status & GENMASK(7, 0)) << 8))
> > - goto trigger_machine;
> > + trigger_machine = true;
> >
> > irq_status = phy_read(phydev, MII_DP83822_MISR2);
> > if (irq_status < 0) {
> > @@ -313,11 +314,11 @@ static irqreturn_t dp83822_handle_interrupt(struct
> > phy_device *phydev)
> > return IRQ_NONE;
> > }
> > if (irq_status & ((irq_status & GENMASK(7, 0)) << 8))
> > - goto trigger_machine;
> > + trigger_machine = true;
> >
> > - return IRQ_NONE;
> > + if (!trigger_machine)
> > + return IRQ_NONE;
> >
> > -trigger_machine:
> > phy_trigger_machine(phydev);
> >
> > return IRQ_HANDLED;
>
> Would it be better to code it up as:
>
> irqreturn_t ret = IRQ_NONE;
>
> if (irq_status & ...)
> ret = IRQ_HANDLED;
>
> /* .. */
>
> if (ret != IRQ_NONE)
> phy_trigger_machine(phydev);
>
> return ret;
>
> That reads a tiny bit better to me, but it's probably majorly
> subjective so I'm happy with existing patch if you prefer it.
I think I prefer it as it is and this is because it would be in line
with all the other PHY drivers which do this:
if (!(irq_status & int_enabled))
return IRQ_NONE;
phy_trigger_machine(phydev);
return IRQ_HANDLED;
Ioana
