Re: [PATCH net-next 1/2] net: phy: marvell: add new m88e1510 LED configuration

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/13/2019 8:31 PM, Jian Shen wrote:
> The default m88e1510 LED configuration is 0x1177, used LED[0]
> for 1000M link, LED[1] for 100M link, and LED[2] for active.
> But for our boards, we want to use 0x1040, which use LED[0] for
> link, and LED[1] for active.
> 
> This patch adds a new m88e1510 LED configuration for it.

There appears to be a precedent with the DNS323 flag that was defined
for the same purpose, but this unfortunately does not scale we cannot
have every new platform come up with its own LED configuration without
having a more structured approach to representing the LED configuration.

Maybe we can encode the desired LED behavior in a more generic way and
utilize the 32 flag bits available to denote a selection, e.g.:

MARVELL_PHY_FLAG_LED0_100M	BIT(3)
MARVELL_PHY_FLAG_LED0_1000M	BIT(4)

etc.

or maybe even better would be to expose the LEDs using the standard LEDs
class subsystem and allow configuring different triggers. We have some
amount of support for PHY LEDs already in tree, but AFAIR what we do not
have support for is a "hardware blinking" trigger which those LEDs are.

> 
> Signed-off-by: Jian Shen <shenjian15@huawei.com>
> ---
>  drivers/net/phy/marvell.c   | 22 +++++++++++++++++++++-
>  include/linux/marvell_phy.h |  1 +
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 3ccba37..c195286 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -128,6 +128,10 @@
>  #define MII_PHY_LED_CTRL	        16
>  #define MII_88E1121_PHY_LED_DEF		0x0030
>  #define MII_88E1510_PHY_LED_DEF		0x1177
> +#define MII_88E1510_PHY_HNS3_LED_DEF	0x1040
> +
> +#define MII_88E1510_PHY_LED_POLARITY_CTRL	0x11
> +#define MII_88E1510_PHY_HNS3_LED_POLARITY	0x4415
>  
>  #define MII_M1011_PHY_STATUS		0x11
>  #define MII_M1011_PHY_STATUS_1000	0x8000
> @@ -619,12 +623,19 @@ static void marvell_config_led(struct phy_device *phydev)
>  		def_config = MII_88E1121_PHY_LED_DEF;
>  		break;
>  	/* Default PHY LED config:
> +	 * For hns3:
> +	 * LED[0] .. Link
> +	 * LED[1] .. Activity
> +	 * For others:
>  	 * LED[0] .. 1000Mbps Link
>  	 * LED[1] .. 100Mbps Link
>  	 * LED[2] .. Blink, Activity
>  	 */
>  	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
> -		def_config = MII_88E1510_PHY_LED_DEF;
> +		if (phydev->dev_flags & MARVELL_PHY_M1510_HNS3_LEDS)
> +			def_config = MII_88E1510_PHY_HNS3_LED_DEF;
> +		else
> +			def_config = MII_88E1510_PHY_LED_DEF;
>  		break;
>  	default:
>  		return;
> @@ -634,6 +645,15 @@ static void marvell_config_led(struct phy_device *phydev)
>  			      def_config);
>  	if (err < 0)
>  		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
> +
> +	if (phydev->dev_flags & MARVELL_PHY_M1510_HNS3_LEDS) {
> +		err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
> +				      MII_88E1510_PHY_LED_POLARITY_CTRL,
> +				      MII_88E1510_PHY_HNS3_LED_POLARITY);
> +		if (err < 0)
> +			phydev_warn(phydev,
> +				    "Fail to config marvell phy LED polarity.\n");
> +	}
>  }
>  
>  static int marvell_config_init(struct phy_device *phydev)
> diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
> index 1eb6f24..99e0bbb 100644
> --- a/include/linux/marvell_phy.h
> +++ b/include/linux/marvell_phy.h
> @@ -32,5 +32,6 @@
>  /* struct phy_device dev_flags definitions */
>  #define MARVELL_PHY_M1145_FLAGS_RESISTANCE	0x00000001
>  #define MARVELL_PHY_M1118_DNS323_LEDS		0x00000002
> +#define MARVELL_PHY_M1510_HNS3_LEDS		0x00000004
>  
>  #endif /* _MARVELL_PHY_H */
> 

-- 
Florian