On 04/04/2017 10:53 PM, Benjamin Herrenschmidt wrote: > On Tue, 2017-04-04 at 21:21 -0700, Florian Fainelli wrote: >> >> This looks pretty good to me, two minor things: >> >> - most drivers keep track of the old status/duplex/pause/link variables >> instead of the current one which is already available within struct >> phy_device, any particular reason for not doing like the other drivers? > > We don't necessarily have a phydev attached when using NC-SI, so it was > easier to have the core code path not have to go fishing for those > settings in different places based on whether we're using NC-SI or not.
Oh right, I missed that part. Is there a reason why NC-SI does not have a PHY device attached? If not, could you somehow model the link using a fixed PHY (which appears to Linux as a normal phy_device) just to keep things simple. > >> - the need to reset the HW during link changes is just ... well too bad > > Yup but there's little choice. The HW wants it. I don't see any real > point in optimizing that path mind you. Losing a few packets around > a link change isn't going to hurt and it keeps the code a lot simpler > by having a single "re-init" path. I was just merely trying to say nicely: what a nicely broken piece of HW (there were other adjectives coming to mind), and I do understand the pain. > >> With that: >> >>> Reviewed-by: Florian Fainelli <f.faine...@gmail.com> > > Thanks ! > > I'll post batch 2 in the next couple of days which tackles the RX path. Cool, looking forward to that! -- Florian
