Re: [PATCH v12 1/2] leds: core: Introduce LED pattern trigger

[Baolin Wang]

On 26 September 2018 at 04:00, Jacek Anaszewski
<jacek.anaszewski@xxxxxxxxx> wrote:
> Hi Baolin,
>
> On 09/25/2018 01:15 PM, Baolin Wang wrote:
>> On 23 September 2018 at 20:25, Jacek Anaszewski
>> <jacek.anaszewski@xxxxxxxxx> wrote:
>>> On 09/22/2018 09:44 PM, Pavel Machek wrote:
>>>> On Sat 2018-09-22 00:18:13, Pavel Machek wrote:
>>>>> On Sat 2018-09-22 00:11:29, Jacek Anaszewski wrote:
>>>>>> On 09/21/2018 11:17 PM, Pavel Machek wrote:
>>>>>>> On Fri 2018-09-21 22:59:40, Jacek Anaszewski wrote:
>>>>>>>> Hi Baolin,
>>>>>>>>
>>>>>>>> On 09/21/2018 05:31 AM, Baolin Wang wrote:
>>>>>>>>> Hi Jacek and Pavel,
>>>>>>>>>
>>>>>>>>> On 11 September 2018 at 10:47, Baolin Wang <baolin.wang@xxxxxxxxxx> wrote:
>>>>>>>>>> This patch adds one new led trigger that LED device can configure
>>>>>>>>>> the software or hardware pattern and trigger it.
>>>>>>>>>>
>>>>>>>>>> Consumers can write 'pattern' file to enable the software pattern
>>>>>>>>>> which alters the brightness for the specified duration with one
>>>>>>>>>> software timer.
>>>>>>>>>>
>>>>>>>>>> Moreover consumers can write 'hw_pattern' file to enable the hardware
>>>>>>>>>> pattern for some LED controllers which can autonomously control
>>>>>>>>>> brightness over time, according to some preprogrammed hardware
>>>>>>>>>> patterns.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Raphael Teysseyre <rteysseyre@xxxxxxxxx>
>>>>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>>>>>
>>>>>>>>> Do you have any comments for the v12 patch set? Thanks.
>>>>>>>>
>>>>>>>> We will probably have to remove hw_pattern from ledtrig-pattern
>>>>>>>> since we are unable to come up with generic interface for it.
>>>>>>>> Unless thread [0] will end up with some brilliant ideas. So far
>>>>>>>> we're waiting for Pavel's reply.
>>>>>>>>
>>>>>>>> [0] https://lkml.org/lkml/2018/9/13/1216
>>>>>>>
>>>>>>> To paint a picture:
>>>>>>>
>>>>>>> brightness
>>>>>>>
>>>>>>>    rise hold          lower       hold down
>>>>>>>   ^    XXXXXXXXXXXXXXX
>>>>>>>   |   X               XX
>>>>>>>   |  X                  XX
>>>>>>>   | X                     XXXXXXXXXXXXXXXXXXXXXXXXXX
>>>>>>>   +-------------------------------------------------------> time
>>>>>>>
>>>>>>> This is what Baolin's hardware can do, right?
>>>>>>>
>>>>>>> This is also what pattern trigger can do, right?
>>>>>>>
>>>>>>> So all we need to do is match the two interfaces, so that hw_pattern
>>>>>>> returns -EINVAL on patterns hardware can not actually do.
>>>>>>>
>>>>>>> I believe I described code to do that in [0] above.
>>>>>>
>>>>>> You said that we should get the same effect by writing the
>>>>>> same series of tuples to either pattern or hw_pattern file.
>>>>>>
>>>>>> Below command consists of four tuples (marked with brackets
>>>>>> to highlight), and it will activate breathing mode in Baolin's
>>>>>> hw_pattern:
>>>>>>
>>>>>> "[0 rise_duration] [brightness high_duration] [brightness fall_duration]
>>>>>> [0   low_duration]"
>>>>>>
>>>>>> Now, I can't see how these four tuples could force the software
>>>>>> fallback to produce breathing effect you depicted.
>>>>>
>>>>> I really should get some sleep now. But my intention was that software
>>>>> fallback produces just that with those four tuples. (If it does not,
>>>>> we can fix the software fallback to do just that).
>>>>
>>>> And you are right, v12 1/2 seems to do the wrong thing.
>>>>
>>>> My "brilliant idea" is to something closer to the original version I
>>>> posted here. I'm attaching it for reference.
>>>>
>>>> I'm also attaching the original documentation. It was clearly designed
>>>> to do smooth transitions, too. (But pattern is written in slightly
>>>> different way there, AFAICT).
>>>>
>>>> Clearly, having same semantics for pattern and hw_pattern is possible.
>>>
>>> Thank you for the attachment. The documentation part makes everything
>>> clear. Comparing the patch from the attachment and the Baolin's patch
>>> there is one vital part missing, from the original
>>> pattern_trig_update():
>>>
>>> if (data->next->brightness == data->curr->brightness) {
>>> [...]
>>> } else {
>>>         /* Gradual dimming */
>>>         led_set_brightness(data->led_cdev, compute_brightness(data));
>>>         data->delta_t += UPDATE_INTERVAL;
>>>         mod_timer(&data->timer, jiffies
>>>                 + msecs_to_jiffies(UPDATE_INTERVAL));
>>> }
>>
>> I have some confusions about this, if data->next->brightness !=
>> data->curr->brightness, we should always do gradual dimming? But
>> suppose below pattan values:
>> echo "0 100 20 100 40 100 80 100 100 100" > patter
> ledtrig-pattern.txt attached by Pavel explains this:
>
> "To make the LED go instantly from one brightness value to another,
> use zero-time lengths."
>
> Actually you have it also:
> "Duration of 0 means brightness should immediately change to new value"
>
> According to this, to get instant changes your pattern should look
> like this:
>
> echo "0 100 0 0 20 100 20 0 40 100 40 0 80 100 80 0 100 100" > pattern

Make sense. Thanks for your explanation.

>
>> I do not want to do gradual dimming, just change the brightness with duration.
>>
>>> And the compute_brightness() implementation:
>>>
>>> static int compute_brightness(struct pattern_trig_data *data)
>>> {
>>>         if (data->delta_t == 0)
>>>                 return data->curr->brightness;
>>>
>>>         if (data->curr->delta_t == 0)
>>>                 return data->next->brightness;
>>>
>>>         return data->curr->brightness + data->delta_t
>>>                 * (data->next->brightness - data->curr->brightness)
>>>                 / data->curr->delta_t;
>>> }
>>>
>>> With the above the linear gradual dimming is indeed feasible.
>>> And for non-linear dimming like breathing mode the hw_pattern will do.
>>
>> I am still not sure when we need the linear gradual dimming? When
>> failed to set hardware pattern?
>
> No. Linear gradual dimming needs to be applied only if set via pattern
> file. In this case we shouldn't attempt to call pattern_set at all,
> since we know that we are asked for enabling software engine.
>
> Similarly, in the hw_pattern handler we should call only pattern_set,
> and return error code, without falling back to the software engine
> in case of error.

OK. Thanks.

-- 
Baolin Wang
Best Regards