On 06.12.2023 18:45, Andrew Lunn wrote: >>>> 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 >> >> Patch is so simple that I send it this way. What do you think? > > That is simpler than i expected. > > But i think we need to document our expectations. What do we expect an > LED driver to do when it cannot support software blinking. So please > could you add a comment somewhere. Maybe extend the > > /* > *Configurable sysfs attributes: > * > > section? > Yes, this is a good place IMO. I'll submit a patch including the functional change and the extended comment. > Thanks > Andrew Heiner
