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

[Baolin Wang]

On 25 August 2018 at 04:44, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 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,
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;
-- 
Baolin Wang
Best Regards