On Mon, 1 Mar 2021 10:21:14 +0200 Ioana Ciornei wrote:
> 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);
>
Okay, applied, thanks!
