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
