Re: [RFC PATCH net-next 1/4] net: phy: used the genphy_soft_reset for phy reset in Lan87xx

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Feb 28, 2022 at 07:35:07PM +0530, Arun Ramadoss wrote:
> Replaced current code for soft resetting phy to genphy_soft_reset
> function. And added the macro for LAN87xx Phy ID.

Hi Arun

Please don't mix multiple things in one patch.

Looking at the actual path, you have:

> +#define LAN87XX_PHY_ID			0x0007c150
> +#define MICROCHIP_PHY_ID_MASK		0xfffffff0

Part of macros for PHY ID.

> +
>  /* External Register Control Register */
>  #define LAN87XX_EXT_REG_CTL                     (0x14)
>  #define LAN87XX_EXT_REG_CTL_RD_CTL              (0x1000)
> @@ -197,20 +200,10 @@ static int lan87xx_phy_init(struct phy_device *phydev)
>  	if (rc < 0)
>  		return rc;
>  
> -	/* Soft Reset the SMI block */
> -	rc = access_ereg_modify_changed(phydev, PHYACC_ATTR_BANK_SMI,
> -					0x00, 0x8000, 0x8000);
> -	if (rc < 0)
> -		return rc;
> -
> -	/* Check to see if the self-clearing bit is cleared */
> -	usleep_range(1000, 2000);
> -	rc = access_ereg(phydev, PHYACC_ATTR_MODE_READ,
> -			 PHYACC_ATTR_BANK_SMI, 0x00, 0);
> +	/* phy Soft reset */
> +	rc = genphy_soft_reset(phydev);
>  	if (rc < 0)
>  		return rc;
> -	if ((rc & 0x8000) != 0)
> -		return -ETIMEDOUT;

Soft reset.

>  
>  	/* PHY Initialization */
>  	for (i = 0; i < ARRAY_SIZE(init); i++) {
> @@ -273,6 +266,9 @@ static int lan87xx_config_init(struct phy_device *phydev)
>  {
>  	int rc = lan87xx_phy_init(phydev);
>  
> +	if (rc < 0)
> +		phydev_err(phydev, "failed to initialize phy\n");
> +

A new error message.

>  	return rc < 0 ? rc : 0;
>  }
>  
> @@ -506,18 +502,14 @@ static int lan87xx_cable_test_get_status(struct phy_device *phydev,
>  
>  static struct phy_driver microchip_t1_phy_driver[] = {
>  	{
> -		.phy_id         = 0x0007c150,
> -		.phy_id_mask    = 0xfffffff0,
> -		.name           = "Microchip LAN87xx T1",
> +		.phy_id         = LAN87XX_PHY_ID,
> +		.phy_id_mask    = MICROCHIP_PHY_ID_MASK,

2nd part of the PHY ID macros.

> +		.name           = "LAN87xx T1",

A change in name.

>  		.flags          = PHY_POLL_CABLE_TEST,
> -
>  		.features       = PHY_BASIC_T1_FEATURES,
> -
>  		.config_init	= lan87xx_config_init,
> -
>  		.config_intr    = lan87xx_phy_config_intr,
>  		.handle_interrupt = lan87xx_handle_interrupt,
> -

White space changes.

You can also use PHY_ID_MATCH_MODEL().

    Andrew