Hi Jacek,
On 9 August 2018 at 05:28, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> wrote:
> Hi Baolin,
>
> On 08/08/2018 08:01 AM, Baolin Wang wrote:
>> Hi Jacek,
>>
>> On 8 August 2018 at 05:54, Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> 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 <rteysseyre@xxxxxxxxx>
>>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>>>> ---
>>>> 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 0000000..bc7475f
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-pattern
>>>> @@ -0,0 +1,24 @@
>>>> +What: /sys/class/leds/<led>/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/<led>/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 0000000..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 <linux/kernel.h>
>>>> +#include <linux/leds.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/timer.h>
>>>> +
>>>> +#define MAX_PATTERNS 1024
>>>> +
>>>> +struct pattern_trig_data {
>>>> + struct led_classdev *led_cdev;
>>>> + struct led_pattern patterns[MAX_PATTERNS];
>>>> + struct led_pattern *curr;
>>>> + struct led_pattern *next;
>>>> + struct mutex lock;
>>>> + u32 npatterns;
>>>> + u32 repeat;
>>>> + u32 last_repeat;
>>>> + bool is_indefinite;
>>>> + struct timer_list timer;
>>>> +};
>>>> +
>>>> +static void pattern_trig_update_patterns(struct pattern_trig_data *data)
>>>> +{
>>>> + data->curr = data->next;
>>>> + if (!data->is_indefinite && data->curr == data->patterns)
>>>> + data->repeat--;
>>>> +
>>>> + if (data->next == data->patterns + data->npatterns - 1)
>>>> + data->next = data->patterns;
>>>> + else
>>>> + data->next++;
>>>> +}
>>>> +
>>>> +static void pattern_trig_timer_function(struct timer_list *t)
>>>> +{
>>>> + struct pattern_trig_data *data = from_timer(data, t, timer);
>>>> +
>>>> + mutex_lock(&data->lock);
>>>> +
>>>> + if (!data->is_indefinite && !data->repeat) {
>>>> + mutex_unlock(&data->lock);
>>>> + return;
>>>> + }
>>>> +
>>>> + led_set_brightness(data->led_cdev, data->curr->brightness);
>>>> + mod_timer(&data->timer, jiffies + msecs_to_jiffies(data->curr->delta_t));
>>>> + pattern_trig_update_patterns(data);
>>>> +
>>>> + mutex_unlock(&data->lock);
>>>> +}
>>>> +
>>>> +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.
>
>> So I think we should let LED driver to handle this case, if failed to
>> issue pattern_set(), the LED driver can set one group default hardware
>> patterns, or turn off the LED hardware pattern or other error handling
>> method supplied by LED driver. That will not combine software pattern
>> and hardware pattern together to make things complicated.
>>
>> So what do you think?
>>
>>> 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
>>>
>>> The same issue applies to the already available timer trigger.
>>> So far the policy implemented by the drivers implementing blink_set
>>> op varies.
>>>
>>> --
>>> Best regards,
>>> Jacek Anaszewski
>>
>>
>>
>
> --
> Best regards,
> Jacek Anaszewski
--
Baolin Wang
Best Regards
