Hi Andrew,
Thanks for your thorough review, much appreciated.
I agree with all suggestions except one and will fix those for v2.
On 2024-04-16 00:55:12 +0200, Andrew Lunn wrote:
<snip>
> > +static void rtsn_set_delay_mode(struct rtsn_private *priv)
> > +{
> > + struct device_node *np = priv->ndev->dev.parent->of_node;
> > + u32 delay;
> > + u32 val;
> > +
> > + val = 0;
> > +
> > + /* Valid values are 0 and 1800, according to DT bindings */
>
> The bindings should not matter. It is what the hardware supports. The
> bindings should match the hardware, since it is hard to modify the
> hardware to make it match the binding.
I agree the comment could be improved. It should likely point to the
datasheet instead. See below for why.
>
> > + if (!of_property_read_u32(np, "rx-internal-delay-ps", &delay))
> > + if (delay)
> > + val |= GPOUT_RDM;
> > +
> > + /* Valid values are 0 and 2000, according to DT bindings */
> > + if (!of_property_read_u32(np, "tx-internal-delay-ps", &delay))
> > + if (delay)
> > + val |= GPOUT_TDM;
> > +
> > + rtsn_write(priv, GPOUT, val);
>
> So you seem to be using it as bool?
Yes.
> That is wrong. It is a number of pico seconds!
The issue is that the hardware only supports no delay or a fixed delay
that can depend on electric properties of the board. The datasheets
states that the typical Rx delay is 1800 ps while the typical Tx delay
is 2000 ps. The hardware register implementation for this is a single
bit for each delay, on or off.
To model this in the bindings after some discussions [1] the standard
property was picked over a vendor specific bool variant of it. Here in
the driver I tried to document that the binding will enforce the value
to either be 0 or {1800,2000}, but that for the hardware it should be
treated as a on/off switch.
<snip>
1. https://lore.kernel.org/linux-renesas-soc/ZVzbigCtv2q_2-Bx@xxxxxxxxxxxxxxxxx/
--
Kind Regards,
Niklas Söderlund
