| Message ID | 20220310145200.3645763-1-ioana.ciornei@nxp.com |
|---|---|
| Headers | show |
| Series | dpaa2-mac: add support for changing the protocol at runtime | expand |
On Thu, Mar 10, 2022 at 04:51:59PM +0200, Ioana Ciornei wrote: > This patch integrates the dpaa2-eth driver with the generic PHY > infrastructure in order to search, find and reconfigure the SerDes lanes > in case of a protocol change. > > On the .mac_config() callback, the phy_set_mode_ext() API is called so > that the Lynx 28G SerDes PHY driver can change the lane's configuration. > In the same phylink callback the MC firmware is called so that it > reconfigures the MAC side to run using the new protocol. > > The consumer drivers - dpaa2-eth and dpaa2-switch - are updated to call > the dpaa2_mac_start/stop functions newly added which will > power_on/power_off the associated SerDes lane. > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> Looks better, there's a minor thing that I missed, sorry: > + if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE && > + !phy_interface_mode_is_rgmii(mac->if_mode) && > + is_of_node(dpmac_node)) { > + serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL); > + > + if (IS_ERR(serdes_phy)) { > + if (PTR_ERR(serdes_phy) == -ENODEV) > + serdes_phy = NULL; > + else > + return PTR_ERR(serdes_phy); > + } else { > + phy_init(serdes_phy); > + } Would: if (PTR_ERR(serdes_phy) == -ENODEV) serdes_phy = NULL; else if (IS_ERR(serdes_phy)) return PTR_ERR(serdes_phy); else phy_init(serdes_phy); be neater? There is no need to check IS_ERR() before testing PTR_ERR(). One may also prefer the pointer-comparison approach: if (serdes_phy == ERR_PTR(-ENODEV)) to remove any question about PTR_ERR(p) on a !IS_ERR(p) value too, but it really doesn't make any difference. I suspect this is just a code formatting issue, I'd think the compiler would generate reasonable code either way, so as I said above, it's quite minor.
On Thu, Mar 10, 2022 at 03:05:50PM +0000, Russell King (Oracle) wrote: > On Thu, Mar 10, 2022 at 04:51:59PM +0200, Ioana Ciornei wrote: > > This patch integrates the dpaa2-eth driver with the generic PHY > > infrastructure in order to search, find and reconfigure the SerDes lanes > > in case of a protocol change. > > > > On the .mac_config() callback, the phy_set_mode_ext() API is called so > > that the Lynx 28G SerDes PHY driver can change the lane's configuration. > > In the same phylink callback the MC firmware is called so that it > > reconfigures the MAC side to run using the new protocol. > > > > The consumer drivers - dpaa2-eth and dpaa2-switch - are updated to call > > the dpaa2_mac_start/stop functions newly added which will > > power_on/power_off the associated SerDes lane. > > > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > Looks better, there's a minor thing that I missed, sorry: > > > + if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE && > > + !phy_interface_mode_is_rgmii(mac->if_mode) && > > + is_of_node(dpmac_node)) { > > + serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL); > > + > > + if (IS_ERR(serdes_phy)) { > > + if (PTR_ERR(serdes_phy) == -ENODEV) > > + serdes_phy = NULL; > > + else > > + return PTR_ERR(serdes_phy); > > + } else { > > + phy_init(serdes_phy); > > + } > > Would: > if (PTR_ERR(serdes_phy) == -ENODEV) > serdes_phy = NULL; > else if (IS_ERR(serdes_phy)) > return PTR_ERR(serdes_phy); > else > phy_init(serdes_phy); > Yes, it wouldn't be an if inside another if statement. > be neater? There is no need to check IS_ERR() before testing PTR_ERR(). > One may also prefer the pointer-comparison approach: > > if (serdes_phy == ERR_PTR(-ENODEV)) > > to remove any question about PTR_ERR(p) on a !IS_ERR(p) value too, but > it really doesn't make any difference. > > I suspect this is just a code formatting issue, I'd think the compiler > would generate reasonable code either way, so as I said above, it's > quite minor. > As you said, since it's quite minor I am going to wait to see if more comments will appear, if not I am going to fix this up in another patch. Thanks!