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

[Jacek Anaszewski]

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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxx>
> > +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