> > > I agree we need to make compromises. We cannot support every LED > > > feature of every PHY, they are simply too diverse. Hopefully we can > > > support some features of every PHY. In the worst case, a PHY simply > > > cannot be controlled via this method, which is the current state > > > today. So it is not worse off. > > > > ... and that compromise is that it's not going to be possible to enable > > activity mode on 88e151x with how the code stands and with the > > independent nature of "rx" and "tx" activity control currently in the > > netdev trigger... making this whole approach somewhat useless for > > Marvell PHYs. > > Again we can consider adding an activity mode. It seems logical that > some switch may only support global traffic instead of independend tx or > rx... The feature are not mutually exclusive. One include the other 2. Looking at the software trigger, adding NETDEV_LED_RXTX looks simple to do. I also suspect it will be used by more than Marvell. > > We really need to see a working implementation for this code for more > > than just one PHY to prove that it is actually possible for it to > > support other PHYs. If not, it isn't actually solving the problem, > > and we're going to continue getting custom implementations to configure > > the LED settings. > > > > Agree that we need other user for this to catch some problem in the > implementation of this generic API. We need a PHY driver implementation. The phylib core needs to be involved, the cled code needs to call generic phylib functions which take the phydev->lock before calling into the PHY driver. Probably the phylib core can do all the memory allocation, and registration of the LED to the LED core. If it is not too ugly, i would also do the DT binding parsing in the core, so we don't end up with subtle differences. Andrew
