| Message ID | 20230405-net-next-topic-net-phy-reset-v1-3-7e5329f08002@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rework PHY reset handling | expand |
> +void phy_device_set_miits(struct phy_device *phydev, > + struct mii_timestamper *mii_ts) > +{ > + if (!phydev) > + return; > + > + if (phydev->mii_ts) { > + phydev_dbg(phydev, > + "MII timestamper already set -> skip set\n"); > + return; > + } > + > + phydev->mii_ts = mii_ts; > +} We tend to be less paranoid. Few, if any, other functions test that phydev is not NULL. And the current code allows overwriting of an existing stamper. If you think overwriting should not be allowed return -EINVAL, and change all the callers to test for it. > +EXPORT_SYMBOL(phy_device_set_miits); _GPL please. The code is a bit inconsistent, but new symbols should be EXPORT_SYMBOL_GPL. I do however like this patch, hiding away the internals of phydev. Andrew
On 23-04-05, Andrew Lunn wrote: > > +void phy_device_set_miits(struct phy_device *phydev, > > + struct mii_timestamper *mii_ts) > > +{ > > + if (!phydev) > > + return; > > + > > + if (phydev->mii_ts) { > > + phydev_dbg(phydev, > > + "MII timestamper already set -> skip set\n"); > > + return; > > + } > > + > > + phydev->mii_ts = mii_ts; > > +} > > We tend to be less paranoid. Few, if any, other functions test that > phydev is not NULL. And the current code allows overwriting of an > existing stamper. If you think overwriting should not be allowed > return -EINVAL, and change all the callers to test for it. I can drop the 'if (!phydev)' check if you want. Return -EINVAL is possible too. > > +EXPORT_SYMBOL(phy_device_set_miits); > > _GPL please. The code is a bit inconsistent, but new symbols should be > EXPORT_SYMBOL_GPL. Sure! Don't know why I added it as EXPORT_SYMBOL in the first place. > I do however like this patch, hiding away the internals of phydev. Thanks for the fast response. Regards, Marco > > Andrew >
diff --git a/drivers/net/phy/bcm-phy-ptp.c b/drivers/net/phy/bcm-phy-ptp.c index ef00d6163061..08bd3d96ce04 100644 --- a/drivers/net/phy/bcm-phy-ptp.c +++ b/drivers/net/phy/bcm-phy-ptp.c @@ -905,7 +905,7 @@ static void bcm_ptp_init(struct bcm_ptp_private *priv) priv->mii_ts.hwtstamp = bcm_ptp_hwtstamp; priv->mii_ts.ts_info = bcm_ptp_ts_info; - priv->phydev->mii_ts = &priv->mii_ts; + phy_device_set_miits(priv->phydev, &priv->mii_ts); } struct bcm_ptp_private *bcm_ptp_probe(struct phy_device *phydev) diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c index ef8b14135133..144c264cb4eb 100644 --- a/drivers/net/phy/dp83640.c +++ b/drivers/net/phy/dp83640.c @@ -1456,7 +1456,7 @@ static int dp83640_probe(struct phy_device *phydev) for (i = 0; i < MAX_RXTS; i++) list_add(&dp83640->rx_pool_data[i].list, &dp83640->rxpool); - phydev->mii_ts = &dp83640->mii_ts; + phy_device_set_miits(phydev, &dp83640->mii_ts); phydev->priv = dp83640; spin_lock_init(&dp83640->rx_lock); diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 2184b1e859ae..d01c4157f704 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -3054,7 +3054,7 @@ static void lan8814_ptp_init(struct phy_device *phydev) ptp_priv->mii_ts.hwtstamp = lan8814_hwtstamp; ptp_priv->mii_ts.ts_info = lan8814_ts_info; - phydev->mii_ts = &ptp_priv->mii_ts; + phy_device_set_miits(phydev, &ptp_priv->mii_ts); } static int lan8814_ptp_probe_once(struct phy_device *phydev) diff --git a/drivers/net/phy/mscc/mscc_ptp.c b/drivers/net/phy/mscc/mscc_ptp.c index cf728bfd83e2..8c38b4efcedc 100644 --- a/drivers/net/phy/mscc/mscc_ptp.c +++ b/drivers/net/phy/mscc/mscc_ptp.c @@ -1485,7 +1485,7 @@ static int __vsc8584_init_ptp(struct phy_device *phydev) vsc8531->mii_ts.txtstamp = vsc85xx_txtstamp; vsc8531->mii_ts.hwtstamp = vsc85xx_hwtstamp; vsc8531->mii_ts.ts_info = vsc85xx_ts_info; - phydev->mii_ts = &vsc8531->mii_ts; + phy_device_set_miits(phydev, &vsc8531->mii_ts); memcpy(&vsc8531->ptp->caps, &vsc85xx_clk_caps, sizeof(vsc85xx_clk_caps)); diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c index 5813b07242ce..360812cea6d6 100644 --- a/drivers/net/phy/nxp-c45-tja11xx.c +++ b/drivers/net/phy/nxp-c45-tja11xx.c @@ -1326,7 +1326,7 @@ static int nxp_c45_probe(struct phy_device *phydev) priv->mii_ts.txtstamp = nxp_c45_txtstamp; priv->mii_ts.hwtstamp = nxp_c45_hwtstamp; priv->mii_ts.ts_info = nxp_c45_ts_info; - phydev->mii_ts = &priv->mii_ts; + phy_device_set_miits(phydev, &priv->mii_ts); ret = nxp_c45_init_ptp_clock(priv); } else { phydev_dbg(phydev, "PTP support not enabled even if the phy supports it"); diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 64292e47e3fc..e4d08dcfed70 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -732,6 +732,22 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, } EXPORT_SYMBOL(phy_device_create); +void phy_device_set_miits(struct phy_device *phydev, + struct mii_timestamper *mii_ts) +{ + if (!phydev) + return; + + if (phydev->mii_ts) { + phydev_dbg(phydev, + "MII timestamper already set -> skip set\n"); + return; + } + + phydev->mii_ts = mii_ts; +} +EXPORT_SYMBOL(phy_device_set_miits); + /* phy_c45_probe_present - checks to see if a MMD is present in the package * @bus: the target MII bus * @prtad: PHY package address on the MII bus diff --git a/include/linux/phy.h b/include/linux/phy.h index 2f83cfc206e5..c17a98712555 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1541,6 +1541,8 @@ int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum, struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id, bool is_c45, struct phy_c45_device_ids *c45_ids); +void phy_device_set_miits(struct phy_device *phydev, + struct mii_timestamper *mii_ts); #if IS_ENABLED(CONFIG_PHYLIB) int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id); struct mdio_device *fwnode_mdio_find_device(struct fwnode_handle *fwnode);
Provide a small setter helper to set the mii timestamper. The helper detects possible overrides and hides the phydev internal from the driver. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/net/phy/bcm-phy-ptp.c | 2 +- drivers/net/phy/dp83640.c | 2 +- drivers/net/phy/micrel.c | 2 +- drivers/net/phy/mscc/mscc_ptp.c | 2 +- drivers/net/phy/nxp-c45-tja11xx.c | 2 +- drivers/net/phy/phy_device.c | 16 ++++++++++++++++ include/linux/phy.h | 2 ++ 7 files changed, 23 insertions(+), 5 deletions(-)