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?
