Message-ID: <CAMz4kuKT=zcH_Kjc5FnvX98vw-5aNQMxMTs4aeqqiM6F4QGEuA@mail.gmail.com>
Date:   Thu, 30 Aug 2018 11:26:44 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>
Cc:     Pavel Machek <pavel@....cz>, rteysseyre@...il.com,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Mark Brown <broonie@...nel.org>,
        Linux LED Subsystem <linux-leds@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger

Hi Jacek,

On 30 August 2018 at 03:15, Jacek Anaszewski <jacek.anaszewski@...il.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@...il.com> wrote:
>>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>>> On 25 August 2018 at 04:44, Jacek Anaszewski <jacek.anaszewski@...il.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?

>
>>       }
>>
>>       data->curr = data->patterns;
>> @@ -106,7 +116,7 @@ static ssize_t repeat_store(struct device *dev,
>> struct device_attribute *attr,
>>       if (err)
>>               return err;
>>
>> -     if (!led_cdev->pattern_set)
>> +     if (!data->is_hw_pattern)
>>               del_timer_sync(&data->timer);
>>
>>       mutex_lock(&data->lock);
>> @@ -159,19 +169,32 @@ static ssize_t pattern_store(struct device *dev,
>> struct device_attribute *attr,
>>  {
>>       struct led_classdev *led_cdev = dev_get_drvdata(dev);
>>       struct pattern_trig_data *data = led_cdev->trigger_data;
>> -     int ccount, cr, offset = 0, err = 0;
>> +     int ccount, cr, len, offset = 0, err = 0;
>> +     const char *s;
>>
>> -     if (!led_cdev->pattern_set)
>> +     if (!data->is_hw_pattern)
>>               del_timer_sync(&data->timer);
>>
>>       mutex_lock(&data->lock);
>>
>> +     s = strstr(buf, "hw");
>> +     if (s && led_cdev->pattern_set) {
>> +             data->is_hw_pattern = true;
>> +             s += strlen("hw");
>> +             len = strlen(s);
>> +     } else {
>> +             data->is_hw_pattern = false;
>> +             s = buf;
>> +             len = count;
>> +     }
>> +
>>       data->npatterns = 0;
>> -     while (offset < count - 1 && data->npatterns < MAX_PATTERNS) {
>> +     while (offset < len - 1 && data->npatterns < MAX_PATTERNS) {
>>               cr = 0;
>> -             ccount = sscanf(buf + offset, "%d %d %n",
>> +             ccount = sscanf(s + offset, "%d %d %n",
>>                               &data->patterns[data->npatterns].brightness,
>>                               &data->patterns[data->npatterns].delta_t, &cr);
>>               if (ccount != 2) {
>>                       data->npatterns = 0;
>>                       err = -EINVAL;
>> @@ -232,7 +255,8 @@ static void pattern_trig_deactivate(struct
>> led_classdev *led_cdev)
>>
>>       if (led_cdev->pattern_clear)
>>               led_cdev->pattern_clear(led_cdev);
>> -     else
>> +
>> +     if (!data->is_hw_pattern)
>>               del_timer_sync(&data->timer);
>>
>>       led_set_brightness(led_cdev, LED_OFF);
>>
>>>
>>>> data->patterns, &data->npatterns);
>>>>         }
>>>>
>>>>         data->curr = data->patterns;
>>>> @@ -106,8 +113,7 @@ static ssize_t repeat_store(struct device *dev,
>>>> struct device_attribute *attr,
>>>>         if (err)
>>>>                 return err;
>>>>
>>>> -       if (!led_cdev->pattern_set)
>>>> -               del_timer_sync(&data->timer);
>>>> +       del_timer_sync(&data->timer);
>>>>
>>>>         mutex_lock(&data->lock);
>>>>
>>>> @@ -161,8 +167,7 @@ static ssize_t pattern_store(struct device *dev,
>>>> struct device_attribute *attr,
>>>>         struct pattern_trig_data *data = led_cdev->trigger_data;
>>>>         int ccount, cr, offset = 0, err = 0;
>>>>
>>>> -       if (!led_cdev->pattern_set)
>>>> -               del_timer_sync(&data->timer);
>>>> +       del_timer_sync(&data->timer);
>>>>
>>>>         mutex_lock(&data->lock);
>>>>
>>>> @@ -232,9 +237,8 @@ static void pattern_trig_deactivate(struct
>>>> led_classdev *led_cdev)
>>>>
>>>>         if (led_cdev->pattern_clear)
>>>>                 led_cdev->pattern_clear(led_cdev);
>>>> -       else
>>>> -               del_timer_sync(&data->timer);
>>>>
>>>> +       del_timer_sync(&data->timer);
>>>>         led_set_brightness(led_cdev, LED_OFF);
>>>>         kfree(data);
>>>>         led_cdev->activated = false;
>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>>>> index 74fc2c6..04f3eaf 100644
>>>> --- a/include/linux/leds.h
>>>> +++ b/include/linux/leds.h
>>>> @@ -93,6 +93,8 @@ struct led_classdev {
>>>>                            struct led_pattern *pattern, int len,
>>>>                            unsigned int repeat);
>>>>         int (*pattern_clear)(struct led_classdev *led_cdev);
>>>> +       int (*pattern_convert)(struct led_classdev *led_cdev,
>>>> +                              struct led_pattern *pattern, int *len);
>>>>
>>>>         struct device           *dev;
>>>>         const struct attribute_group    **groups;
>>>>
>>>
>
>
> --
> Best regards,
> Jacek Anaszewski



-- 
Baolin Wang
Best Regards

Powered by blists - more mailing lists