Re: [PATCH] phylib: add driver for aquantia phy

[Florian Fainelli <f.fainelli@gmail.com>]

Le 07/23/15 20:46, shh.xie@gmail.com a écrit :
> From: Shaohui Xie <Shaohui.Xie@freescale.com>
> 
> This patch added driver to support Aquantia PHYs AQ1202, AQ2104, AQR105,
> AQR405, which accessed through clause 45.

Could you prefix your patches with "net: phy: " in the future to be
consistent with what is typically used?

See comments below

> 
> Signed-off-by: Shaohui Xie <Shaohui.Xie@freescale.com>
> ---

[snip]

> +static int aquantia_read_status(struct phy_device *phydev)
> +{
> +	int reg;
> +
> +	phydev->speed = SPEED_10000;
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> +	if (reg & MDIO_STAT1_LSTATUS)
> +		phydev->link = 1;
> +	else
> +		phydev->link = 0;
> +
> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
> +	mdelay(10);
> +	reg = phy_read_mmd(phydev, MDIO_MMD_AN, 0xc800);
> +	if (reg == 0x9)
> +		phydev->speed = SPEED_2500;
> +	else if (reg == 0x5)
> +		phydev->speed = SPEED_1000;
> +	else if (reg == 0x3)
> +		phydev->speed = SPEED_100;

Could we use a switch/case here? How about 10Mbits/sec and duplex are we
guaranteed to be full-duplex at e.g: 100 or 10Mbits/sec?

> +
> +	return 0;
> +}
> +
> +static struct phy_driver aquantia_driver[] = {
> +{
> +	.phy_id		= PHY_ID_AQ1202,
> +	.phy_id_mask	= 0xfffffff0,
> +	.name		= "Aquantia AQ1202",
> +	.features	= PHY_GBIT_FEATURES,

If these are 10GbE PHYs, should not we start defining a new features
bitmask here to reflect that accordingly? That way MAC

> +	.soft_reset	= aquantia_soft_reset,
> +	.aneg_done	= aquantia_aneg_done,
> +	.config_init    = aquantia_config_init,
> +	.config_aneg    = aquantia_config_aneg,
> +	.read_status	= aquantia_read_status,
> +	.driver		= { .owner = THIS_MODULE,},
> +},
> +{
> +	.phy_id		= PHY_ID_AQ2104,
> +	.phy_id_mask	= 0xfffffff0,
> +	.name		= "Aquantia AQ2104",
> +	.features	= PHY_GBIT_FEATURES,
> +	.soft_reset	= aquantia_soft_reset,
> +	.aneg_done	= aquantia_aneg_done,
> +	.config_init    = aquantia_config_init,
> +	.config_aneg    = aquantia_config_aneg,
> +	.read_status	= aquantia_read_status,
> +	.driver		= { .owner = THIS_MODULE,},
> +},
> +{
> +	.phy_id		= PHY_ID_AQR105,
> +	.phy_id_mask	= 0xfffffff0,
> +	.name		= "Aquantia AQR105",
> +	.features	= PHY_GBIT_FEATURES,
> +	.soft_reset	= aquantia_soft_reset,
> +	.aneg_done	= aquantia_aneg_done,
> +	.config_init    = aquantia_config_init,
> +	.config_aneg    = aquantia_config_aneg,
> +	.read_status	= aquantia_read_status,
> +	.driver		= { .owner = THIS_MODULE,},
> +},
> +{
> +	.phy_id		= PHY_ID_AQR405,
> +	.phy_id_mask	= 0xfffffff0,
> +	.name		= "Aquantia AQR405",
> +	.features	= PHY_GBIT_FEATURES,
> +	.soft_reset	= aquantia_soft_reset,
> +	.aneg_done	= aquantia_aneg_done,
> +	.config_init    = aquantia_config_init,
> +	.config_aneg    = aquantia_config_aneg,
> +	.read_status	= aquantia_read_status,
> +	.driver		= { .owner = THIS_MODULE,},
> +},
> +};
> +
> +static int __init aquantia_init(void)
> +{
> +	return phy_drivers_register(aquantia_driver,
> +				    ARRAY_SIZE(aquantia_driver));
> +}
> +
> +static void __exit aquantia_exit(void)
> +{
> +	return phy_drivers_unregister(aquantia_driver,
> +				      ARRAY_SIZE(aquantia_driver));
> +}
> +
> +module_init(aquantia_init);
> +module_exit(aquantia_exit);
> +
> +static struct mdio_device_id __maybe_unused aquantia_tbl[] = {
> +	{ PHY_ID_AQ1202, 0xfffffff0 },
> +	{ PHY_ID_AQ2104, 0xfffffff0 },
> +	{ PHY_ID_AQR105, 0xfffffff0 },
> +	{ PHY_ID_AQR405, 0xfffffff0 },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, aquantia_tbl);
> +
> +MODULE_DESCRIPTION("Aquantia PHY driver");
> +MODULE_AUTHOR("Shaohui Xie <Shaohui.Xie@freescale.com>");
> +MODULE_LICENSE("GPL v2");
> 


-- 
Florian