> -----Original Message-----
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Friday, July 24, 2015 12:39 PM
> To: shh....@gmail.com; netdev@vger.kernel.org; da...@davemloft.net
> Cc: Xie Shaohui-B21989
> Subject: Re: [PATCH] phylib: add driver for aquantia phy
>
> Le 07/23/15 20:46, shh....@gmail.com a écrit :
> > From: Shaohui Xie <shaohui....@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?
[S.H] OK.
>
> See comments below
>
> >
> > Signed-off-by: Shaohui Xie <shaohui....@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?
[S.H] OK.
How about 10Mbits/sec and duplex are we
> guaranteed to be full-duplex at e.g: 100 or 10Mbits/sec?
[S.H] The PHY does not support 10M bits/sec.
When link to 100M. the phy is full-duplex.
>
> > +
> > + 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
[S.H] there are several defines for 10G PHYs, should be used by a given 10G
PHY.
for this Aquantia PHY, SUPPORTED_10000baseT_Full is a valid define, should I
set it as below:
.features = PHY_GBIT_FEATURES | SUPPORTED_10000baseT_Full,
Or handle the SUPPORTED_10000baseT_Full separately?
Thanks for reviewing!
Shaohui
