diff mbox series

[03/12] net: phy: add phy_device_set_miits helper

Message ID 20230405-net-next-topic-net-phy-reset-v1-3-7e5329f08002@pengutronix.de
State New
Headers
Series Rework PHY reset handling |

Commit Message

Marco Felsch April 5, 2023, 9:26 a.m. UTC 
  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(-)

Comments

Andrew Lunn April 5, 2023, 12:06 p.m. UTC | #1 
> +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
Marco Felsch April 5, 2023, 2:49 p.m. UTC | #2 
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 mbox series

Patch

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);