Messages in this thread | | | Date | Wed, 1 Apr 2020 16:11:13 +0200 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next 6/9] net: phy: add backplane kr driver support |
| |
> > > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, bp_phy- > > >bp_dev.mdio.an_status); > > > + val = phy_read_mmd(bpphy, MDIO_MMD_AN, > > > + bp_phy->bp_dev.mdio.an_status); > > > > Why not just > > > > val = phy_read_mmd(bpphy, MDIO_MMD_AN, MDIO_CTRL1); > > > > Or is your hardware not actually conformant to the standard? > > > > There has also been a lot of discussion of reading the status twice is correct or > > not. Don't you care the link has briefly gone down and up again? > > > > Andrew > > This could be changed to use directly the MDIO_STAT1 in order to read > AN status (and use MDIO_CTRL1 for writing the control register) but this > is more flexible and more readable
Less readale. MDIO_STAT1 is well known. What does bp_dev.mdio.an_status mean?
In general, core code should be KISS, and assume everything follows the standard. We don't want to scatter workarounds for non-conformant hardware in core code. Such workarounds should be in the drivers where they are hidden away.
> + bpkr->mdio.an_control = MDIO_CTRL1; > + bpkr->mdio.an_status = MDIO_STAT1; > + bpkr->mdio.an_ad_ability_0 = MDIO_PMA_EXTABLE_10GBKR; > + bpkr->mdio.an_ad_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 1; > + bpkr->mdio.an_lp_base_page_ability_1 = MDIO_PMA_EXTABLE_10GBKR + 4;
Please drop this, and use the #defines directly.
Andrew
| |