On 05.12.2023 13:40, Heiner Kallweit wrote:
> 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
Patch is so simple that I send it this way. What do you think?
diff --git a/drivers/leds/trigger/ledtrig-netdev.c b/drivers/leds/trigger/ledtrig-netdev.c
index ec0395a6b..a24f3aade 100644
--- a/drivers/leds/trigger/ledtrig-netdev.c
+++ b/drivers/leds/trigger/ledtrig-netdev.c
@@ -310,6 +310,7 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
size_t size, enum led_trigger_netdev_modes attr)
{
struct led_netdev_data *trigger_data = led_trigger_get_drvdata(dev);
+ struct led_classdev *led_cdev = trigger_data->led_cdev;
unsigned long state, mode = trigger_data->mode;
int ret;
int bit;
@@ -349,6 +350,10 @@ static ssize_t netdev_led_attr_store(struct device *dev, const char *buf,
trigger_data->mode = mode;
trigger_data->hw_control = can_hw_control(trigger_data);
+ if (!led_cdev->brightness_set && !led_cdev->brightness_set_blocking &&
+ !trigger_data->hw_control)
+ return -EOPNOTSUPP;
+
set_baseline_state(trigger_data);
return size;
--
2.43.0
