Re: [PATCH net-next] net: phy: add driver for MediaTek SoC built-in GE PHYs

[Andrew Lunn <andrew@lunn.ch>]

> +/* Registers on MDIO_MMD_VEND1 */
> +#define MTK_PHY_MIDDLE_LEVEL_SHAPPER_0TO1	0
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TO1	1
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TO1	2
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_1TO0	4
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_1TO0	5 /* N means negative */
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_0TON1	7
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_0TON1	8
> +#define MTK_PHY_1st_OVERSHOOT_LEVEL_N1TO0	10
> +#define MTK_PHY_2nd_OVERSHOOT_LEVEL_N1TO0	11

Mixed case like this is very unusual.

> +static int tx_amp_fill_result(struct phy_device *phydev, u16 *buf)
> +{
> +	int i;
> +	int bias[16] = {0};
> +	const int vals_9461[16] = { 7, 1, 4, 7,
> +				    7, 1, 4, 7,
> +				    7, 1, 4, 7,
> +				    7, 1, 4, 7 };
> +	const int vals_9481[16] = { 10, 6, 6, 10,
> +				    10, 6, 6, 10,
> +				    10, 6, 6, 10,
> +				    10, 6, 6, 10 };
> +
> +	switch (phydev->drv->phy_id) {
> +	case MTK_GPHY_ID_MT7981:
> +		/* We add some calibration to efuse values
> +		 * due to board level influence.
> +		 * GBE: +7, TBT: +1, HBT: +4, TST: +7
> +		 */
> +		memcpy(bias, (const void *)vals_9461, sizeof(bias));
> +		for (i = 0; i <= 12; i += 4) {
> +			if (likely(buf[i >> 2] + bias[i] >= 32)) {
> +				bias[i] -= 13;
> +			} else {
> +				phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> +					       0x5c, 0x7 << i, bias[i] << i);
> +				bias[i + 1] += 13;
> +				bias[i + 2] += 13;
> +				bias[i + 3] += 13;

How does 13 map to GBE: +7, TBT: +1, HBT: +4, TST: +7 ?

> +static inline void mt798x_phy_common_finetune(struct phy_device *phydev)

No inline functions in .c files. Let the compiler decide. There
appears to be a number of these. And they are big function, too big to
make sense to inline.

>  static struct phy_driver mtk_gephy_driver[] = {
>  	{
> -		PHY_ID_MATCH_EXACT(0x03a29412),
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7530),
>  		.name		= "MediaTek MT7530 PHY",
>  		.config_init	= mt7530_phy_config_init,
>  		/* Interrupts are handled by the switch, not the PHY
> @@ -84,7 +1205,7 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		.write_page	= mtk_gephy_write_page,
>  	},
>  	{
> -		PHY_ID_MATCH_EXACT(0x03a29441),
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7531),
>  		.name		= "MediaTek MT7531 PHY",
>  		.config_init	= mt7531_phy_config_init,
>  		/* Interrupts are handled by the switch, not the PHY

Useful changes, but please put them in a separate patch.

> @@ -97,16 +1218,42 @@ static struct phy_driver mtk_gephy_driver[] = {
>  		.read_page	= mtk_gephy_read_page,
>  		.write_page	= mtk_gephy_write_page,
>  	},
> +#if IS_ENABLED(CONFIG_MEDIATEK_GE_PHY_SOC)
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7981),
> +		.name		= "MediaTek MT7981 PHY",
> +		.probe		= mt7981_phy_probe,
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_gephy_read_page,
> +		.write_page	= mtk_gephy_write_page,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(MTK_GPHY_ID_MT7988),
> +		.name		= "MediaTek MT7988 PHY",
> +		.probe		= mt7988_phy_probe,
> +		.config_intr	= genphy_no_config_intr,
> +		.handle_interrupt = genphy_handle_interrupt_no_ack,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.read_page	= mtk_gephy_read_page,
> +		.write_page	= mtk_gephy_write_page,

So the only thing these two new PHYs share with the other two PHYs is
mtk_gephy_read_page and mtk_gephy_write_page?

static int mtk_gephy_read_page(struct phy_device *phydev)
{
        return __phy_read(phydev, MTK_EXT_PAGE_ACCESS);
}

static int mtk_gephy_write_page(struct phy_device *phydev, int page)
{
        return __phy_write(phydev, MTK_EXT_PAGE_ACCESS, page);
}

Given the size of the new code, maybe consider adding
mediatek-ge-soc.c and make a copy these two functions?

	Andrew