On Tue, Nov 09, 2021 at 10:09:40PM +0100, Andrew Lunn wrote: > > > +/* Expose sysfs for every blink to be configurable from userspace */ > > > +DEFINE_OFFLOAD_TRIGGER(blink_tx, BLINK_TX); > > > +DEFINE_OFFLOAD_TRIGGER(blink_rx, BLINK_RX); > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_10m, KEEP_LINK_10M); > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_100m, KEEP_LINK_100M); > > > +DEFINE_OFFLOAD_TRIGGER(keep_link_1000m, KEEP_LINK_1000M); > > You might get warnings about CamelCase, but i suggest keep_link_10M, > keep_link_100M and keep_link_1000M. These are megabits, not millibits. > > > > +DEFINE_OFFLOAD_TRIGGER(keep_half_duplex, KEEP_HALF_DUPLEX); > > > +DEFINE_OFFLOAD_TRIGGER(keep_full_duplex, KEEP_FULL_DUPLEX); > > What does keep mean in this context? > LED is turned on but doesn't blink. Hint for a better name? > > > +DEFINE_OFFLOAD_TRIGGER(option_linkup_over, OPTION_LINKUP_OVER); > > > +DEFINE_OFFLOAD_TRIGGER(option_power_on_reset, OPTION_POWER_ON_RESET); > > > +DEFINE_OFFLOAD_TRIGGER(option_blink_2hz, OPTION_BLINK_2HZ); > > > +DEFINE_OFFLOAD_TRIGGER(option_blink_4hz, OPTION_BLINK_4HZ); > > > +DEFINE_OFFLOAD_TRIGGER(option_blink_8hz, OPTION_BLINK_8HZ); > > > > This is very strange. Is option_blink_2hz a trigger on itself? Or just > > an option for another trigger? It seems that it is an option, so that I > > can set something like > > blink_tx,option_blink_2hz > > and the LED will blink on tx activity with frequency 2 Hz... If that is > > so, I think you are misnaming your macros or something, since you are > > defining option_blink_2hz as a trigger with > > DEFINE_OFFLOAD_TRIGGER > > Yes, i already said this needs handling differently. The 2Hz, 4Hz and > 8Hz naturally fit the delay_on, delay_of sysfs attributes. > > Andrew You are totally right. I tought the blink control was something specific that only some PHY had, but considering we have more than 1 that supports a variant of it, I can see how it should be handled separately. I assume even more have this kind of customizzation. -- Ansuel
