Received-SPF: pass (google.com: domain of linux-kernel+bounces-139699-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 10 Apr 2024 23:18:19 -0400
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>, 
 Kory Maincent <kory.maincent@bootlin.com>, 
 Florian Fainelli <florian.fainelli@broadcom.com>, 
 Broadcom internal kernel review list <bcm-kernel-feedback-list@broadcom.com>, 
 Andrew Lunn <andrew@lunn.ch>, 
 Heiner Kallweit <hkallweit1@gmail.com>, 
 Russell King <linux@armlinux.org.uk>, 
 "David S. Miller" <davem@davemloft.net>, 
 Eric Dumazet <edumazet@google.com>, 
 Jakub Kicinski <kuba@kernel.org>, 
 Paolo Abeni <pabeni@redhat.com>, 
 Richard Cochran <richardcochran@gmail.com>, 
 Radu Pirea <radu-nicolae.pirea@oss.nxp.com>, 
 Jay Vosburgh <j.vosburgh@gmail.com>, 
 Andy Gospodarek <andy@greyhouse.net>, 
 Nicolas Ferre <nicolas.ferre@microchip.com>, 
 Claudiu Beznea <claudiu.beznea@tuxon.dev>, 
 Willem de Bruijn <willemdebruijn.kernel@gmail.com>, 
 Jonathan Corbet <corbet@lwn.net>, 
 Horatiu Vultur <horatiu.vultur@microchip.com>, 
 UNGLinuxDriver@microchip.com, 
 Simon Horman <horms@kernel.org>, 
 Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>, 
 netdev@vger.kernel.org, 
 linux-kernel@vger.kernel.org, 
 linux-doc@vger.kernel.org, 
 Maxime Chevallier <maxime.chevallier@bootlin.com>, 
 Rahul Rameshbabu <rrameshbabu@nvidia.com>, 
 Kory Maincent <kory.maincent@bootlin.com>
Message-ID: <6617567bd4da7_2dcc3c294c5@willemb.c.googlers.com.notmuch>
In-Reply-To: <66175265992c8_2d6bc6294d8@willemb.c.googlers.com.notmuch>
References: <20240409-feature_ptp_netnext-v10-0-0fa2ea5c89a9@bootlin.com>
 <20240409-feature_ptp_netnext-v10-8-0fa2ea5c89a9@bootlin.com>
 <66175265992c8_2d6bc6294d8@willemb.c.googlers.com.notmuch>
Subject: Re: [PATCH net-next v10 08/13] ptp: Add phc source and helpers to
 register specific PTP clock or get information
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

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?