(quick reply...) On Fri, Jan 06, 2017 at 02:50:21PM +0100, Jerome Brunet wrote: > So I'm not sure I understand, are you against EEE integration in phylib > entirely, or specifically against the test I added in set_eee to filter > out broken modes ? I'm happy to see EEE integrated into phylib, but I think the current implementation is very buggy and needs a rewrite. > > BTW, one of the problems (not caused by your patch) is that changing > > the EEE advertisment does not (on all PHY drivers) cause the link to > > be renegotiated - there's no call to phy_start_aneg() when the advert > > changes, and even if there was, there's no guarantee that > > phy_start_aneg() will even set the AN restart bit in the control > > register. > > > > However, given that you're hooking into the set_eee function, I'm not > > sure why you placed your EEE advertisment thing into config_aneg() - > > isn't it more an initialisation thing (so should be in > > config_init()?) > > What I change is what the PHY advertise, so it seems logical to do it > where "genphy_config_advert" was called. Just taking the existing code > as an example You need to adjust the adverisment in two places: 1. On initialisation, when you need to change the default value. 2. Whenever the user requests a different EEE advertisment. You don't need to do it each time config_aneg() is called - nothing's going to change the EEE advertisment in that path. Hence, to check it each and every time seems like a waste of CPU cycles. However, there's another path that needs to be considered, which the current EEE code fails to do, and that is the resume path. Nothing at present saves and restores the EEE settings, they are completely lost if the PHY is powered down. This is just another symptom of the current poor quality EEE implementation in phylib, and another reason why I say above that the EEE code is in need of a rewrite... which is something I will be looking at. If the EEE settings are properly saved and restored over suspend/ resume, then the previously programmed EEE advertisment would also be restored. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html