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

[Jacek Anaszewski]

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

> 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.

>> There is also vital discrepancy between the documentation and the
>> proposed ledtrig-pattern implementation. The doc says:
>>
>> "Duration of 0 means brightness should immediately change to new value"
> 
> OK. Will add in next version.

You already have it added.

>> This syntax seems to be not supported in the Baolin's patch.
>>
>> With all the above covered we will be almost there.
>>
>> Now, only the issues raised by Bjorn need a clarification:
>>
>> On 09/08/2018 07:02 AM, Bjorn Andersson wrote:
>> [...]
>>> The controls for my hardware is:
>>> * a list of brightness values
>>> * the rate of the pattern
>>
>> The two can be described using [brightness delta_t] tuples.
>>
>>> * a flag to indicate that the pattern should be played from start
>>>   to end, end to start or start to end to start
>>
>> As above, but the tuples would have to be suitably arranged.
>>
>> We won't need any specific flag to indicate how the pattern
>> should be played, but instead explicitly give the tuples in the
>> required order. For the start-end-start case the sequence of tuples
>> will need to be tripled, but the middle part should be put in the
>> reverse order.
>>
>>> * a boolean indicating if the pattern should be played once or repeated
>>>   indefinitely.
>>
>> We will have "repeat" file for that.
>>
>> Baolin, would you mind adding the support for gradual dimming to your
>> ledtrig-timer.c implementation?
> 
> Yes, I will do. Thanks.
> 

-- 
Best regards,
Jacek Anaszewski