RE: [PATCH net-next] net: phy: microchip_t1: Interrupt support for lan887x

[Divya.Koppera]

Hi Parthiban,

Thanks for the review comments. Please find my reply inline.

> -----Original Message-----
> From: Parthiban Veerasooran - I17164
> <Parthiban.Veerasooran@xxxxxxxxxxxxx>
> Sent: Tuesday, October 1, 2024 5:28 PM
> To: Divya Koppera - I30481 <Divya.Koppera@xxxxxxxxxxxxx>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@xxxxxxxxxxxxx>;
> UNGLinuxDriver <UNGLinuxDriver@xxxxxxxxxxxxx>; andrew@xxxxxxx;
> hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next] net: phy: microchip_t1: Interrupt support for
> lan887x
> 
> Hi,
> 
> On 30/09/24 9:04 pm, Divya Koppera wrote:
> > Add support for link up and link down interrupts in lan887x.
> >
> > Signed-off-by: Divya Koppera <divya.koppera@xxxxxxxxxxxxx>
> > ---
> >   drivers/net/phy/microchip_t1.c | 63
> ++++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/net/phy/microchip_t1.c
> > b/drivers/net/phy/microchip_t1.c index a5ef8fe50704..383050a5b0ed
> > 100644
> > --- a/drivers/net/phy/microchip_t1.c
> > +++ b/drivers/net/phy/microchip_t1.c
> > @@ -226,6 +226,18 @@
> >   #define MICROCHIP_CABLE_MAX_TIME_DIFF	\
> >   	(MICROCHIP_CABLE_MIN_TIME_DIFF +
> MICROCHIP_CABLE_TIME_MARGIN)
> >
> > +#define LAN887X_INT_STS				0xf000
> > +#define LAN887X_INT_MSK				0xf001
> > +#define LAN887X_INT_MSK_T1_PHY_INT_MSK		BIT(2)
> > +#define LAN887X_INT_MSK_LINK_UP_MSK		BIT(1)
> > +#define LAN887X_INT_MSK_LINK_DOWN_MSK		BIT(0)
> > +
> > +#define LAN887X_MX_CHIP_TOP_LINK_MSK
> 	(LAN887X_INT_MSK_LINK_UP_MSK |\
> > +
> LAN887X_INT_MSK_LINK_DOWN_MSK)
> > +
> > +#define LAN887X_MX_CHIP_TOP_ALL_MSK
> 	(LAN887X_INT_MSK_T1_PHY_INT_MSK |\
> > +					 LAN887X_MX_CHIP_TOP_LINK_MSK)
> > +
> >   #define DRIVER_AUTHOR	"Nisar Sayed <nisar.sayed@xxxxxxxxxxxxx>"
> >   #define DRIVER_DESC	"Microchip LAN87XX/LAN937x/LAN887x T1
> PHY driver"
> >
> > @@ -1474,6 +1486,7 @@ static void lan887x_get_strings(struct phy_device
> *phydev, u8 *data)
> >   		ethtool_puts(&data, lan887x_hw_stats[i].string);
> >   }
> >
> > +static int lan887x_config_intr(struct phy_device *phydev);
> >   static int lan887x_cd_reset(struct phy_device *phydev,
> >   			    enum cable_diag_state cd_done)
> >   {
> > @@ -1504,6 +1517,10 @@ static int lan887x_cd_reset(struct phy_device
> *phydev,
> >   		if (rc < 0)
> >   			return rc;
> >
> > +		rc = lan887x_config_intr(phydev);
> > +		if (rc < 0)
> > +			return rc;
> > +
> >   		rc = lan887x_phy_reconfig(phydev);
> >   		if (rc < 0)
> >   			return rc;
> > @@ -1830,6 +1847,50 @@ static int lan887x_cable_test_get_status(struct
> phy_device *phydev,
> >   	return lan887x_cable_test_report(phydev);
> >   }
> >
> > +static int lan887x_config_intr(struct phy_device *phydev) {
> > +	int ret;
> > +
> > +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> > +		/* Clear the interrupt status before enabling interrupts */
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_INT_STS);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		/* Unmask for enabling interrupt */
> > +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_INT_MSK,
> > +				    (u16)~LAN887X_MX_CHIP_TOP_ALL_MSK);
> > +	} else {
> > +		ret = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_INT_MSK,
> > +				    GENMASK(15, 0));
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		ret = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_INT_STS);
> > +	}
> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> > +
> > +static irqreturn_t lan887x_handle_interrupt(struct phy_device
> > +*phydev) {
> > +	int ret = IRQ_NONE;
> I think you can remove 'ret' by simply returning IRQ_HANDLED and
> IRQ_NONE directly to save one line.
> 
> Best regards,
> Parthiban V

This is declared for future series purpose.
As suggested, I'll remove this variable now and introduce in next series.

/Divya

> > +	int irq_status;
> > +
> > +	irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_INT_STS);
> > +	if (irq_status < 0) {
> > +		phy_error(phydev);
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	if (irq_status & LAN887X_MX_CHIP_TOP_LINK_MSK) {
> > +		phy_trigger_machine(phydev);
> > +		ret = IRQ_HANDLED;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >   static struct phy_driver microchip_t1_phy_driver[] = {
> >   	{
> >   		PHY_ID_MATCH_MODEL(PHY_ID_LAN87XX),
> > @@ -1881,6 +1942,8 @@ static struct phy_driver microchip_t1_phy_driver[]
> = {
> >   		.read_status	= genphy_c45_read_status,
> >   		.cable_test_start = lan887x_cable_test_start,
> >   		.cable_test_get_status = lan887x_cable_test_get_status,
> > +		.config_intr    = lan887x_config_intr,
> > +		.handle_interrupt = lan887x_handle_interrupt,
> >   	}
> >   };
> >