Messages in this thread | | | From | Kai-Heng Feng <> | Date | Fri, 14 Jan 2022 23:25:17 +0800 | Subject | Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware |
| |
On Fri, Jan 14, 2022 at 9:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > static void marvell_config_led(struct phy_device *phydev) > > { > > - u16 def_config; > > + struct marvell_priv *priv = phydev->priv; > > int err; > > > > - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) { > > - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */ > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R): > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S): > > - def_config = MII_88E1121_PHY_LED_DEF; > > - break; > > - /* Default PHY LED config: > > - * LED[0] .. 1000Mbps Link > > - * LED[1] .. 100Mbps Link > > - * LED[2] .. Blink, Activity > > - */ > > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510): > > - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE) > > - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE; > > - else > > - def_config = MII_88E1510_PHY_LED_DEF; > > - break; > > - default: > > + if (priv->led_def_config == -1) > > return; > > + > > + if (priv->led_def_config) > > + goto write; > > Really? > > Please restructure this code. Take it apart into helpers. You need: > > A function to set the actual LED configuration. > A function to decide what, if any, configuration to set > A function to store the current configuration on suspend. > A function to restore the current configuration on resume. > > Lots of little functions will make it much easier to understand, and > avoid 1980s BASIC style.
Sure, will turn these into helper functions.
> > I'm also surprised you need to deal with suspend/resume. Why does the > BIOS not set the LED mode on resume, same as it does on power up?
I was told this is a BIOS limitation. I'll ask vendor _why_ the LED can't be restored by BIOS.
Kai-Heng
> > Andrew
| |