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" > 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? > > 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. > > 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. -- Baolin Wang Best Regards
