Re: [PATCH net-next v2 4/4] net: dp83869: Add RGMII internal delay configuration

[Dan Murphy <dmurphy@ti.com>]

Andrew

On 5/23/20 5:07 PM, Andrew Lunn wrote:
>>> Any why is your PHY special, in that is does care about out of range
>>> delays, when others using new the new core helper don't?
>> We are not rounding to nearest here.  Basically the helper works to find the
>> best match
>>
>> If the delay passed in is less than or equal to the smallest delay then
>> return the smallest delay index
>>
>> If the delay passed in is greater then the largest delay then return the max
>> delay index
> +               /* Find an approximate index by looking up the table */
> +               if (delay > delay_values[i - 1] &&
> +                   delay < delay_values[i]) {
> +                       if (delay - delay_values[i - 1] < delay_values[i] - delay)
> +                               return i - 1;
> +                       else
> +                               return i;
>
> This appears to round to the nearest value when it is not an exact
> match.
>
> The documentation is a hint to the DT developer what value to put in
> DT. By saying it rounders, the developer does not need to go digging
> through the source code to find an exact value, otherwise -EINVAL will
> be returned. They can just use the value the HW engineer suggested,
> and the PHY will pick whatever is nearest.
>
>> Not sure what you mean about this PHY being special.  This helper is
>> not PHY specific.
> As you said, if out of range, the helper returns the top/bottom
> value. Your PHY is special, the top/bottom value is not good enough,
> you throw an error.
>
> The point of helpers is to give uniform behaviour. We have one line
> helpers, simply because they give uniform behaviour, rather than have
> each driver do it subtlety different. But it also means drivers should
> try to not add additional constraints over what the helper already
> has, unless it is actually required by the hardware.
>
>> After I think about this more I am thinking a helper may be over kill here
>> and the delay to setting should be done within the PHY driver itself
> The helper is useful, it will result in uniform handling of rounding
> between DT values and what the PHY can actually do. But please also
> move your range check and error message inside the helper.


I re-worked v3 to be a bit more of a helper and incorporated Florian's 
and you comments

Dan


>       Andrew