Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 30 August 2018 at 11:26, Baolin Wang wrote:
> Hi Jacek,
>
> On 30 August 2018 at 03:15, Jacek Anaszewski
> wrote:
>> Hi Baolin,
>>
>> On 08/29/2018 11:48 AM, Baolin Wang wrote:
>>> Hi Jacek,
>>>
>>> On 29 August 2018 at 04:25, Jacek Anaszewski
>>> wrote:
On 08/25/2018 09:51 AM, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 30 August 2018 at 11:26, Baolin Wang wrote:
> Hi Jacek,
>
> On 30 August 2018 at 03:15, Jacek Anaszewski
> wrote:
>> Hi Baolin,
>>
>> On 08/29/2018 11:48 AM, Baolin Wang wrote:
>>> Hi Jacek,
>>>
>>> On 29 August 2018 at 04:25, Jacek Anaszewski
>>> wrote:
On 08/25/2018 09:51 AM, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 30 August 2018 at 03:15, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/29/2018 11:48 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 29 August 2018 at 04:25, Jacek Anaszewski
>> wrote:
>>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
On 25 August 2018 at 04:44, Jacek Anaszewski
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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 30 August 2018 at 03:15, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/29/2018 11:48 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 29 August 2018 at 04:25, Jacek Anaszewski
>> wrote:
>>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
On 25 August 2018 at 04:44, Jacek Anaszewski
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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/29/2018 11:48 AM, Baolin Wang wrote:
> Hi Jacek,
>
> On 29 August 2018 at 04:25, Jacek Anaszewski
> wrote:
>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>> On 25 August 2018 at 04:44, Jacek Anaszewski
>>> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/29/2018 11:48 AM, Baolin Wang wrote:
> Hi Jacek,
>
> On 29 August 2018 at 04:25, Jacek Anaszewski
> wrote:
>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>> On 25 August 2018 at 04:44, Jacek Anaszewski
>>> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 08/28/2018 11:13 PM, Bjorn Andersson wrote:
> On Tue 28 Aug 13:25 PDT 2018, Jacek Anaszewski wrote:
>
>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>> On 25 August 2018 at 04:44, Jacek Anaszewski
>>> 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.
>>
>
> We started this discussion with the suggestion that rather than
> introducing a new Qualcomm specific sysfs interface for controlling the
> pattern engine we should have a common one, but if I understand your
> suggestion we should not have a common interface, just a common sysfs
> file name?
True, when it is related to hardware pattern. Software pattern would
have common both file and interface. Alternatively, we could have two
separate files for
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 08/28/2018 11:13 PM, Bjorn Andersson wrote:
> On Tue 28 Aug 13:25 PDT 2018, Jacek Anaszewski wrote:
>
>> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>>> On 25 August 2018 at 04:44, Jacek Anaszewski
>>> 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.
>>
>
> We started this discussion with the suggestion that rather than
> introducing a new Qualcomm specific sysfs interface for controlling the
> pattern engine we should have a common one, but if I understand your
> suggestion we should not have a common interface, just a common sysfs
> file name?
True, when it is related to hardware pattern. Software pattern would
have common both file and interface. Alternatively, we could have two
separate files for
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 29 August 2018 at 04:25, Jacek Anaszewski wrote:
> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>> On 25 August 2018 at 04:44, Jacek Anaszewski
>> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 29 August 2018 at 04:25, Jacek Anaszewski wrote:
> On 08/25/2018 09:51 AM, Baolin Wang wrote:
>> On 25 August 2018 at 04:44, Jacek Anaszewski
>> 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
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On Tue 28 Aug 13:25 PDT 2018, Jacek Anaszewski wrote:
> On 08/25/2018 09:51 AM, Baolin Wang wrote:
> > On 25 August 2018 at 04:44, Jacek Anaszewski
> > 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.
>
We started this discussion with the suggestion that rather than
introducing a new Qualcomm specific sysfs interface for controlling the
pattern engine we should have a common one, but if I understand your
suggestion we should not have a common interface, just a common sysfs
file name?
Regards,
Bjorn
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On Tue 28 Aug 13:25 PDT 2018, Jacek Anaszewski wrote:
> On 08/25/2018 09:51 AM, Baolin Wang wrote:
> > On 25 August 2018 at 04:44, Jacek Anaszewski
> > 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.
>
We started this discussion with the suggestion that rather than
introducing a new Qualcomm specific sysfs interface for controlling the
pattern engine we should have a common one, but if I understand your
suggestion we should not have a common interface, just a common sysfs
file name?
Regards,
Bjorn
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On Sat 25 Aug 00:51 PDT 2018, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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:
[..]
> >>> +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?
>
I do not agree that a silent fallback mechanism is the way to go here.
There is a enormous difference in power consumption between letting a
dedicated piece of hardware drive a pulse on a LED or using a general
purpose ARM core running at hundreds of MHz.
Note that the most common use case for the driver I'm trying to upstream
is to pulse the LED when you have a notification pending on your Android
device, in which case you want all the CPUs are completely powered down.
Regards,
Bjorn
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On Sat 25 Aug 00:51 PDT 2018, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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:
[..]
> >>> +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?
>
I do not agree that a silent fallback mechanism is the way to go here.
There is a enormous difference in power consumption between letting a
dedicated piece of hardware drive a pulse on a LED or using a general
purpose ARM core running at hundreds of MHz.
Note that the most common use case for the driver I'm trying to upstream
is to pulse the LED when you have a notification pending on your Android
device, in which case you want all the CPUs are completely powered down.
Regards,
Bjorn
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 08/25/2018 09:51 AM, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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.
> data->patterns, >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(>timer);
> + del_timer_sync(>timer);
>
> mutex_lock(>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(>timer);
> + del_timer_sync(>timer);
>
> mutex_lock(>lock);
>
> @@ -232,9
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 08/25/2018 09:51 AM, Baolin Wang wrote:
> On 25 August 2018 at 04:44, Jacek Anaszewski
> 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.
> data->patterns, >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(>timer);
> + del_timer_sync(>timer);
>
> mutex_lock(>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(>timer);
> + del_timer_sync(>timer);
>
> mutex_lock(>lock);
>
> @@ -232,9
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 25 August 2018 at 04:44, Jacek Anaszewski 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, >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(>timer);
+ del_timer_sync(>timer);
mutex_lock(>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(>timer);
+ del_timer_sync(>timer);
mutex_lock(>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(>timer);
+ del_timer_sync(>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,
+
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
On 25 August 2018 at 04:44, Jacek Anaszewski 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, >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(>timer);
+ del_timer_sync(>timer);
mutex_lock(>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(>timer);
+ del_timer_sync(>timer);
mutex_lock(>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(>timer);
+ del_timer_sync(>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,
+
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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);
+ }
> If human can not tell the difference, it probably is.
>
> We may want to do something more formal later.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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);
+ }
> If human can not tell the difference, it probably is.
>
> We may want to do something more formal later.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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". If human can not tell the difference, it probably is. We may want to do something more formal later. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
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". If human can not tell the difference, it probably is. We may want to do something more formal later. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 11 August 2018 at 02:10, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/10/2018 05:26 PM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 9 August 2018 at 21:21, Jacek Anaszewski
>> wrote:
>>> Hi Baolin,
>>>
>>> On 08/09/2018 07:48 AM, Baolin Wang wrote:
>>> [...]
+static int pattern_trig_start_pattern(struct pattern_trig_data *data,
+ struct led_classdev *led_cdev)
+{
+ if (!data->npatterns)
+ return 0;
+
+ if (led_cdev->pattern_set) {
+ return led_cdev->pattern_set(led_cdev, data->patterns,
+ data->npatterns,
data->repeat);
>>>
>>> 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.
>>
>> Hmmm, I am not sure this is useful for hardware pattern, since the LED
>> hardware can be diverse which is hard to be compatible with software
>> pattern.
>>
>> For example, for our SC27XX LED, it only supports 4 hardware patterns
>> setting (low time, rising time, high time and falling time) to make it
>> work at breathing mode. If user provides 3 or 5 patterns values, it
>> will failed to issue pattern_set(). But at this time, we don't want to
>> use software pattern instead since it will be not the breathing mode
>> expected by user. I don't know if there are other special LED
>> hardware.
>
> Good point. So this is the issue we should dwell on, since the
> requested pattern effect should look similar on all devices.
> Of course in case of software pattern it will be often impossible
> to achieve the same fluency. Similarly as in case of rendering graphics
> with and without acceleration.
>
> In case of your device, I'd say that we'd need more complex description
> of breathing mode pattern. More complex than just four intervals.
> It should reflect all the intervals the hardware engine needs to perform
> to achieve the breathing effect. Can this information be inferred from
> the docs?
>From our docs, it only provides registers to set the low time, rising
time, high time and falling time (value unit is 0.125s and max value
is 63 = 7.875s) to enable breathing mode. So each interval value can
be 1 ~ 63. But that is still hard for software pattern to simulate the
breathing mode performed by hardware pattern.
>>>
>>> Software fallback is not expected to show similar performance to the
>>> hardware engine on the whole span of the supported time range.
>>>
>>> Having min rise time value at 125ms, and given that max_brightness
>>> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
>>> 1ms. So, the pattern for rising edge could look like (assuming we
>>> stop at 254):
>>>
>>> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
>>
>> Right, maybe this can work, maybe not. Since we can met different
>> cases when failed to issue pattern_set(). Maybe the LED hardware
>> occurs some errors, in this case we should shutdown the LED, not
>> enable the software pattern instead, which still can not work.
>
> This is arguable. Timer trigger always resorts to using software
> fallback if blink_set() fails.
Timer trigger is simple which only need delay_on and delay_off, that
means software can be easy to show the same performance. But hardware
pattern can be diverse, we need to convert hardware patterns to
software patterns.
>> Maybe
>> driver can set NULL for pattern_set/clear interfaces to disable
>> hardware pattern, then next time user will perform the patterns with
>> software pattern mode.
>
> Resetting any ops after LED class driver is probed should be
> deemed forbidden, since LED core can make some decisions basing on
> whether given ops are initialized or not.
OK.
>
>> For our SC27XX LED, like I said if user provides only 3 pattern values
>> which will failed to issue pattern_set(). But I still do not need use
>> software patterns to show similar performance, instead it will still
>> keep the last hardware pattern performance ( It did not overlap the
>> previous hardware pattern setting). Maybe different drivers have
>> different error handling, that's why I think we can leave driver to
>> choose the proper way to handle.
>
> ABI semantics is generic, i.e. common for all drivers. Driver can
> always log any problems occurred while setting pattern, but it shouldn't
> necessarily need to prevent pattern trigger from using software
> fallback.
Fine.
>>
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 11 August 2018 at 02:10, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/10/2018 05:26 PM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 9 August 2018 at 21:21, Jacek Anaszewski
>> wrote:
>>> Hi Baolin,
>>>
>>> On 08/09/2018 07:48 AM, Baolin Wang wrote:
>>> [...]
+static int pattern_trig_start_pattern(struct pattern_trig_data *data,
+ struct led_classdev *led_cdev)
+{
+ if (!data->npatterns)
+ return 0;
+
+ if (led_cdev->pattern_set) {
+ return led_cdev->pattern_set(led_cdev, data->patterns,
+ data->npatterns,
data->repeat);
>>>
>>> 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.
>>
>> Hmmm, I am not sure this is useful for hardware pattern, since the LED
>> hardware can be diverse which is hard to be compatible with software
>> pattern.
>>
>> For example, for our SC27XX LED, it only supports 4 hardware patterns
>> setting (low time, rising time, high time and falling time) to make it
>> work at breathing mode. If user provides 3 or 5 patterns values, it
>> will failed to issue pattern_set(). But at this time, we don't want to
>> use software pattern instead since it will be not the breathing mode
>> expected by user. I don't know if there are other special LED
>> hardware.
>
> Good point. So this is the issue we should dwell on, since the
> requested pattern effect should look similar on all devices.
> Of course in case of software pattern it will be often impossible
> to achieve the same fluency. Similarly as in case of rendering graphics
> with and without acceleration.
>
> In case of your device, I'd say that we'd need more complex description
> of breathing mode pattern. More complex than just four intervals.
> It should reflect all the intervals the hardware engine needs to perform
> to achieve the breathing effect. Can this information be inferred from
> the docs?
>From our docs, it only provides registers to set the low time, rising
time, high time and falling time (value unit is 0.125s and max value
is 63 = 7.875s) to enable breathing mode. So each interval value can
be 1 ~ 63. But that is still hard for software pattern to simulate the
breathing mode performed by hardware pattern.
>>>
>>> Software fallback is not expected to show similar performance to the
>>> hardware engine on the whole span of the supported time range.
>>>
>>> Having min rise time value at 125ms, and given that max_brightness
>>> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
>>> 1ms. So, the pattern for rising edge could look like (assuming we
>>> stop at 254):
>>>
>>> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
>>
>> Right, maybe this can work, maybe not. Since we can met different
>> cases when failed to issue pattern_set(). Maybe the LED hardware
>> occurs some errors, in this case we should shutdown the LED, not
>> enable the software pattern instead, which still can not work.
>
> This is arguable. Timer trigger always resorts to using software
> fallback if blink_set() fails.
Timer trigger is simple which only need delay_on and delay_off, that
means software can be easy to show the same performance. But hardware
pattern can be diverse, we need to convert hardware patterns to
software patterns.
>> Maybe
>> driver can set NULL for pattern_set/clear interfaces to disable
>> hardware pattern, then next time user will perform the patterns with
>> software pattern mode.
>
> Resetting any ops after LED class driver is probed should be
> deemed forbidden, since LED core can make some decisions basing on
> whether given ops are initialized or not.
OK.
>
>> For our SC27XX LED, like I said if user provides only 3 pattern values
>> which will failed to issue pattern_set(). But I still do not need use
>> software patterns to show similar performance, instead it will still
>> keep the last hardware pattern performance ( It did not overlap the
>> previous hardware pattern setting). Maybe different drivers have
>> different error handling, that's why I think we can leave driver to
>> choose the proper way to handle.
>
> ABI semantics is generic, i.e. common for all drivers. Driver can
> always log any problems occurred while setting pattern, but it shouldn't
> necessarily need to prevent pattern trigger from using software
> fallback.
Fine.
>>
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/10/2018 05:26 PM, Baolin Wang wrote:
> Hi Jacek,
>
> On 9 August 2018 at 21:21, Jacek Anaszewski
> wrote:
>> Hi Baolin,
>>
>> On 08/09/2018 07:48 AM, Baolin Wang wrote:
>> [...]
>>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>>> + struct led_classdev *led_cdev)
>>> +{
>>> + if (!data->npatterns)
>>> + return 0;
>>> +
>>> + if (led_cdev->pattern_set) {
>>> + return led_cdev->pattern_set(led_cdev, data->patterns,
>>> + data->npatterns,
>>> data->repeat);
>>
>> 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.
>
> Hmmm, I am not sure this is useful for hardware pattern, since the LED
> hardware can be diverse which is hard to be compatible with software
> pattern.
>
> For example, for our SC27XX LED, it only supports 4 hardware patterns
> setting (low time, rising time, high time and falling time) to make it
> work at breathing mode. If user provides 3 or 5 patterns values, it
> will failed to issue pattern_set(). But at this time, we don't want to
> use software pattern instead since it will be not the breathing mode
> expected by user. I don't know if there are other special LED
> hardware.
Good point. So this is the issue we should dwell on, since the
requested pattern effect should look similar on all devices.
Of course in case of software pattern it will be often impossible
to achieve the same fluency. Similarly as in case of rendering graphics
with and without acceleration.
In case of your device, I'd say that we'd need more complex description
of breathing mode pattern. More complex than just four intervals.
It should reflect all the intervals the hardware engine needs to perform
to achieve the breathing effect. Can this information be inferred from
the docs?
>>>
>>> >From our docs, it only provides registers to set the low time, rising
>>> time, high time and falling time (value unit is 0.125s and max value
>>> is 63 = 7.875s) to enable breathing mode. So each interval value can
>>> be 1 ~ 63. But that is still hard for software pattern to simulate the
>>> breathing mode performed by hardware pattern.
>>
>> Software fallback is not expected to show similar performance to the
>> hardware engine on the whole span of the supported time range.
>>
>> Having min rise time value at 125ms, and given that max_brightness
>> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
>> 1ms. So, the pattern for rising edge could look like (assuming we
>> stop at 254):
>>
>> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
>
> Right, maybe this can work, maybe not. Since we can met different
> cases when failed to issue pattern_set(). Maybe the LED hardware
> occurs some errors, in this case we should shutdown the LED, not
> enable the software pattern instead, which still can not work.
This is arguable. Timer trigger always resorts to using software
fallback if blink_set() fails.
> Maybe
> driver can set NULL for pattern_set/clear interfaces to disable
> hardware pattern, then next time user will perform the patterns with
> software pattern mode.
Resetting any ops after LED class driver is probed should be
deemed forbidden, since LED core can make some decisions basing on
whether given ops are initialized or not.
> For our SC27XX LED, like I said if user provides only 3 pattern values
> which will failed to issue pattern_set(). But I still do not need use
> software patterns to show similar performance, instead it will still
> keep the last hardware pattern performance ( It did not overlap the
> previous hardware pattern setting). Maybe different drivers have
> different error handling, that's why I think we can leave driver to
> choose the proper way to handle.
ABI semantics is generic, i.e. common for all drivers. Driver can
always log any problems occurred while setting pattern, but it shouldn't
necessarily need to prevent pattern trigger from using software
fallback.
> Honestly, can we keep this pattern trigger simple and easy at first?
> If some drivers want to use software patterns to perform again once
> their hardware patterns performed failed (IMHO our SC27XX LED do not
> need), then we can add this feature, at that time we will have users
> to test and optimize the feature. Maybe I am wrong:)
In other words you want to prevent users from exploiting the flexibility
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/10/2018 05:26 PM, Baolin Wang wrote:
> Hi Jacek,
>
> On 9 August 2018 at 21:21, Jacek Anaszewski
> wrote:
>> Hi Baolin,
>>
>> On 08/09/2018 07:48 AM, Baolin Wang wrote:
>> [...]
>>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>>> + struct led_classdev *led_cdev)
>>> +{
>>> + if (!data->npatterns)
>>> + return 0;
>>> +
>>> + if (led_cdev->pattern_set) {
>>> + return led_cdev->pattern_set(led_cdev, data->patterns,
>>> + data->npatterns,
>>> data->repeat);
>>
>> 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.
>
> Hmmm, I am not sure this is useful for hardware pattern, since the LED
> hardware can be diverse which is hard to be compatible with software
> pattern.
>
> For example, for our SC27XX LED, it only supports 4 hardware patterns
> setting (low time, rising time, high time and falling time) to make it
> work at breathing mode. If user provides 3 or 5 patterns values, it
> will failed to issue pattern_set(). But at this time, we don't want to
> use software pattern instead since it will be not the breathing mode
> expected by user. I don't know if there are other special LED
> hardware.
Good point. So this is the issue we should dwell on, since the
requested pattern effect should look similar on all devices.
Of course in case of software pattern it will be often impossible
to achieve the same fluency. Similarly as in case of rendering graphics
with and without acceleration.
In case of your device, I'd say that we'd need more complex description
of breathing mode pattern. More complex than just four intervals.
It should reflect all the intervals the hardware engine needs to perform
to achieve the breathing effect. Can this information be inferred from
the docs?
>>>
>>> >From our docs, it only provides registers to set the low time, rising
>>> time, high time and falling time (value unit is 0.125s and max value
>>> is 63 = 7.875s) to enable breathing mode. So each interval value can
>>> be 1 ~ 63. But that is still hard for software pattern to simulate the
>>> breathing mode performed by hardware pattern.
>>
>> Software fallback is not expected to show similar performance to the
>> hardware engine on the whole span of the supported time range.
>>
>> Having min rise time value at 125ms, and given that max_brightness
>> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
>> 1ms. So, the pattern for rising edge could look like (assuming we
>> stop at 254):
>>
>> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
>
> Right, maybe this can work, maybe not. Since we can met different
> cases when failed to issue pattern_set(). Maybe the LED hardware
> occurs some errors, in this case we should shutdown the LED, not
> enable the software pattern instead, which still can not work.
This is arguable. Timer trigger always resorts to using software
fallback if blink_set() fails.
> Maybe
> driver can set NULL for pattern_set/clear interfaces to disable
> hardware pattern, then next time user will perform the patterns with
> software pattern mode.
Resetting any ops after LED class driver is probed should be
deemed forbidden, since LED core can make some decisions basing on
whether given ops are initialized or not.
> For our SC27XX LED, like I said if user provides only 3 pattern values
> which will failed to issue pattern_set(). But I still do not need use
> software patterns to show similar performance, instead it will still
> keep the last hardware pattern performance ( It did not overlap the
> previous hardware pattern setting). Maybe different drivers have
> different error handling, that's why I think we can leave driver to
> choose the proper way to handle.
ABI semantics is generic, i.e. common for all drivers. Driver can
always log any problems occurred while setting pattern, but it shouldn't
necessarily need to prevent pattern trigger from using software
fallback.
> Honestly, can we keep this pattern trigger simple and easy at first?
> If some drivers want to use software patterns to perform again once
> their hardware patterns performed failed (IMHO our SC27XX LED do not
> need), then we can add this feature, at that time we will have users
> to test and optimize the feature. Maybe I am wrong:)
In other words you want to prevent users from exploiting the flexibility
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 9 August 2018 at 21:21, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/09/2018 07:48 AM, Baolin Wang wrote:
> [...]
>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>> + struct led_classdev *led_cdev)
>> +{
>> + if (!data->npatterns)
>> + return 0;
>> +
>> + if (led_cdev->pattern_set) {
>> + return led_cdev->pattern_set(led_cdev, data->patterns,
>> + data->npatterns,
>> data->repeat);
>
> 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.
Hmmm, I am not sure this is useful for hardware pattern, since the LED
hardware can be diverse which is hard to be compatible with software
pattern.
For example, for our SC27XX LED, it only supports 4 hardware patterns
setting (low time, rising time, high time and falling time) to make it
work at breathing mode. If user provides 3 or 5 patterns values, it
will failed to issue pattern_set(). But at this time, we don't want to
use software pattern instead since it will be not the breathing mode
expected by user. I don't know if there are other special LED
hardware.
>>>
>>> Good point. So this is the issue we should dwell on, since the
>>> requested pattern effect should look similar on all devices.
>>> Of course in case of software pattern it will be often impossible
>>> to achieve the same fluency. Similarly as in case of rendering graphics
>>> with and without acceleration.
>>>
>>> In case of your device, I'd say that we'd need more complex description
>>> of breathing mode pattern. More complex than just four intervals.
>>> It should reflect all the intervals the hardware engine needs to perform
>>> to achieve the breathing effect. Can this information be inferred from
>>> the docs?
>>
>>>From our docs, it only provides registers to set the low time, rising
>> time, high time and falling time (value unit is 0.125s and max value
>> is 63 = 7.875s) to enable breathing mode. So each interval value can
>> be 1 ~ 63. But that is still hard for software pattern to simulate the
>> breathing mode performed by hardware pattern.
>
> Software fallback is not expected to show similar performance to the
> hardware engine on the whole span of the supported time range.
>
> Having min rise time value at 125ms, and given that max_brightness
> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
> 1ms. So, the pattern for rising edge could look like (assuming we
> stop at 254):
>
> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
Right, maybe this can work, maybe not. Since we can met different
cases when failed to issue pattern_set(). Maybe the LED hardware
occurs some errors, in this case we should shutdown the LED, not
enable the software pattern instead, which still can not work. Maybe
driver can set NULL for pattern_set/clear interfaces to disable
hardware pattern, then next time user will perform the patterns with
software pattern mode.
For our SC27XX LED, like I said if user provides only 3 pattern values
which will failed to issue pattern_set(). But I still do not need use
software patterns to show similar performance, instead it will still
keep the last hardware pattern performance ( It did not overlap the
previous hardware pattern setting). Maybe different drivers have
different error handling, that's why I think we can leave driver to
choose the proper way to handle.
Honestly, can we keep this pattern trigger simple and easy at first?
If some drivers want to use software patterns to perform again once
their hardware patterns performed failed (IMHO our SC27XX LED do not
need), then we can add this feature, at that time we will have users
to test and optimize the feature. Maybe I am wrong:)
> Now, I'm starting to wonder if we shouldn't have specialized trigger
> for breathing patterns, that would accept brightness level change per
> time period. Pattern trigger needs more flexibility and inferring if the
> hardware can handle given series of pattern intervals would entail
> unnecessary code complexity.
>
> Such breathing trigger would require triplets comprised of
> start brightness, end brightness and a duration of the brightness
> transition.
But this is the only place we can set the hardware patterns according
to our previous discussion. Thanks.
--
Baolin Wang
Best Regards
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek,
On 9 August 2018 at 21:21, Jacek Anaszewski wrote:
> Hi Baolin,
>
> On 08/09/2018 07:48 AM, Baolin Wang wrote:
> [...]
>> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
>> + struct led_classdev *led_cdev)
>> +{
>> + if (!data->npatterns)
>> + return 0;
>> +
>> + if (led_cdev->pattern_set) {
>> + return led_cdev->pattern_set(led_cdev, data->patterns,
>> + data->npatterns,
>> data->repeat);
>
> 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.
Hmmm, I am not sure this is useful for hardware pattern, since the LED
hardware can be diverse which is hard to be compatible with software
pattern.
For example, for our SC27XX LED, it only supports 4 hardware patterns
setting (low time, rising time, high time and falling time) to make it
work at breathing mode. If user provides 3 or 5 patterns values, it
will failed to issue pattern_set(). But at this time, we don't want to
use software pattern instead since it will be not the breathing mode
expected by user. I don't know if there are other special LED
hardware.
>>>
>>> Good point. So this is the issue we should dwell on, since the
>>> requested pattern effect should look similar on all devices.
>>> Of course in case of software pattern it will be often impossible
>>> to achieve the same fluency. Similarly as in case of rendering graphics
>>> with and without acceleration.
>>>
>>> In case of your device, I'd say that we'd need more complex description
>>> of breathing mode pattern. More complex than just four intervals.
>>> It should reflect all the intervals the hardware engine needs to perform
>>> to achieve the breathing effect. Can this information be inferred from
>>> the docs?
>>
>>>From our docs, it only provides registers to set the low time, rising
>> time, high time and falling time (value unit is 0.125s and max value
>> is 63 = 7.875s) to enable breathing mode. So each interval value can
>> be 1 ~ 63. But that is still hard for software pattern to simulate the
>> breathing mode performed by hardware pattern.
>
> Software fallback is not expected to show similar performance to the
> hardware engine on the whole span of the supported time range.
>
> Having min rise time value at 125ms, and given that max_brightness
> is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
> 1ms. So, the pattern for rising edge could look like (assuming we
> stop at 254):
>
> 0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
Right, maybe this can work, maybe not. Since we can met different
cases when failed to issue pattern_set(). Maybe the LED hardware
occurs some errors, in this case we should shutdown the LED, not
enable the software pattern instead, which still can not work. Maybe
driver can set NULL for pattern_set/clear interfaces to disable
hardware pattern, then next time user will perform the patterns with
software pattern mode.
For our SC27XX LED, like I said if user provides only 3 pattern values
which will failed to issue pattern_set(). But I still do not need use
software patterns to show similar performance, instead it will still
keep the last hardware pattern performance ( It did not overlap the
previous hardware pattern setting). Maybe different drivers have
different error handling, that's why I think we can leave driver to
choose the proper way to handle.
Honestly, can we keep this pattern trigger simple and easy at first?
If some drivers want to use software patterns to perform again once
their hardware patterns performed failed (IMHO our SC27XX LED do not
need), then we can add this feature, at that time we will have users
to test and optimize the feature. Maybe I am wrong:)
> Now, I'm starting to wonder if we shouldn't have specialized trigger
> for breathing patterns, that would accept brightness level change per
> time period. Pattern trigger needs more flexibility and inferring if the
> hardware can handle given series of pattern intervals would entail
> unnecessary code complexity.
>
> Such breathing trigger would require triplets comprised of
> start brightness, end brightness and a duration of the brightness
> transition.
But this is the only place we can set the hardware patterns according
to our previous discussion. Thanks.
--
Baolin Wang
Best Regards
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/09/2018 07:48 AM, Baolin Wang wrote:
[...]
> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
> + struct led_classdev *led_cdev)
> +{
> + if (!data->npatterns)
> + return 0;
> +
> + if (led_cdev->pattern_set) {
> + return led_cdev->pattern_set(led_cdev, data->patterns,
> + data->npatterns, data->repeat);
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.
>>>
>>> Hmmm, I am not sure this is useful for hardware pattern, since the LED
>>> hardware can be diverse which is hard to be compatible with software
>>> pattern.
>>>
>>> For example, for our SC27XX LED, it only supports 4 hardware patterns
>>> setting (low time, rising time, high time and falling time) to make it
>>> work at breathing mode. If user provides 3 or 5 patterns values, it
>>> will failed to issue pattern_set(). But at this time, we don't want to
>>> use software pattern instead since it will be not the breathing mode
>>> expected by user. I don't know if there are other special LED
>>> hardware.
>>
>> Good point. So this is the issue we should dwell on, since the
>> requested pattern effect should look similar on all devices.
>> Of course in case of software pattern it will be often impossible
>> to achieve the same fluency. Similarly as in case of rendering graphics
>> with and without acceleration.
>>
>> In case of your device, I'd say that we'd need more complex description
>> of breathing mode pattern. More complex than just four intervals.
>> It should reflect all the intervals the hardware engine needs to perform
>> to achieve the breathing effect. Can this information be inferred from
>> the docs?
>
>>From our docs, it only provides registers to set the low time, rising
> time, high time and falling time (value unit is 0.125s and max value
> is 63 = 7.875s) to enable breathing mode. So each interval value can
> be 1 ~ 63. But that is still hard for software pattern to simulate the
> breathing mode performed by hardware pattern.
Software fallback is not expected to show similar performance to the
hardware engine on the whole span of the supported time range.
Having min rise time value at 125ms, and given that max_brightness
is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
1ms. So, the pattern for rising edge could look like (assuming we
stop at 254):
0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
Now, I'm starting to wonder if we shouldn't have specialized trigger
for breathing patterns, that would accept brightness level change per
time period. Pattern trigger needs more flexibility and inferring if the
hardware can handle given series of pattern intervals would entail
unnecessary code complexity.
Such breathing trigger would require triplets comprised of
start brightness, end brightness and a duration of the brightness
transition.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
On 08/09/2018 07:48 AM, Baolin Wang wrote:
[...]
> +static int pattern_trig_start_pattern(struct pattern_trig_data *data,
> + struct led_classdev *led_cdev)
> +{
> + if (!data->npatterns)
> + return 0;
> +
> + if (led_cdev->pattern_set) {
> + return led_cdev->pattern_set(led_cdev, data->patterns,
> + data->npatterns, data->repeat);
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.
>>>
>>> Hmmm, I am not sure this is useful for hardware pattern, since the LED
>>> hardware can be diverse which is hard to be compatible with software
>>> pattern.
>>>
>>> For example, for our SC27XX LED, it only supports 4 hardware patterns
>>> setting (low time, rising time, high time and falling time) to make it
>>> work at breathing mode. If user provides 3 or 5 patterns values, it
>>> will failed to issue pattern_set(). But at this time, we don't want to
>>> use software pattern instead since it will be not the breathing mode
>>> expected by user. I don't know if there are other special LED
>>> hardware.
>>
>> Good point. So this is the issue we should dwell on, since the
>> requested pattern effect should look similar on all devices.
>> Of course in case of software pattern it will be often impossible
>> to achieve the same fluency. Similarly as in case of rendering graphics
>> with and without acceleration.
>>
>> In case of your device, I'd say that we'd need more complex description
>> of breathing mode pattern. More complex than just four intervals.
>> It should reflect all the intervals the hardware engine needs to perform
>> to achieve the breathing effect. Can this information be inferred from
>> the docs?
>
>>From our docs, it only provides registers to set the low time, rising
> time, high time and falling time (value unit is 0.125s and max value
> is 63 = 7.875s) to enable breathing mode. So each interval value can
> be 1 ~ 63. But that is still hard for software pattern to simulate the
> breathing mode performed by hardware pattern.
Software fallback is not expected to show similar performance to the
hardware engine on the whole span of the supported time range.
Having min rise time value at 125ms, and given that max_brightness
is 255, then we'd have 255 / 125 = 2.04 of brightness level rise per
1ms. So, the pattern for rising edge could look like (assuming we
stop at 254):
0 1 2 1 4 1 6 1 8 1 10 1 ... 254 1
Now, I'm starting to wonder if we shouldn't have specialized trigger
for breathing patterns, that would accept brightness level change per
time period. Pattern trigger needs more flexibility and inferring if the
hardware can handle given series of pattern intervals would entail
unnecessary code complexity.
Such breathing trigger would require triplets comprised of
start brightness, end brightness and a duration of the brightness
transition.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek, On 9 August 2018 at 05:28, Jacek Anaszewski wrote: > Hi Baolin, > > On 08/08/2018 08:01 AM, Baolin Wang wrote: >> Hi Jacek, >> >> On 8 August 2018 at 05:54, Jacek Anaszewski >> wrote: >>> Hi Baolin, >>> >>> Thank you for addressing the review remarks. >>> Since the patch set is targeted for 4.19, then we have three weeks >>> before it will be merged to the for-next anyway. That said, I propose >>> one more modification, please take a look below. >> >> Sure. >> >>> >>> On 08/06/2018 02:05 PM, Baolin Wang wrote: Some LED controllers have support for autonomously controlling brightness over time, according to some preprogrammed pattern or function. This patch adds pattern trigger that LED device can configure the pattern and trigger it. Signed-off-by: Raphael Teysseyre Signed-off-by: Baolin Wang --- Changes from v4: - Change the repeat file to return the originally written number. - Improve comments. - Fix some build warnings. Changes from v3: - Reset pattern number to 0 if user provides incorrect pattern string. - Support one pattern. Changes from v2: - Remove hardware_pattern boolen. - Chnage the pattern string format. Changes from v1: - Use ATTRIBUTE_GROUPS() to define attributes. - Introduce hardware_pattern flag to determine if software pattern or hardware pattern. - Re-implement pattern_trig_store_pattern() function. - Remove pattern_get() interface. - Improve comments. - Other small optimization. --- .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ drivers/leds/trigger/Kconfig |7 + drivers/leds/trigger/Makefile |1 + drivers/leds/trigger/ledtrig-pattern.c | 266 include/linux/leds.h | 16 ++ 5 files changed, 314 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern create mode 100644 drivers/leds/trigger/ledtrig-pattern.c diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern new file mode 100644 index 000..bc7475f --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern @@ -0,0 +1,24 @@ +What:/sys/class/leds//pattern +Date:August 2018 +KernelVersion: 4.19 +Description: + Specify a pattern for the LED, for LED hardware that support + altering the brightness as a function of time. + + The pattern is given by a series of tuples, of brightness and + duration (ms). The LED is expected to traverse the series and + each brightness value for the specified duration. Duration of + 0 means brightness should immediately change to new value. + + The format of the pattern values should be: + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 + duration_3 ...". + +What:/sys/class/leds//repeat +Date:August 2018 +KernelVersion: 4.19 +Description: + Specify a pattern repeat number. 0 means repeat indefinitely. + + This file will always return the originally written repeat + number. diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 4018af7..b76fc3c 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV This allows LEDs to be controlled by network device activity. If unsure, say Y. +config LEDS_TRIGGER_PATTERN + tristate "LED Pattern Trigger" + help + This allows LEDs to be controlled by a software or hardware pattern + which is a series of tuples, of brightness and duration (ms). + If unsure, say N + endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index f3cfe19..9bcb64e 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c new file mode 100644 index 000..63b94a2
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek, On 9 August 2018 at 05:28, Jacek Anaszewski wrote: > Hi Baolin, > > On 08/08/2018 08:01 AM, Baolin Wang wrote: >> Hi Jacek, >> >> On 8 August 2018 at 05:54, Jacek Anaszewski >> wrote: >>> Hi Baolin, >>> >>> Thank you for addressing the review remarks. >>> Since the patch set is targeted for 4.19, then we have three weeks >>> before it will be merged to the for-next anyway. That said, I propose >>> one more modification, please take a look below. >> >> Sure. >> >>> >>> On 08/06/2018 02:05 PM, Baolin Wang wrote: Some LED controllers have support for autonomously controlling brightness over time, according to some preprogrammed pattern or function. This patch adds pattern trigger that LED device can configure the pattern and trigger it. Signed-off-by: Raphael Teysseyre Signed-off-by: Baolin Wang --- Changes from v4: - Change the repeat file to return the originally written number. - Improve comments. - Fix some build warnings. Changes from v3: - Reset pattern number to 0 if user provides incorrect pattern string. - Support one pattern. Changes from v2: - Remove hardware_pattern boolen. - Chnage the pattern string format. Changes from v1: - Use ATTRIBUTE_GROUPS() to define attributes. - Introduce hardware_pattern flag to determine if software pattern or hardware pattern. - Re-implement pattern_trig_store_pattern() function. - Remove pattern_get() interface. - Improve comments. - Other small optimization. --- .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ drivers/leds/trigger/Kconfig |7 + drivers/leds/trigger/Makefile |1 + drivers/leds/trigger/ledtrig-pattern.c | 266 include/linux/leds.h | 16 ++ 5 files changed, 314 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern create mode 100644 drivers/leds/trigger/ledtrig-pattern.c diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern new file mode 100644 index 000..bc7475f --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern @@ -0,0 +1,24 @@ +What:/sys/class/leds//pattern +Date:August 2018 +KernelVersion: 4.19 +Description: + Specify a pattern for the LED, for LED hardware that support + altering the brightness as a function of time. + + The pattern is given by a series of tuples, of brightness and + duration (ms). The LED is expected to traverse the series and + each brightness value for the specified duration. Duration of + 0 means brightness should immediately change to new value. + + The format of the pattern values should be: + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 + duration_3 ...". + +What:/sys/class/leds//repeat +Date:August 2018 +KernelVersion: 4.19 +Description: + Specify a pattern repeat number. 0 means repeat indefinitely. + + This file will always return the originally written repeat + number. diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index 4018af7..b76fc3c 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV This allows LEDs to be controlled by network device activity. If unsure, say Y. +config LEDS_TRIGGER_PATTERN + tristate "LED Pattern Trigger" + help + This allows LEDs to be controlled by a software or hardware pattern + which is a series of tuples, of brightness and duration (ms). + If unsure, say N + endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile index f3cfe19..9bcb64e 100644 --- a/drivers/leds/trigger/Makefile +++ b/drivers/leds/trigger/Makefile @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= ledtrig-transient.o obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o diff --git a/drivers/leds/trigger/ledtrig-pattern.c b/drivers/leds/trigger/ledtrig-pattern.c new file mode 100644 index 000..63b94a2
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin, On 08/08/2018 08:01 AM, Baolin Wang wrote: > Hi Jacek, > > On 8 August 2018 at 05:54, Jacek Anaszewski > wrote: >> Hi Baolin, >> >> Thank you for addressing the review remarks. >> Since the patch set is targeted for 4.19, then we have three weeks >> before it will be merged to the for-next anyway. That said, I propose >> one more modification, please take a look below. > > Sure. > >> >> On 08/06/2018 02:05 PM, Baolin Wang wrote: >>> Some LED controllers have support for autonomously controlling >>> brightness over time, according to some preprogrammed pattern or >>> function. >>> >>> This patch adds pattern trigger that LED device can configure the >>> pattern and trigger it. >>> >>> Signed-off-by: Raphael Teysseyre >>> Signed-off-by: Baolin Wang >>> --- >>> Changes from v4: >>> - Change the repeat file to return the originally written number. >>> - Improve comments. >>> - Fix some build warnings. >>> >>> Changes from v3: >>> - Reset pattern number to 0 if user provides incorrect pattern string. >>> - Support one pattern. >>> >>> Changes from v2: >>> - Remove hardware_pattern boolen. >>> - Chnage the pattern string format. >>> >>> Changes from v1: >>> - Use ATTRIBUTE_GROUPS() to define attributes. >>> - Introduce hardware_pattern flag to determine if software pattern >>> or hardware pattern. >>> - Re-implement pattern_trig_store_pattern() function. >>> - Remove pattern_get() interface. >>> - Improve comments. >>> - Other small optimization. >>> --- >>> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ >>> drivers/leds/trigger/Kconfig |7 + >>> drivers/leds/trigger/Makefile |1 + >>> drivers/leds/trigger/ledtrig-pattern.c | 266 >>> >>> include/linux/leds.h | 16 ++ >>> 5 files changed, 314 insertions(+) >>> create mode 100644 >>> Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c >>> >>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> new file mode 100644 >>> index 000..bc7475f >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> @@ -0,0 +1,24 @@ >>> +What:/sys/class/leds//pattern >>> +Date:August 2018 >>> +KernelVersion: 4.19 >>> +Description: >>> + Specify a pattern for the LED, for LED hardware that support >>> + altering the brightness as a function of time. >>> + >>> + The pattern is given by a series of tuples, of brightness and >>> + duration (ms). The LED is expected to traverse the series and >>> + each brightness value for the specified duration. Duration of >>> + 0 means brightness should immediately change to new value. >>> + >>> + The format of the pattern values should be: >>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >>> + duration_3 ...". >>> + >>> +What:/sys/class/leds//repeat >>> +Date:August 2018 >>> +KernelVersion: 4.19 >>> +Description: >>> + Specify a pattern repeat number. 0 means repeat indefinitely. >>> + >>> + This file will always return the originally written repeat >>> + number. >>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >>> index 4018af7..b76fc3c 100644 >>> --- a/drivers/leds/trigger/Kconfig >>> +++ b/drivers/leds/trigger/Kconfig >>> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV >>> This allows LEDs to be controlled by network device activity. >>> If unsure, say Y. >>> >>> +config LEDS_TRIGGER_PATTERN >>> + tristate "LED Pattern Trigger" >>> + help >>> + This allows LEDs to be controlled by a software or hardware pattern >>> + which is a series of tuples, of brightness and duration (ms). >>> + If unsure, say N >>> + >>> endif # LEDS_TRIGGERS >>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile >>> index f3cfe19..9bcb64e 100644 >>> --- a/drivers/leds/trigger/Makefile >>> +++ b/drivers/leds/trigger/Makefile >>> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= >>> ledtrig-transient.o >>> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o >>> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o >>> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o >>> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o >>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c >>> b/drivers/leds/trigger/ledtrig-pattern.c >>> new file mode 100644 >>> index 000..63b94a2 >>> --- /dev/null >>> +++ b/drivers/leds/trigger/ledtrig-pattern.c >>> @@ -0,0 +1,266 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +/* >>> + * LED pattern trigger >>> + * >>> +
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin, On 08/08/2018 08:01 AM, Baolin Wang wrote: > Hi Jacek, > > On 8 August 2018 at 05:54, Jacek Anaszewski > wrote: >> Hi Baolin, >> >> Thank you for addressing the review remarks. >> Since the patch set is targeted for 4.19, then we have three weeks >> before it will be merged to the for-next anyway. That said, I propose >> one more modification, please take a look below. > > Sure. > >> >> On 08/06/2018 02:05 PM, Baolin Wang wrote: >>> Some LED controllers have support for autonomously controlling >>> brightness over time, according to some preprogrammed pattern or >>> function. >>> >>> This patch adds pattern trigger that LED device can configure the >>> pattern and trigger it. >>> >>> Signed-off-by: Raphael Teysseyre >>> Signed-off-by: Baolin Wang >>> --- >>> Changes from v4: >>> - Change the repeat file to return the originally written number. >>> - Improve comments. >>> - Fix some build warnings. >>> >>> Changes from v3: >>> - Reset pattern number to 0 if user provides incorrect pattern string. >>> - Support one pattern. >>> >>> Changes from v2: >>> - Remove hardware_pattern boolen. >>> - Chnage the pattern string format. >>> >>> Changes from v1: >>> - Use ATTRIBUTE_GROUPS() to define attributes. >>> - Introduce hardware_pattern flag to determine if software pattern >>> or hardware pattern. >>> - Re-implement pattern_trig_store_pattern() function. >>> - Remove pattern_get() interface. >>> - Improve comments. >>> - Other small optimization. >>> --- >>> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ >>> drivers/leds/trigger/Kconfig |7 + >>> drivers/leds/trigger/Makefile |1 + >>> drivers/leds/trigger/ledtrig-pattern.c | 266 >>> >>> include/linux/leds.h | 16 ++ >>> 5 files changed, 314 insertions(+) >>> create mode 100644 >>> Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c >>> >>> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> new file mode 100644 >>> index 000..bc7475f >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >>> @@ -0,0 +1,24 @@ >>> +What:/sys/class/leds//pattern >>> +Date:August 2018 >>> +KernelVersion: 4.19 >>> +Description: >>> + Specify a pattern for the LED, for LED hardware that support >>> + altering the brightness as a function of time. >>> + >>> + The pattern is given by a series of tuples, of brightness and >>> + duration (ms). The LED is expected to traverse the series and >>> + each brightness value for the specified duration. Duration of >>> + 0 means brightness should immediately change to new value. >>> + >>> + The format of the pattern values should be: >>> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >>> + duration_3 ...". >>> + >>> +What:/sys/class/leds//repeat >>> +Date:August 2018 >>> +KernelVersion: 4.19 >>> +Description: >>> + Specify a pattern repeat number. 0 means repeat indefinitely. >>> + >>> + This file will always return the originally written repeat >>> + number. >>> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >>> index 4018af7..b76fc3c 100644 >>> --- a/drivers/leds/trigger/Kconfig >>> +++ b/drivers/leds/trigger/Kconfig >>> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV >>> This allows LEDs to be controlled by network device activity. >>> If unsure, say Y. >>> >>> +config LEDS_TRIGGER_PATTERN >>> + tristate "LED Pattern Trigger" >>> + help >>> + This allows LEDs to be controlled by a software or hardware pattern >>> + which is a series of tuples, of brightness and duration (ms). >>> + If unsure, say N >>> + >>> endif # LEDS_TRIGGERS >>> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile >>> index f3cfe19..9bcb64e 100644 >>> --- a/drivers/leds/trigger/Makefile >>> +++ b/drivers/leds/trigger/Makefile >>> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= >>> ledtrig-transient.o >>> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o >>> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o >>> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o >>> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o >>> diff --git a/drivers/leds/trigger/ledtrig-pattern.c >>> b/drivers/leds/trigger/ledtrig-pattern.c >>> new file mode 100644 >>> index 000..63b94a2 >>> --- /dev/null >>> +++ b/drivers/leds/trigger/ledtrig-pattern.c >>> @@ -0,0 +1,266 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +/* >>> + * LED pattern trigger >>> + * >>> +
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek, On 8 August 2018 at 05:54, Jacek Anaszewski wrote: > Hi Baolin, > > Thank you for addressing the review remarks. > Since the patch set is targeted for 4.19, then we have three weeks > before it will be merged to the for-next anyway. That said, I propose > one more modification, please take a look below. Sure. > > On 08/06/2018 02:05 PM, Baolin Wang wrote: >> Some LED controllers have support for autonomously controlling >> brightness over time, according to some preprogrammed pattern or >> function. >> >> This patch adds pattern trigger that LED device can configure the >> pattern and trigger it. >> >> Signed-off-by: Raphael Teysseyre >> Signed-off-by: Baolin Wang >> --- >> Changes from v4: >> - Change the repeat file to return the originally written number. >> - Improve comments. >> - Fix some build warnings. >> >> Changes from v3: >> - Reset pattern number to 0 if user provides incorrect pattern string. >> - Support one pattern. >> >> Changes from v2: >> - Remove hardware_pattern boolen. >> - Chnage the pattern string format. >> >> Changes from v1: >> - Use ATTRIBUTE_GROUPS() to define attributes. >> - Introduce hardware_pattern flag to determine if software pattern >> or hardware pattern. >> - Re-implement pattern_trig_store_pattern() function. >> - Remove pattern_get() interface. >> - Improve comments. >> - Other small optimization. >> --- >> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ >> drivers/leds/trigger/Kconfig |7 + >> drivers/leds/trigger/Makefile |1 + >> drivers/leds/trigger/ledtrig-pattern.c | 266 >> >> include/linux/leds.h | 16 ++ >> 5 files changed, 314 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c >> >> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> new file mode 100644 >> index 000..bc7475f >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> @@ -0,0 +1,24 @@ >> +What:/sys/class/leds//pattern >> +Date:August 2018 >> +KernelVersion: 4.19 >> +Description: >> + Specify a pattern for the LED, for LED hardware that support >> + altering the brightness as a function of time. >> + >> + The pattern is given by a series of tuples, of brightness and >> + duration (ms). The LED is expected to traverse the series and >> + each brightness value for the specified duration. Duration of >> + 0 means brightness should immediately change to new value. >> + >> + The format of the pattern values should be: >> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >> + duration_3 ...". >> + >> +What:/sys/class/leds//repeat >> +Date:August 2018 >> +KernelVersion: 4.19 >> +Description: >> + Specify a pattern repeat number. 0 means repeat indefinitely. >> + >> + This file will always return the originally written repeat >> + number. >> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >> index 4018af7..b76fc3c 100644 >> --- a/drivers/leds/trigger/Kconfig >> +++ b/drivers/leds/trigger/Kconfig >> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV >> This allows LEDs to be controlled by network device activity. >> If unsure, say Y. >> >> +config LEDS_TRIGGER_PATTERN >> + tristate "LED Pattern Trigger" >> + help >> + This allows LEDs to be controlled by a software or hardware pattern >> + which is a series of tuples, of brightness and duration (ms). >> + If unsure, say N >> + >> endif # LEDS_TRIGGERS >> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile >> index f3cfe19..9bcb64e 100644 >> --- a/drivers/leds/trigger/Makefile >> +++ b/drivers/leds/trigger/Makefile >> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= >> ledtrig-transient.o >> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o >> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o >> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o >> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o >> diff --git a/drivers/leds/trigger/ledtrig-pattern.c >> b/drivers/leds/trigger/ledtrig-pattern.c >> new file mode 100644 >> index 000..63b94a2 >> --- /dev/null >> +++ b/drivers/leds/trigger/ledtrig-pattern.c >> @@ -0,0 +1,266 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +/* >> + * LED pattern trigger >> + * >> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented >> + * the first version, Baolin Wang simplified and improved the approach. >> + */ >> + >> +#include >> +#include >>
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Jacek, On 8 August 2018 at 05:54, Jacek Anaszewski wrote: > Hi Baolin, > > Thank you for addressing the review remarks. > Since the patch set is targeted for 4.19, then we have three weeks > before it will be merged to the for-next anyway. That said, I propose > one more modification, please take a look below. Sure. > > On 08/06/2018 02:05 PM, Baolin Wang wrote: >> Some LED controllers have support for autonomously controlling >> brightness over time, according to some preprogrammed pattern or >> function. >> >> This patch adds pattern trigger that LED device can configure the >> pattern and trigger it. >> >> Signed-off-by: Raphael Teysseyre >> Signed-off-by: Baolin Wang >> --- >> Changes from v4: >> - Change the repeat file to return the originally written number. >> - Improve comments. >> - Fix some build warnings. >> >> Changes from v3: >> - Reset pattern number to 0 if user provides incorrect pattern string. >> - Support one pattern. >> >> Changes from v2: >> - Remove hardware_pattern boolen. >> - Chnage the pattern string format. >> >> Changes from v1: >> - Use ATTRIBUTE_GROUPS() to define attributes. >> - Introduce hardware_pattern flag to determine if software pattern >> or hardware pattern. >> - Re-implement pattern_trig_store_pattern() function. >> - Remove pattern_get() interface. >> - Improve comments. >> - Other small optimization. >> --- >> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++ >> drivers/leds/trigger/Kconfig |7 + >> drivers/leds/trigger/Makefile |1 + >> drivers/leds/trigger/ledtrig-pattern.c | 266 >> >> include/linux/leds.h | 16 ++ >> 5 files changed, 314 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c >> >> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> new file mode 100644 >> index 000..bc7475f >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern >> @@ -0,0 +1,24 @@ >> +What:/sys/class/leds//pattern >> +Date:August 2018 >> +KernelVersion: 4.19 >> +Description: >> + Specify a pattern for the LED, for LED hardware that support >> + altering the brightness as a function of time. >> + >> + The pattern is given by a series of tuples, of brightness and >> + duration (ms). The LED is expected to traverse the series and >> + each brightness value for the specified duration. Duration of >> + 0 means brightness should immediately change to new value. >> + >> + The format of the pattern values should be: >> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3 >> + duration_3 ...". >> + >> +What:/sys/class/leds//repeat >> +Date:August 2018 >> +KernelVersion: 4.19 >> +Description: >> + Specify a pattern repeat number. 0 means repeat indefinitely. >> + >> + This file will always return the originally written repeat >> + number. >> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >> index 4018af7..b76fc3c 100644 >> --- a/drivers/leds/trigger/Kconfig >> +++ b/drivers/leds/trigger/Kconfig >> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV >> This allows LEDs to be controlled by network device activity. >> If unsure, say Y. >> >> +config LEDS_TRIGGER_PATTERN >> + tristate "LED Pattern Trigger" >> + help >> + This allows LEDs to be controlled by a software or hardware pattern >> + which is a series of tuples, of brightness and duration (ms). >> + If unsure, say N >> + >> endif # LEDS_TRIGGERS >> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile >> index f3cfe19..9bcb64e 100644 >> --- a/drivers/leds/trigger/Makefile >> +++ b/drivers/leds/trigger/Makefile >> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+= >> ledtrig-transient.o >> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o >> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o >> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o >> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o >> diff --git a/drivers/leds/trigger/ledtrig-pattern.c >> b/drivers/leds/trigger/ledtrig-pattern.c >> new file mode 100644 >> index 000..63b94a2 >> --- /dev/null >> +++ b/drivers/leds/trigger/ledtrig-pattern.c >> @@ -0,0 +1,266 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +/* >> + * LED pattern trigger >> + * >> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented >> + * the first version, Baolin Wang simplified and improved the approach. >> + */ >> + >> +#include >> +#include >>
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
Thank you for addressing the review remarks.
Since the patch set is targeted for 4.19, then we have three weeks
before it will be merged to the for-next anyway. That said, I propose
one more modification, please take a look below.
On 08/06/2018 02:05 PM, Baolin Wang wrote:
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
>
> Signed-off-by: Raphael Teysseyre
> Signed-off-by: Baolin Wang
> ---
> Changes from v4:
> - Change the repeat file to return the originally written number.
> - Improve comments.
> - Fix some build warnings.
>
> Changes from v3:
> - Reset pattern number to 0 if user provides incorrect pattern string.
> - Support one pattern.
>
> Changes from v2:
> - Remove hardware_pattern boolen.
> - Chnage the pattern string format.
>
> Changes from v1:
> - Use ATTRIBUTE_GROUPS() to define attributes.
> - Introduce hardware_pattern flag to determine if software pattern
> or hardware pattern.
> - Re-implement pattern_trig_store_pattern() function.
> - Remove pattern_get() interface.
> - Improve comments.
> - Other small optimization.
> ---
> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++
> drivers/leds/trigger/Kconfig |7 +
> drivers/leds/trigger/Makefile |1 +
> drivers/leds/trigger/ledtrig-pattern.c | 266
>
> include/linux/leds.h | 16 ++
> 5 files changed, 314 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> new file mode 100644
> index 000..bc7475f
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,24 @@
> +What:/sys/class/leds//pattern
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a function of time.
> +
> + The pattern is given by a series of tuples, of brightness and
> + duration (ms). The LED is expected to traverse the series and
> + each brightness value for the specified duration. Duration of
> + 0 means brightness should immediately change to new value.
> +
> + The format of the pattern values should be:
> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> + duration_3 ...".
> +
> +What:/sys/class/leds//repeat
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern repeat number. 0 means repeat indefinitely.
> +
> + This file will always return the originally written repeat
> + number.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 4018af7..b76fc3c 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
> This allows LEDs to be controlled by network device activity.
> If unsure, say Y.
>
> +config LEDS_TRIGGER_PATTERN
> + tristate "LED Pattern Trigger"
> + help
> + This allows LEDs to be controlled by a software or hardware pattern
> + which is a series of tuples, of brightness and duration (ms).
> + If unsure, say N
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..9bcb64e 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
> ledtrig-transient.o
> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
> b/drivers/leds/trigger/ledtrig-pattern.c
> new file mode 100644
> index 000..63b94a2
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * LED pattern trigger
> + *
> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
> + * the first version, Baolin Wang simplified and improved the approach.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_PATTERNS 1024
> +
> +struct pattern_trig_data {
> + struct led_classdev *led_cdev;
> + struct led_pattern
Re: [PATCH v5 1/2] leds: core: Introduce LED pattern trigger
Hi Baolin,
Thank you for addressing the review remarks.
Since the patch set is targeted for 4.19, then we have three weeks
before it will be merged to the for-next anyway. That said, I propose
one more modification, please take a look below.
On 08/06/2018 02:05 PM, Baolin Wang wrote:
> Some LED controllers have support for autonomously controlling
> brightness over time, according to some preprogrammed pattern or
> function.
>
> This patch adds pattern trigger that LED device can configure the
> pattern and trigger it.
>
> Signed-off-by: Raphael Teysseyre
> Signed-off-by: Baolin Wang
> ---
> Changes from v4:
> - Change the repeat file to return the originally written number.
> - Improve comments.
> - Fix some build warnings.
>
> Changes from v3:
> - Reset pattern number to 0 if user provides incorrect pattern string.
> - Support one pattern.
>
> Changes from v2:
> - Remove hardware_pattern boolen.
> - Chnage the pattern string format.
>
> Changes from v1:
> - Use ATTRIBUTE_GROUPS() to define attributes.
> - Introduce hardware_pattern flag to determine if software pattern
> or hardware pattern.
> - Re-implement pattern_trig_store_pattern() function.
> - Remove pattern_get() interface.
> - Improve comments.
> - Other small optimization.
> ---
> .../ABI/testing/sysfs-class-led-trigger-pattern| 24 ++
> drivers/leds/trigger/Kconfig |7 +
> drivers/leds/trigger/Makefile |1 +
> drivers/leds/trigger/ledtrig-pattern.c | 266
>
> include/linux/leds.h | 16 ++
> 5 files changed, 314 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> create mode 100644 drivers/leds/trigger/ledtrig-pattern.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> new file mode 100644
> index 000..bc7475f
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
> @@ -0,0 +1,24 @@
> +What:/sys/class/leds//pattern
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern for the LED, for LED hardware that support
> + altering the brightness as a function of time.
> +
> + The pattern is given by a series of tuples, of brightness and
> + duration (ms). The LED is expected to traverse the series and
> + each brightness value for the specified duration. Duration of
> + 0 means brightness should immediately change to new value.
> +
> + The format of the pattern values should be:
> + "brightness_1 duration_1 brightness_2 duration_2 brightness_3
> + duration_3 ...".
> +
> +What:/sys/class/leds//repeat
> +Date:August 2018
> +KernelVersion: 4.19
> +Description:
> + Specify a pattern repeat number. 0 means repeat indefinitely.
> +
> + This file will always return the originally written repeat
> + number.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 4018af7..b76fc3c 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -129,4 +129,11 @@ config LEDS_TRIGGER_NETDEV
> This allows LEDs to be controlled by network device activity.
> If unsure, say Y.
>
> +config LEDS_TRIGGER_PATTERN
> + tristate "LED Pattern Trigger"
> + help
> + This allows LEDs to be controlled by a software or hardware pattern
> + which is a series of tuples, of brightness and duration (ms).
> + If unsure, say N
> +
> endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index f3cfe19..9bcb64e 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)+=
> ledtrig-transient.o
> obj-$(CONFIG_LEDS_TRIGGER_CAMERA)+= ledtrig-camera.o
> obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
> obj-$(CONFIG_LEDS_TRIGGER_NETDEV)+= ledtrig-netdev.o
> +obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
> diff --git a/drivers/leds/trigger/ledtrig-pattern.c
> b/drivers/leds/trigger/ledtrig-pattern.c
> new file mode 100644
> index 000..63b94a2
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-pattern.c
> @@ -0,0 +1,266 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * LED pattern trigger
> + *
> + * Idea discussed with Pavel Machek. Raphael Teysseyre implemented
> + * the first version, Baolin Wang simplified and improved the approach.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MAX_PATTERNS 1024
> +
> +struct pattern_trig_data {
> + struct led_classdev *led_cdev;
> + struct led_pattern
