Re: [PATCH net-next 1/5] net: mdio: add timestamped write operation

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * __mdiobus_write_sts - Timestamped version of the __mdiobus_write function
> + * @bus: the mii_bus struct
> + * @addr: the phy address
> + * @regnum: register number to write
> + * @val: value to write to @regnum
> + * @sts: system timestamps bounding completion, or NULL
> + *
> + * Return: Zero if successful, negative error code on failure. Returns
> + *	   -EBUSY or -EINVAL if the system timestamps are not valid. If @sts
> + *	   is not NULL, -EOPNOTSUPP is returned only if
> + *	   mdiobus_supports_write_sts() is false.
> + *
> + * Write a MDIO bus register, with system timestamps bounding completion;
> + * a transfer is considered complete on the rising edge of the MDC
> + * that clocks the last data bit. Caller must hold the mdio bus lock.
> + *
> + * For clocks that can be stepped, validate the clock generation through
> + * the raw time of the upper bound.
> + *
> + * If @sts is NULL, perform an ordinary write.
> + *

Please don't do that. We don't want drivers calling this when they
don't need to.

> +		if (!ktime_before(now.monoraw, sts->post_sts.monoraw))
> +			return 0;
> +
> +		/* Cap the wait at 1 ms, which is more than any single
> +		 * write's delay. This guards against an aux clock being
> +		 * disabled and reenabled, which restarts its raw time.
> +		 */
> +		if (!ktime_before(ktime_get_raw(), deadline))
> +			return -EBUSY;

A bit banging MDIO bus might be slower.

Also, i've not thought about the mux case, where the write gets
blocked until some other operation is performed on the shared hardware
of the mux.

> +	/**
> +	 * @write_sts: Perform a write transfer on the bus,
> +	 * with system timestamps bounding its completion. Set only
> +	 * if timestamps can always be provided. Must not return
> +	 * -EOPNOTSUPP.
> +	 */
> +	int (*write_sts)(struct mii_bus *bus, int addr, int regnum, u16 val,
> +			 struct ptp_system_timestamp *sts);

I suggest putting C22 in the comment. At some point, somebody might
need C45. And that gets complex because of C45 over C22.

Is there any need for read_sts?

	Andrew