RE: [RFC net-next] net: phy: Add basic support for Synopsys XPCS using a PHY driver

From: Jose Abreu
Date: Mon Jan 13 2020 - 09:07:56 EST


From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
Date: Jan/13/2020, 13:42:47 (UTC+00:00)

> > +#define SYNOPSYS_XPHY_ID 0x7996ced0
> > +#define SYNOPSYS_XPHY_MASK 0xffffffff
>
> GENMASK() ?
> (It seems bits.h is missed in the headers block)

This is on purpose, it's an ID and the mask is more clearly read this
way (in my opinion).

>
> > +#define DW_USXGMII_2500 (BIT(5))
> > +#define DW_USXGMII_1000 (BIT(6))
> > +#define DW_USXGMII_100 (BIT(13))
> > +#define DW_USXGMII_10 (0)
>
> Useless parentheses.

Just coding style, to keep it aligned with the other speeds, you can
call it OCD :)

> > +static int dw_poll_reset(struct phy_device *phydev, int dev)
> > +{
> > + /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
> > + unsigned int retries = 12;
> > + int ret;
> > +
> > + do {
>
> > + msleep(50);
>
> It's a bit unusual to have timeout loop to be started with sleep.
> Imagine the case when between writing a reset bit and actual checking
> the scheduling happens and reset has been done You add here
> unnecessary 50 ms of waiting.

Intended also. Actually, unconditional sleep allows to safely wait for
reset and not try to poll the bit in the middle of reset where the FSM
may not be operational and reads may not return correctly. This is also
needed because in some configurations the XPCS does not allow reads in
the middle of a reset.

> > +static int __dw_soft_reset(struct phy_device *phydev, int dev, int reg)
> > +{
> > + int val;
>
> val?! Perhaps ret is better name?
> Applicable to the rest of functions.

This is the most commonly used pattern in net/phy subsystem.

> > +#define dw_warn(__phy, __args...) \
>
> dw_warn() -> dw_warn_if_phy_link()

Hmm, this will probably make the warns lines pass the 80 chars and I
don't like to break lines :)

> > +static int dw_aneg_done(struct phy_device *phydev)
> > +{
> > + int val;
> > +
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > + if (val < 0)
> > + return val;
> > +
> > + if (val & MDIO_AN_STAT1_COMPLETE) {
> > + val = phy_read_mmd(phydev, MDIO_MMD_AN, DW_SR_AN_LP_ABL1);
> > + if (val < 0)
> > + return val;
> > +
> > + /* Check if Aneg outcome is valid */
> > + if (!(val & 0x1))
> > + goto fault;
> > +
>
> > + return 1;
>
> 1?! What does it mean?

Just like phy_aneg_done():
"* Description: Return the auto-negotiation status from this @phydev
* Returns > 0 on success or < 0 on error. 0 means that
auto-negotiation
* is still pending."

I'll add remaining changes in official version. Thanks for the review!

---
Thanks,
Jose Miguel Abreu