Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

Hi,

On 05/09/2018 04:25 PM, Pavel Machek wrote:
> On Tue 2018-05-08 13:39:45, Baolin Wang wrote:
>> From: Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>>
>> This patch adds Spreadtrum SC27xx PMIC series breathing light controller
>> driver, which can support 3 LEDs. Each LED can work at normal PWM mode
>> and breathing mode.
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> new file mode 100644
>> index 0000000..22166fb
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-sc27xx
>> @@ -0,0 +1,19 @@
>> +What:		/sys/class/leds/<led>/rise_time
>> +What:		/sys/class/leds/<led>/high_time
>> +What:		/sys/class/leds/<led>/fall_time
>> +What:		/sys/class/leds/<led>/low_time
>> +Date:		May 2018
>> +KernelVersion:	4.18
>> +Contact:	Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>> +Description:
>> +		Set the pattern generator rise, high, fall and low
>> +		times (0..63). It's unit is 0.125s, it should be > 0.
>> +
>> +		1 - 125 ms
>> +		2 - 250 ms
>> +		3 - 375 ms
>> +		...
>> +		...
>> +		...
>> +		62 - 7.75 s
>> +		63 - 7.875 s
> 
> How does this interact with triggers? With manually setting
> brightness? Are the pattern generators independend for the LEDs?
> 
> Can you generate white breathing pattern? If so, how?
> 
> How do you select between normal and breathing modes?
> 
> I'd specify times in miliseconds or something, this is way too
> hardware specific.

Agreed.

> Now... functionality like this is common between many LED
> controllers. N900 could do this kind of "breathing", too, and it also
> supports other patterns.
> 
> I believe we need interface common between different LED controllers.
> 
> And I guess it would be easiest if you dropped this part from initial
> merge.

I disagree here. We already had the same discussion at the occasion
of the patch [0] and it turned out to be a dead-end [1]. Now we have
neither the driver nor the generic pattern interface.

We also already have some older LED class drivers that implement custom
pattern interfaces (e.g. drivers/leds/leds-lm3533.c) and the same
approach can be applied in this case.

Regarding interaction with triggers - we could disable pattern
on setting any trigger and return -EBUSY from the pattern
interface if led_cdev->trigger is not NULL.

[0] https://lkml.org/lkml/2017/3/23/33
[1] https://lkml.org/lkml/2017/11/15/27

-- 
Best regards,
Jacek Anaszewski