Re: [PATCH net-next] net: phy: mediatek-ge-soc: initialize MT7988 PHY LEDs default state

[Andrew Lunn <andrew@lunn.ch>]

> +/* Registers on MDIO_MMD_VEND2 */
> +#define MTK_PHY_LED0_ON_CTRL			0x24
> +#define MTK_PHY_LED1_ON_CTRL			0x26
> +#define   MTK_PHY_LED_ON_MASK			GENMASK(6, 0)
> +#define   MTK_PHY_LED_ON_LINK1000		BIT(0)
> +#define   MTK_PHY_LED_ON_LINK100		BIT(1)
> +#define   MTK_PHY_LED_ON_LINK10			BIT(2)
> +#define   MTK_PHY_LED_ON_LINKDOWN		BIT(3)
> +#define   MTK_PHY_LED_ON_FDX			BIT(4) /* Full duplex */
> +#define   MTK_PHY_LED_ON_HDX			BIT(5) /* Half duplex */
> +#define   MTK_PHY_LED_FORCE_ON			BIT(6)
> +#define   MTK_PHY_LED_POLARITY			BIT(14)
> +#define   MTK_PHY_LED_ENABLE			BIT(15)

Would enable being clear result in the LED being off? You can force it
on with MTK_PHY_LED_FORCE_ON | MTK_PHY_LED_ENABLE? That gives you
enough to allow software control of the LED. You can then implement
the led_brightness_set() op, if you want.

I assume the above are specific to LED0? It would be good to include
the 0 in the name, to make that clear.

> +
> +#define MTK_PHY_LED0_BLINK_CTRL			0x25
> +#define MTK_PHY_LED1_BLINK_CTRL			0x27
> +#define   MTK_PHY_LED_1000TX			BIT(0)

So do this mean LINK1000 + blink on TX ?

> +#define   MTK_PHY_LED_1000RX			BIT(1)

So do this mean LINK1000 + blink on RX ?

It would be good to add a comment, because at some point it is likely
somebody will want to offload the ledtrig-netdev and will need to
understand what these really do.

> +#define   MTK_PHY_LED_100TX			BIT(2)
> +#define   MTK_PHY_LED_100RX			BIT(3)
> +#define   MTK_PHY_LED_10TX			BIT(4)
> +#define   MTK_PHY_LED_10RX			BIT(5)
> +#define   MTK_PHY_LED_COLLISION			BIT(6)
> +#define   MTK_PHY_LED_RX_CRC_ERR		BIT(7)
> +#define   MTK_PHY_LED_RX_IDLE_ERR		BIT(8)
> +#define   MTK_PHY_LED_FORCE_BLINK		BIT(9)

Is there a way to force the LED1 off/on?  I guess not setting any of
these bits will force it off.

So the polarity and enable bits apply to LED1? 

> +
>  #define MTK_PHY_RG_BG_RASEL			0x115
>  #define   MTK_PHY_RG_BG_RASEL_MASK		GENMASK(2, 0)
>  
> +/* Register in boottrap syscon defining the initial state of the 4 PHY LEDs */

Should this be bootstrap? You had the same in the commit message.

Also, i'm confused. Here you say 4 PHY LEDs, yet

> +#define MTK_PHY_LED0_ON_CTRL			0x24
> +#define MTK_PHY_LED1_ON_CTRL			0x26

suggests there are two LEDs?

Should these actually be :

> +#define MTK_PHY_LED_ON_CTRL1			0x24
> +#define MTK_PHY_LED_ON_CTRL2			0x26

meaning each LED has two control registers?

MTK_PHY_LED_ON_LINK1000 should actually be MTK_PHY_LED_ON_CTRL1_LINK1000 ?
MTK_PHY_LED_100TX should be MTK_PHY_LED_CTRL2_100TX ?

I find it good practice to ensure a bit value #define includes enough
information in its name to clear indicate what register it applies to.

> +static int mt798x_phy_setup_led(struct phy_device *phydev, bool inverted)
> +{
> +	struct pinctrl *pinctrl;
> +	const u16 led_on_ctrl_defaults = MTK_PHY_LED_ENABLE      |
> +					 MTK_PHY_LED_ON_LINK1000 |
> +					 MTK_PHY_LED_ON_LINK100  |
> +					 MTK_PHY_LED_ON_LINK10;
> +	const u16 led_blink_defaults = MTK_PHY_LED_1000TX |
> +				       MTK_PHY_LED_1000RX |
> +				       MTK_PHY_LED_100TX  |
> +				       MTK_PHY_LED_100RX  |
> +				       MTK_PHY_LED_10TX   |
> +				       MTK_PHY_LED_10RX;
> +
> +	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
> +		      led_on_ctrl_defaults ^
> +		      (inverted ? MTK_PHY_LED_POLARITY : 0));
> +
> +	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
> +		      led_on_ctrl_defaults);
> +

Now i'm even more confused. Both have the same value, expect the
polarity bit?

Please add a lot of comments about how this hardware actually works!

> +	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_BLINK_CTRL,
> +		      led_blink_defaults);
> +
> +	phy_write_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_BLINK_CTRL,
> +		      led_blink_defaults);
> +
> +	pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");

This is also very unusual. At minimum, it needs a comment as to why it
is needed. But more likely, because no other driver in driver/net does
this, it makes me think it is wrong.

> +static bool mt7988_phy_get_boottrap_polarity(struct phy_device *phydev)
> +{
> +	struct mtk_socphy_shared *priv = phydev->shared->priv;
> +
> +	if (priv->boottrap & BIT(phydev->mdio.addr))
> +		return false;

This can be simplified to

	return !priv->boottrap & BIT(phydev->mdio.addr);

	Andrew