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

[Baolin Wang <baolin.wang@linaro.org>]

Hi Jacek,

On 30 August 2018 at 11:26, Baolin Wang <baolin.wang@linaro.org> wrote:
> Hi Jacek,
>
> On 30 August 2018 at 03:15, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>> Hi Baolin,
>>
>> On 08/29/2018 11:48 AM, Baolin Wang wrote:
>>> Hi Jacek,
>>>
>>> On 29 August 2018 at 04:25, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>>>> On 25 August 2018 at 04:44, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
>>>>>> On 08/24/2018 10:12 PM, Pavel Machek wrote:
>>>>>>> On Fri 2018-08-24 21:49:50, Jacek Anaszewski wrote:
>>>>>>>> Hi Pavel,
>>>>>>>>
>>>>>>>> On 08/24/2018 12:11 PM, Pavel Machek wrote:
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>>> I think that it would be more flexible if software pattern fallback
>>>>>>>>>> was applied in case of pattern_set failure. Otherwise, it would
>>>>>>>>>> lead to the situation where LED class devices that support hardware
>>>>>>>>>> blinking couldn't be applied the same set of patterns as LED class
>>>>>>>>>> devices that don't implement pattern_set. The latter will always have to
>>>>>>>>>> resort to using software pattern engine which will accept far greater
>>>>>>>>>> amount of pattern combinations.
>>>>>>>>>>
>>>>>>>>>> In this case we need to discuss on what basis the decision will be
>>>>>>>>>> made on whether hardware or software engine will be used.
>>>>>>>>>>
>>>>>>>>>> Possible options coming to mind:
>>>>>>>>>> - an interface will be provided to determine max difference between
>>>>>>>>>>   the settings supported by the hardware and the settings requested by
>>>>>>>>>>   the user, that will result in aligning user's setting to the hardware
>>>>>>>>>>   capabilities
>>>>>>>>>> - the above alignment rate will be predefined instead
>>>>>>>>>> - hardware engine will be used only if user requests supported settings
>>>>>>>>>>   on the whole span of the requested pattern
>>>>>>>>>> - in each of the above cases it would be worth to think of the
>>>>>>>>>>   interface to show the scope of the settings supported by hardware
>>>>>>>>>
>>>>>>>>> I'd recommend keeping it simple. We use hardware engine if driver
>>>>>>>>> author thinks pattern is "close enough".
>>>>>>>>
>>>>>>>> The thing is that in the ledtrig-pattern v5 implementation there
>>>>>>>> is no option of using software fallback if pattern_set op
>>>>>>>> is initialized:
>>>>>>>>
>>>>>>>> +    if (led_cdev->pattern_set) {
>>>>>>>> +            return led_cdev->pattern_set(led_cdev, data->patterns,
>>>>>>>> +                                         data->npatterns, data->repeat);
>>>>>>>> +    }
>>>>>>>
>>>>>>> Yeah, that sounds wrong. (Sorry I did not pay enough attention).
>>>>>>>
>>>>>>> It pattern_set() returns special error code, it should just continue
>>>>>>> and use software pattern fallback.
>>>>>>
>>>>>> And now we can get back to the issue I was concerned about in the
>>>>>> email you replied to, i.e. what series of [brightness delta_t] tuples
>>>>>> should be written to the pattern file to enable hardware breathing
>>>>>> engine.
>>>>>
>>>>> OK. So now we've made a consensus to use the software pattern fallback
>>>>> if failed to set hardware pattern. How about introduce one interface
>>>>> to convert hardware patterns to software patterns if necessary?
>>>>>
>>>>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
>>>>> b/drivers/leds/trigger/ledtrig-pattern.c
>>>>> index 63b94a2..d46a641 100644
>>>>> --- a/drivers/leds/trigger/ledtrig-pattern.c
>>>>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>>>>> @@ -66,8 +66,15 @@ static int pattern_trig_start_pattern(struct
>>>>> pattern_trig_data *data,
>>>>>                 return 0;
>>>>>
>>>>>         if (led_cdev->pattern_set) {
>>>>> -               return led_cdev->pattern_set(led_cdev, data->patterns,
>>>>> -                                            data->npatterns, data->repeat);
>>>>> +               ret = led_cdev->pattern_set(led_cdev, data->patterns,
>>>>> +                                           data->npatterns, data->repeat);
>>>>> +               if (!ret)
>>>>> +                       return 0;
>>>>> +
>>>>> +               dev_warn(led_cdev->dev, "Failed to set hardware pattern\n");
>>>>> +
>>>>> +               if (led_cdev->pattern_convert)
>>>>> +                       led_cdev->pattern_convert(led_cdev,
>>>>
>>>> I can't see how it could help to assess if hw pattern
>>>> engine can launch given pattern, and with what accuracy.
>>>>
>>>> Instead, I propose to add a means for defining whether the pattern
>>>> to be set is intended for hardware pattern engine or for software
>>>> fallback. It could be either separate sysfs file e.g. hw_pattern,
>>>> or a modifier to the pattern written to the pattern file, e,g,
>>>>
>>>> echo "hw 100 2 200 3 100 2" > pattern
>>>>
>>>> hw format expected by given driver would have to be described
>>>> in the per-driver ABI documentation. All patterns without
>>>> "hw" prefix would enable software pattern engine.
>>>
>>> OK. So I did some optimization based on v5 according to suggestion, if
>>> you think this is okay for you, then I will send out the formal v6
>>> patch set.
>>>
>>> 1. echo "hw 100 2 200 3 100 2" > pattern
>>> Only for hardware pattern, if failed,  we will return error number.
>>
>> After thinking more about it, I'd opt for a separate file
>> dedicated to hardware pattern, e.g. hw_pattern, which would
>> be created only if the LED class driver implemented pattern_set op.
>>
>>> 2. echo "100 2 200 3 100 2" > pattern
>>> Will try to set hardware pattern firstly, if failed, will use software
>>> pattern instead.
>>>
>>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
>>> b/drivers/leds/trigger/ledtrig-pattern.c
>>> index 63b94a2..f08e797 100644
>>> --- a/drivers/leds/trigger/ledtrig-pattern.c
>>> +++ b/drivers/leds/trigger/ledtrig-pattern.c
>>> @@ -26,6 +26,7 @@ struct pattern_trig_data {
>>>       u32 repeat;
>>>       u32 last_repeat;
>>>       bool is_indefinite;
>>> +     bool is_hw_pattern;
>>>       struct timer_list timer;
>>>  };
>>>
>>> @@ -62,12 +63,21 @@ static void pattern_trig_timer_function(struct
>>> timer_list *t)
>>>  static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>>>                                     struct led_classdev *led_cdev)
>>>  {
>>> +     int ret;
>>> +
>>>       if (!data->npatterns)
>>>               return 0;
>>>
>>>       if (led_cdev->pattern_set) {
>>> -             return led_cdev->pattern_set(led_cdev, data->patterns,
>>> +             ret = led_cdev->pattern_set(led_cdev, data->patterns,
>>>                                            data->npatterns, data->repeat);
>>> +             if (!ret)
>>> +                     return 0;
>>> +
>>> +             dev_warn(led_cdev->dev, "Failed to set hardware patterns\n");
>>> +
>>> +             if (data->is_hw_pattern)
>>> +                     return ret;
>>
>> With separate pattern files we could get rid of this ambiguity and
>> attempt calling pattern_set only if requested via hw_pattern file.
>> No software fallback should be employed in case of failure then.
>>
>> Similarly in case of requests originating from pattern file -
>> the software engine should be set up.
>
> Please correct me if I understood you incorrectly.
>
> So we should create the hw_pattern file if the LED class driver
> implemented pattern_set op, then no need use software fallback if it
> failed to set hardware pattern.
>
> If the LED class driver did not implement pattern_set op, we just show
> up pattern file for users, and we always use software pattern now.
>
> But AFAICT the V5 has implemented the logics, but did not change the
> pattern file name according to if LED class driver implemented
> pattern_set op. So now we just change the pattern file to hw_pattern
> file if pattern_set op is set?
>

Sorry for misunderstanding your points, now I realized we will create
2 files for software pattern and hardware pattern if the pattern_set
op was implemented. I submitted v6 patch set according to your
suggestion. Please help to review. Thanks.

-- 
Baolin Wang
Best Regards