| Message ID | 20240121201511.8997-3-andre.werner@systec-electronic.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Prevent nullptr exceptions in ISR | expand |
The 01/21/2024 20:54, Andre Werner wrote: Hi Andre, > An interrupt handler was added to the driver as well as functions > to enable interrupts at the phy. > > There are several interrupts maskable at the phy, but only link change > interrupts are handled by the driver yet. > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> > --- > v4: > - Change read-modify-write behavior as suggested to phy_modify_mmd. Usually it is good to keep the change log also from the previous versions, so it is easier to see what has been changed. > --- > drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 56 insertions(+) > > diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c > index 7619d6185801..7c82384e5d30 100644 > --- a/drivers/net/phy/adin1100.c > +++ b/drivers/net/phy/adin1100.c > @@ -18,6 +18,12 @@ > #define PHY_ID_ADIN1110 0x0283bc91 > #define PHY_ID_ADIN2111 0x0283bca1 > > +#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021 > +#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1) > + > +#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011 > +#define ADIN_LINK_STAT_CHNG BIT(1) > + > #define ADIN_FORCED_MODE 0x8000 > #define ADIN_FORCED_MODE_EN BIT(0) > > @@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev) > return genphy_c45_config_aneg(phydev); > } > > +static int adin_phy_ack_intr(struct phy_device *phydev) > +{ > + /* Clear pending interrupts */ > + int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2, > + ADIN_PHY_SUBSYS_IRQ_STATUS); > + > + return rc < 0 ? rc : 0; > +} > + > +static int adin_config_intr(struct phy_device *phydev) > +{ > + int ret; > + u16 irq_mask; Please use reverse x-mas notation here. > + > + ret = adin_phy_ack_intr(phydev); > + No new line here, between ret and if. > + if (ret) > + return ret; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > + irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN; > + else > + irq_mask = 0; > + > + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + ADIN_PHY_SUBSYS_IRQ_MASK, > + ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask); > +} > + > +static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev) > +{ > + int irq_status; > + > + irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2, > + ADIN_PHY_SUBSYS_IRQ_STATUS); > + if (irq_status < 0) { > + phy_error(phydev); > + return IRQ_NONE; > + } > + > + if (!(irq_status & ADIN_LINK_STAT_CHNG)) > + return IRQ_NONE; > + > + phy_trigger_machine(phydev); > + > + return IRQ_HANDLED; > +} > + > static int adin_set_powerdown_mode(struct phy_device *phydev, bool en) > { > int ret; > @@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = { > .probe = adin_probe, > .config_aneg = adin_config_aneg, > .read_status = adin_read_status, > + .config_intr = adin_config_intr, > + .handle_interrupt = adin_phy_handle_interrupt, > .set_loopback = adin_set_loopback, > .suspend = adin_suspend, > .resume = adin_resume, > -- > 2.43.0 > >
Dear Maintainers, I'm a bit confused about the patch submitting process for net-next. Do I need to send the patchset again, if the merge window is opened again and the patchset was previously submitted as RFC? Best regards. Andre On Mon, 22 Jan 2024, Horatiu Vultur wrote: > The 01/21/2024 20:54, Andre Werner wrote: > > Hi Andre, > > >> An interrupt handler was added to the driver as well as functions >> to enable interrupts at the phy. >> >> There are several interrupts maskable at the phy, but only link change >> interrupts are handled by the driver yet. >> >> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> >> --- >> v4: >> - Change read-modify-write behavior as suggested to phy_modify_mmd. > > Usually it is good to keep the change log also from the previous > versions, so it is easier to see what has been changed. > >> --- >> drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 56 insertions(+) >> >> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c >> index 7619d6185801..7c82384e5d30 100644 >> --- a/drivers/net/phy/adin1100.c >> +++ b/drivers/net/phy/adin1100.c >> @@ -18,6 +18,12 @@ >> #define PHY_ID_ADIN1110 0x0283bc91 >> #define PHY_ID_ADIN2111 0x0283bca1 >> >> +#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021 >> +#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1) >> + >> +#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011 >> +#define ADIN_LINK_STAT_CHNG BIT(1) >> + >> #define ADIN_FORCED_MODE 0x8000 >> #define ADIN_FORCED_MODE_EN BIT(0) >> >> @@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev) >> return genphy_c45_config_aneg(phydev); >> } >> >> +static int adin_phy_ack_intr(struct phy_device *phydev) >> +{ >> + /* Clear pending interrupts */ >> + int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2, >> + ADIN_PHY_SUBSYS_IRQ_STATUS); >> + >> + return rc < 0 ? rc : 0; >> +} >> + >> +static int adin_config_intr(struct phy_device *phydev) >> +{ >> + int ret; >> + u16 irq_mask; > > Please use reverse x-mas notation here. > >> + >> + ret = adin_phy_ack_intr(phydev); >> + > > No new line here, between ret and if. > >> + if (ret) >> + return ret; >> + >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) >> + irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN; >> + else >> + irq_mask = 0; >> + >> + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, >> + ADIN_PHY_SUBSYS_IRQ_MASK, >> + ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask); >> +} >> + >> +static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev) >> +{ >> + int irq_status; >> + >> + irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2, >> + ADIN_PHY_SUBSYS_IRQ_STATUS); >> + if (irq_status < 0) { >> + phy_error(phydev); >> + return IRQ_NONE; >> + } >> + >> + if (!(irq_status & ADIN_LINK_STAT_CHNG)) >> + return IRQ_NONE; >> + >> + phy_trigger_machine(phydev); >> + >> + return IRQ_HANDLED; >> +} >> + >> static int adin_set_powerdown_mode(struct phy_device *phydev, bool en) >> { >> int ret; >> @@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = { >> .probe = adin_probe, >> .config_aneg = adin_config_aneg, >> .read_status = adin_read_status, >> + .config_intr = adin_config_intr, >> + .handle_interrupt = adin_phy_handle_interrupt, >> .set_loopback = adin_set_loopback, >> .suspend = adin_suspend, >> .resume = adin_resume, >> -- >> 2.43.0 >> >> > > -- > /Horatiu >
On Mon, Jan 29, 2024 at 01:51:11PM +0100, Andre Werner wrote: > Dear Maintainers, > > I'm a bit confused about the patch submitting process for net-next. > Do I need to send the patchset again, if the merge window is opened again and > the patchset was previously submitted as RFC? RFC patches should in theory never be merged. So that alone is sufficient to require a resend. Anything sent during the merge window is also not eligible for merging. So please rebase on net-next, collect and add any Acked-by: Reviewed-by: etc and resend. Andrew
diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c index 7619d6185801..7c82384e5d30 100644 --- a/drivers/net/phy/adin1100.c +++ b/drivers/net/phy/adin1100.c @@ -18,6 +18,12 @@ #define PHY_ID_ADIN1110 0x0283bc91 #define PHY_ID_ADIN2111 0x0283bca1 +#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021 +#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1) + +#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011 +#define ADIN_LINK_STAT_CHNG BIT(1) + #define ADIN_FORCED_MODE 0x8000 #define ADIN_FORCED_MODE_EN BIT(0) @@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev) return genphy_c45_config_aneg(phydev); } +static int adin_phy_ack_intr(struct phy_device *phydev) +{ + /* Clear pending interrupts */ + int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2, + ADIN_PHY_SUBSYS_IRQ_STATUS); + + return rc < 0 ? rc : 0; +} + +static int adin_config_intr(struct phy_device *phydev) +{ + int ret; + u16 irq_mask; + + ret = adin_phy_ack_intr(phydev); + + if (ret) + return ret; + + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) + irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN; + else + irq_mask = 0; + + return phy_modify_mmd(phydev, MDIO_MMD_VEND2, + ADIN_PHY_SUBSYS_IRQ_MASK, + ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask); +} + +static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev) +{ + int irq_status; + + irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2, + ADIN_PHY_SUBSYS_IRQ_STATUS); + if (irq_status < 0) { + phy_error(phydev); + return IRQ_NONE; + } + + if (!(irq_status & ADIN_LINK_STAT_CHNG)) + return IRQ_NONE; + + phy_trigger_machine(phydev); + + return IRQ_HANDLED; +} + static int adin_set_powerdown_mode(struct phy_device *phydev, bool en) { int ret; @@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = { .probe = adin_probe, .config_aneg = adin_config_aneg, .read_status = adin_read_status, + .config_intr = adin_config_intr, + .handle_interrupt = adin_phy_handle_interrupt, .set_loopback = adin_set_loopback, .suspend = adin_suspend, .resume = adin_resume,
An interrupt handler was added to the driver as well as functions to enable interrupts at the phy. There are several interrupts maskable at the phy, but only link change interrupts are handled by the driver yet. Signed-off-by: Andre Werner <andre.werner@systec-electronic.com> --- v4: - Change read-modify-write behavior as suggested to phy_modify_mmd. --- drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+)