Received-SPF: pass (google.com: domain of linux-kernel+bounces-32450-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 22 Jan 2024 08:42:58 +0100
From: Horatiu Vultur <horatiu.vultur@microchip.com>
To: Andre Werner <andre.werner@systec-electronic.com>
CC: <andrew@lunn.ch>, <hkallweit1@gmail.com>, <davem@davemloft.net>,
	<edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
	<linux@armlinux.org.uk>, <netdev@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [RFC net-next v4 2/2] net: phy: adin1100: Add interrupt support
 for link change
Message-ID: <20240122074258.zmbzngrl7dzhkvwo@DEN-DL-M31836.microchip.com>
References: <20240121201511.8997-1-andre.werner@systec-electronic.com>
 <20240121201511.8997-3-andre.werner@systec-electronic.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20240121201511.8997-3-andre.werner@systec-electronic.com>

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