On 05.12.2023 04:00, Andrew Lunn wrote: >> 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. > Sounds good to me. Let me come up with a RFC patch. > 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 Heiner
