On Mon, Jun 30, 2014 at 02:12:51PM -0400, Gabriel L. Somlo wrote:
> On Mon, Jun 30, 2014 at 09:04:12PM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 30, 2014 at 12:55:49PM -0400, Gabriel L. Somlo wrote:
> > > set_phy_ctrl() runs the same autonegotiation availability checks
> > > that were factored out into have_autoneg() in an earlier commit
> > > (d7a4155265416a1c8f3067b59e68bf5fda1d6215), except it runs them
> > > on the value about to be written into the phy_ctrl register, and
> > > then does not actually write the register.
> > >
> > > This patch moves the have_autoneg() function in front of
> > > set_phy_ctrl(), then updates the latter to actually write
> > > the register with the given value, and use the factored-out
> > > check to determine whether the link should be bounced.
> > >
> > > Also, fix indentation/line width when setting up the timer.
> > >
> > > Signed-off-by: Gabriel Somlo <so...@cmu.edu>
> >
> > Weird.
> > So does this mean have_autoneg is always false then?
> > How does it work?
Oh, I think I know what's going on now:
In set_mdic() we have:
...
} else {
if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
phyreg_writeops[addr](s, index, data);
}
s->phy_reg[addr] = data;
}
...
Basically, phyreg_writeops[PHY_CTRL] == set_phy_ctrl() is the only
one that applies, but it's like a "pre-write" writeop, the write
happens *after* the write_op completes :) :) Fun stuff...
Leaving it as-is is probably OK, although it does indeed not handle
SC bits properly.
I could rewrite have_autoneg() to accept the *value* of the phy control
register, then we can use it from set_phy_ctrl() before the register
is actually written. Not sure about whether that would be more or less
ugly than the current form :)
Thanks,
--Gabriel
