Re: [PATCH] net: phy: add support for Motorcomm yt8531C phy

[Andrew Lunn]

> +#define YT8531_RGMII_CONFIG1	0xa003
> +
> +/* TX Gig-E Delay is bits 3:0, default 0x1
> + * TX Fast-E Delay is bits 7:4, default 0xf
> + * RX Delay is bits 13:10, default 0x0
> + * Delay = 150ps * N
> + * On = 2000ps, off = 50ps
> + */
> +#define YT8531_DELAY_GE_TX_EN	(0xd << 0)
> +#define YT8531_DELAY_GE_TX_DIS	(0x0 << 0)

The comments above and the value here don't correspond.  These seem to
be enable/disable, which is usually a single bit. Here you have 3 bits
set? And what about the default 0x1?

0xd is 13. 13*150 = 1950, which is about 2000ps?

So YT8531_DELAY_GE_TX_EN is not really enable, it is
YT8531_DELAY_GE_TX_1950_PS, and YT8531_DELAY_GE_TX_DIS should be
YT8531_DELAY_GE_TX_0_PS.

> +#define YT8531_DELAY_FE_TX_EN	(0xd << 4)
> +#define YT8531_DELAY_FE_TX_DIS	(0x0 << 4)

Default is 0xf?

> +#define YT8531_DELAY_RX_EN	(0xd << 10)
> +#define YT8531_DELAY_RX_DIS	(0x0 << 10)
> +#define YT8531_DELAY_MASK	(GENMASK(13, 10) | GENMASK(7, 0))

Please rework these.

> +	ret = __phy_write(phydev, YT8511_PAGE, YT8531_CLKCFG_125M);
> +	if (ret < 0)
> +		goto err_restore_page;

This if statement is pointless.

> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +