Re: [RFC net-next v4 2/2] net: phy: adin1100: Add interrupt support for link change

[Horatiu Vultur]

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@xxxxxxxxxxxxxxxxxxxxx>
> ---
> 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