On 11/11/2018 08:53 PM, Pavel Machek wrote:
> On Sun 2018-11-11 18:00:54, Uwe Kleine-König wrote:
>> On Sun, Nov 11, 2018 at 12:30:39PM +0100, Pavel Machek wrote:
>>> Hi!
>>>
>>>> This is only a proof of concept driver that makes an LED blink on UART
>>>> activity. Feedback about the concept is very welcome.
>>>
>>>> +static void ledtrig_uart_halt(struct ledtrig_uart_data *trigger_data)
>>>> +{
>>>> + cancel_delayed_work_sync(&trigger_data->dwork);
>>>> +}
>>>> +
>>>> +static void ledtrig_uart_restart(struct ledtrig_uart_data *trigger_data)
>>>> +{
>>>> + pr_info("%s:%d: devname = %s\n", __func__, __LINE__, trigger_data->devname);
>>>> + if (!trigger_data->devname)
>>>> + return;
>>>> +
>>>> + schedule_delayed_work(&trigger_data->dwork, 0);
>>>> +}
>>>
>>> You should not need to do delayed work by hand.
>>
>> What is the obvious alternative I missed then? This is taken from the
>> network trigger which then might benefit from your suggestion, too.
>
> Can you take a look at disk and CPU triggers?
>
> /**
> * led_set_brightness - set LED brightness
> * @led_cdev: the LED to set
> * @brightness: the brightness to set it to
> *
> * Set an LED's brightness, and, if necessary, cancel the
> * software blink timer that implements blinking when the
> * hardware doesn't. This function is guaranteed not to sleep.
> */
>
> set_brigthness guarantees not to sleep, so there really should not be
> need to do workqueues by hand.
IMHO use of delayed work is correct here - it serves for launching
ledtrig_uart_work() with reasonable rate (here 1s), which needs to
update port->icount. This is unrelated to the LED core workqueue which
is used for drivers that implement brightness_set_blocking() op.
--
Best regards,
Jacek Anaszewski
