> Let's take a very simple use case: We have a one bit configuration to > switch a LED between link_100 and link_1000 hw trigger mode. > > Then we have the atomicity issue you described: We can't go directly > from one hw-controlled mode to the other, we have to go via both > modes active or no mode active. > > And unfortunately we don't have the option to indicate this by some > optical LED activity like blinking, especially if the link is down > at the moment. > > Would be a pity if our nice framework can't support such a simple > use case. So, what I could imagine, we react based on the return code > from hw_control_is_supported(): > > - 0: use hw control > - -EOPNOTSUPP: fall back to LED software control, no error returned to use > - -ENOTSUPP (another idea: ENOEXEC): store new mode in trigger_data->mode and return error to the user > - other errors: don't store new mode and return error to user > > Not fully intuitive and the subtle difference between EOPNOTSUPP and > ENOTSUPP may confuse driver authors adding device LED support. Using an NFS error code for LEDs will definitely confuse developers. This is not a network file system, where it is valid to use ENOTSUPP. I actually think we need to define some best practices, ordered on what the hardware can do. 1) With software control, set_brightness should do what you expect, not return an error. 2) Without full software control, but there is a mechanism to report a problem, like constant blinking, or off, do that, and return -EOPNOTSUPP. 3) Really dumb hardware like this, set_brightness should be a NULL pointer. The core returns -EOPNOTSUPP. The core should return this -EOPNOTSUPP to user space, but it should accept the configuration change. So the user can put it into an invalid state, in order to get to a valid state with further configuration. I don't see an easy way to let the user know what the valid states are. We currently have a 10bit state. I don't think we can put all the valid ones in a /sysfs file, especially when QCA8K pretty much supports everything. Andrew
