From: Jose Abreu <Jose.Abreu@synopsys.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: netdev <netdev@vger.kernel.org>,
Joao Pinto <Joao.Pinto@synopsys.com>,
Andrew Lunn <andrew@lunn.ch>,
Florian Fainelli <f.fainelli@gmail.com>,
Heiner Kallweit <hkallweit1@gmail.com>,
"David S. Miller" <davem@davemloft.net>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: RE: [RFC net-next] net: phy: Add basic support for Synopsys XPCS using a PHY driver
Date: Mon, 13 Jan 2020 14:07:48 +0000 [thread overview]
Message-ID: <BN8PR12MB3266363BD930BEC46E9680E2D3350@BN8PR12MB3266.namprd12.prod.outlook.com> (raw)
In-Reply-To: <CAHp75VeOefY_BK7MitFtdb7enrh5TOOwZ8kDJfVxvW28gejUbg@mail.gmail.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
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
prev parent reply other threads:[~2020-01-13 14:07 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-13 13:11 [RFC net-next] net: phy: Add basic support for Synopsys XPCS using a PHY driver Jose Abreu
2020-01-13 13:38 ` Andrew Lunn
2020-01-13 13:54 ` Jose Abreu
2020-01-13 14:18 ` Russell King - ARM Linux admin
2020-01-13 14:27 ` Jose Abreu
2020-01-13 14:55 ` Vladimir Oltean
2020-01-13 15:05 ` Jose Abreu
2020-01-13 14:50 ` Vladimir Oltean
2020-01-13 15:09 ` Russell King - ARM Linux admin
2020-01-20 10:31 ` Jose Abreu
2020-01-20 10:50 ` Russell King - ARM Linux admin
2020-01-20 11:07 ` Jose Abreu
2020-01-20 11:39 ` Russell King - ARM Linux admin
[not found] ` <e942b414-08bd-0305-9128-26666a7a5d5a@linux.intel.com>
2020-02-04 9:41 ` Chng, Jack Ping
2020-03-09 9:22 ` Jose Abreu
2020-01-13 13:42 ` [RFC " Andy Shevchenko
2020-01-13 14:07 ` Jose Abreu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN8PR12MB3266363BD930BEC46E9680E2D3350@BN8PR12MB3266.namprd12.prod.outlook.com \
--to=jose.abreu@synopsys.com \
--cc=Joao.Pinto@synopsys.com \
--cc=andrew@lunn.ch \
--cc=andy.shevchenko@gmail.com \
--cc=davem@davemloft.net \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).