Re: [PATCH net-next v19 03/10] ptp: Add phc source and helpers to register specific PTP clock or get information

[Jakub Kicinski]

On Wed, 30 Oct 2024 14:54:45 +0100 Kory Maincent wrote:
> @@ -41,6 +43,11 @@ struct ptp_clock {
>  	struct ptp_clock_info *info;
>  	dev_t devid;
>  	int index; /* index into clocks.map */
> +	enum hwtstamp_source phc_source;
> +	union { /* Pointer of the phc_source device */
> +		struct net_device *netdev;
> +		struct phy_device *phydev;
> +	};

Storing the info about the "user" (netdev, phydev) in the "provider"
(PHC) feels too much like a layering violation. Why do you need this?

In general I can't shake the feeling that we're trying to configure 
the "default" PHC for a narrow use case, while the goal should be 
to let the user pick the PHC per socket.

> +/**
> + * 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.

> +/**
> + * ptp_clock_from_netdev() - Does the PTP clock comes from netdev
> + *
> + * @ptp:  The clock obtained from net/phy_ptp_clock_register().
> + *
> + * Return: True if the PTP clock comes from netdev, false otherwise.

> +/**
> + * ptp_clock_netdev() - Obtain the net_device reference of PTP clock

nit: pick one way to spell netdev ?

> +	ret = ptp_clock_get(dev, ptp);
> +	if (ret)
> +		return ERR_PTR(ret);

why do you take references on the ptp device?