Sorry for the repeated self-replies, I appear to be thinking in very
small increments today :)
On Mon, Jun 30, 2014 at 03:29:57PM -0400, Gabriel L. Somlo wrote:
> > > 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;
> }
> ...
Of course, we could change this to
...
} else {
if (addr < NPHYWRITEOPS && phyreg_writeops[addr]) {
phyreg_writeops[addr](s, index, data);
} else {
s->phy_reg[addr] = data;
}
}
and have phy_set_ctrl() be a proper write_op, expected to actually
write the data as well as do other things. That would at least be a
step toward the principle of least WTF, IMHO :)
I'll send out an updated patch shortly...
Thanks,
--Gabriel
> ...
>
> 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
