Re: [PATCH net-next v10 08/13] ptp: Add phc source and helpers to register specific PTP clock or get information

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Kory Maincent wrote:
> Prepare for future hardware timestamp selection by adding source and
> corresponding pointers to ptp_clock structure. Additionally, introduce
> helpers for registering specific phydev or netdev PTP clocks, retrieving
> PTP clock information such as hwtstamp source or phydev/netdev pointers,
> and obtaining the ptp_clock structure from the phc index.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
> 
> Change in v8:
> - New patch.
> 
> Change in v10:
> - Add get and put function to avoid unregistering a ptp clock object used.
> - Fix kdoc issues.
> ---
>  drivers/ptp/ptp_clock.c          | 114 +++++++++++++++++++++++++++++++++++++++
>  drivers/ptp/ptp_private.h        |   5 ++
>  include/linux/ptp_clock_kernel.h | 104 +++++++++++++++++++++++++++++++++++
>  3 files changed, 223 insertions(+)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index c56cd0f63909..f962f460ec9d 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -512,6 +512,120 @@ void ptp_cancel_worker_sync(struct ptp_clock *ptp)
>  }
>  EXPORT_SYMBOL(ptp_cancel_worker_sync);
>  
> +struct ptp_clock *netdev_ptp_clock_register(struct ptp_clock_info *info,
> +					    struct net_device *dev)
> +{
> +	struct ptp_clock *ptp;
> +
> +	ptp = ptp_clock_register(info, &dev->dev);
> +	if (IS_ERR(ptp))
> +		return ptp;
> +
> +	ptp->phc_source = HWTSTAMP_SOURCE_NETDEV;
> +	ptp->netdev = dev;
> +
> +	return ptp;
> +}
> +EXPORT_SYMBOL(netdev_ptp_clock_register);
> +
> +struct ptp_clock *phydev_ptp_clock_register(struct ptp_clock_info *info,
> +					    struct phy_device *phydev)
> +{
> +	struct ptp_clock *ptp;
> +
> +	ptp = ptp_clock_register(info, &phydev->mdio.dev);
> +	if (IS_ERR(ptp))
> +		return ptp;
> +
> +	ptp->phc_source = HWTSTAMP_SOURCE_PHYLIB;
> +	ptp->phydev = phydev;
> +
> +	return ptp;
> +}
> +EXPORT_SYMBOL(phydev_ptp_clock_register);
> +
> +bool ptp_clock_from_phylib(struct ptp_clock *ptp)
> +{
> +	return ptp->phc_source == HWTSTAMP_SOURCE_PHYLIB;
> +}
> +EXPORT_SYMBOL(ptp_clock_from_phylib);
> +
> +bool ptp_clock_from_netdev(struct ptp_clock *ptp)
> +{
> +	return ptp->phc_source == HWTSTAMP_SOURCE_NETDEV;
> +}
> +EXPORT_SYMBOL(ptp_clock_from_netdev);
> +
> +struct net_device *ptp_clock_netdev(struct ptp_clock *ptp)
> +{
> +	if (ptp->phc_source != HWTSTAMP_SOURCE_NETDEV)
> +		return NULL;
> +
> +	return ptp->netdev;
> +}
> +EXPORT_SYMBOL(ptp_clock_netdev);
> +
> +struct phy_device *ptp_clock_phydev(struct ptp_clock *ptp)
> +{
> +	if (ptp->phc_source != HWTSTAMP_SOURCE_PHYLIB)
> +		return NULL;
> +
> +	return ptp->phydev;
> +}
> +EXPORT_SYMBOL(ptp_clock_phydev);

IMHO these four helpers just add a layer of indirection without much
benefit.

Do we ever expect more than two sources? Else, the phc_source could be
embedded as the least significant bit of the pointer in the union. In
that case we would need helpers to return the pointer without that LSB.
But space in struct ptp_clock is probably not so valuable that we need
to play such games.

> +/**
> + * netdev_ptp_clock_register() - register a PTP hardware clock driver for
> + *				 a net device
> + *
> + * @info: Structure describing the new clock.
> + * @dev:  Pointer of the net device
> + */
> +
> +extern struct ptp_clock *
> +netdev_ptp_clock_register(struct ptp_clock_info *info,
> +			  struct net_device *dev);

No need for explicit extern?