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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bddc1f89-8c2c-ba6e-3c7e-d89710ed5424@gmail.com>
Date:   Wed, 9 May 2018 21:40:35 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Pavel Machek <pavel@....cz>, Baolin Wang <baolin.wang@...aro.org>
Cc:     robh+dt@...nel.org, mark.rutland@....com,
        xiaotong.lu@...eadtrum.com, broonie@...nel.org,
        linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light
 controller driver

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@...eadtrum.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@...eadtrum.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