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.
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;
}
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
