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

[Jose Abreu]

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