Re: [PATCH net-next 3/3] net: phy: mediatek: add driver for built-in 2.5G ethernet PHY on MT7988

[Daniel Golle]

Hi Sky,

On Thu, Jan 16, 2025 at 09:21:58AM +0800, Sky Huang wrote:
> From: Sky Huang <skylake.huang@xxxxxxxxxxxx>
> 
> Add support for internal 2.5Gphy on MT7988. This driver will load
> necessary firmware and add appropriate time delay to make sure
> that firmware works stably. Also, certain control registers will
> be set to fix link-up issues.
> 
> Signed-off-by: Sky Huang <skylake.huang@xxxxxxxxxxxx>
> ---
>  MAINTAINERS                          |   1 +
>  drivers/net/phy/mediatek/Kconfig     |  11 +
>  drivers/net/phy/mediatek/Makefile    |   1 +
>  drivers/net/phy/mediatek/mtk-2p5ge.c | 343 +++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+)
>  create mode 100644 drivers/net/phy/mediatek/mtk-2p5ge.c
> 
> [...]
> +static int mt798x_2p5ge_phy_probe(struct phy_device *phydev)
> +{
> +	struct mtk_i2p5ge_phy_priv *priv;
> +
> +	priv = devm_kzalloc(&phydev->mdio.dev,
> +			    sizeof(struct mtk_i2p5ge_phy_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_2P5GPHY_ID_MT7988:
> +		/* The original hardware only sets MDIO_DEVS_PMAPMD */
> +		phydev->c45_ids.mmds_present |= MDIO_DEVS_PCS |
> +						MDIO_DEVS_AN |
> +						MDIO_DEVS_VEND1 |
> +						MDIO_DEVS_VEND2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	priv->fw_loaded = false;
> +	phydev->priv = priv;
> +
> +	mtk_phy_leds_state_init(phydev);

Calling mtk_phy_leds_state_init() can't work without also defining
led_hw_control_get() for that driver.

This is what mtk_phy_leds_state_init() does:
        for (i = 0; i < 2; ++i)
                phydev->drv->led_hw_control_get(phydev, i, NULL);

The driver lacking led_hw_control_get() method (see below) will make
this a call to a NULL function pointer.

Imho it's fine to add the driver without support for the LEDs for now
and add LED support later on. But in that case you also shouldn't call
mtk_phy_leds_state_init().

> +
> +	return 0;
> +}
> +
> +static struct phy_driver mtk_2p5gephy_driver[] = {
> +	{
> +		PHY_ID_MATCH_MODEL(MTK_2P5GPHY_ID_MT7988),
> +		.name = "MediaTek MT7988 2.5GbE PHY",
> +		.probe = mt798x_2p5ge_phy_probe,
> +		.config_init = mt798x_2p5ge_phy_config_init,
> +		.config_aneg = mt798x_2p5ge_phy_config_aneg,
> +		.get_features = mt798x_2p5ge_phy_get_features,
> +		.read_status = mt798x_2p5ge_phy_read_status,
> +		.get_rate_matching = mt798x_2p5ge_phy_get_rate_matching,
> +		.suspend = genphy_suspend,
> +		.resume = genphy_resume,
> +		.read_page = mtk_phy_read_page,
> +		.write_page = mtk_phy_write_page,
> +	},
> +};
> +
> +module_phy_driver(mtk_2p5gephy_driver);
> +
> +static struct mdio_device_id __maybe_unused mtk_2p5ge_phy_tbl[] = {
> +	{ PHY_ID_MATCH_VENDOR(0x00339c00) },
> +	{ }
> +};
> +
> +MODULE_DESCRIPTION("MediaTek 2.5Gb Ethernet PHY driver");
> +MODULE_AUTHOR("SkyLake Huang <SkyLake.Huang@xxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> +
> +MODULE_DEVICE_TABLE(mdio, mtk_2p5ge_phy_tbl);
> +MODULE_FIRMWARE(MT7988_2P5GE_PMB_FW);
> -- 
> 2.45.2
>