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@xxxxxxxxx> wrote:
>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>> 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,
>>
>> 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.
> }
>
> 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
