Re: [PATCH 4/5] net: phy: marvell: Add LED accessors for Marvell 88E1510

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Apr 20, 2022 at 08:40:51PM +0800, Kai-Heng Feng wrote:
> Implement get_led_config() and set_led_config() callbacks so phy core
> can use firmware LED as platform requested.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/net/phy/marvell.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 2702faf7b0f60..c5f13e09b0692 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -750,6 +750,30 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
>  	return err;
>  }
>  
> +static int marvell_get_led_config(struct phy_device *phydev)
> +{
> +	int led;
> +
> +	led = phy_read_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL);
> +	if (led < 0) {
> +		phydev_warn(phydev, "Fail to get marvell phy LED.\n");
> +		led = 0;
> +	}

I've said this multiple times, there are three LED registers, The
Function Control register, the Priority Control register and the Timer
control register. It is the combination of all three that defines how
the LEDs work. You need to save all of them.

And you need to make your API generic enough that the PHY driver can
save anywhere from 1 bit to 42 bytes of configuration.

I don't know ACPI, but i'm pretty sure this is not the ACPI way of
doing this. I think you should be defining an ACPI method, which
phylib can call after initialising the hardware to allow the firmware
to configure the LED.

     Andrew