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>]

Willem de Bruijn wrote:
> 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.

Actually, ignore this. This is not important enough to revise when
we're alredy at v10. That also goes for the other points.

> 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?