LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Pavel Machek <pavel@ucw.cz>
Cc: Baolin Wang <baolin.wang@linaro.org>,
	robh+dt@kernel.org, mark.rutland@arm.com,
	xiaotong.lu@spreadtrum.com, broonie@kernel.org,
	linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver
Date: Thu, 10 May 2018 21:41:50 +0200	[thread overview]
Message-ID: <6289571e-7224-ca5e-1acf-5b099be57302@gmail.com> (raw)
In-Reply-To: <20180510113749.GG6977@amd>

Hi Pavel,

On 05/10/2018 01:37 PM, Pavel Machek wrote:
> Hi!
> 
>>>> 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.
> 
> Please don't. It was mistake to implement custom pattern interfaces
> back then, it is still mistake now.

It turned out to be really hard to cover all known pattern generator
implementations with generic interface. Sure, it would be nice to have
one, but the whole discussion around [0] only unveiled the diversity of
parameters to cover. And still new devices appear on the market.

We would have to propose a set of pattern schemes and allow to
add new ones to it.

> If we really need solution now, I'd recommend "pattern" file with
> 
> "<delta time> <brightness> <delta time> <brightness>".
> 
> In this specific case, hardware only supports patterns in this format:
> 
> low_time 0 rise_time 255 high_time 255 fall_time 0
> 
> so driver would simply -EINVAL on anything else.

I'm fine with the pattern file, but the pattern format would have
to be defined in the per-driver ABI documentation. It wouldn't much
differ from the custom pattern approach though, unless I'm missing some
gain of having pattern setting in a uniformly named single sysfs file
(with semantics differing from driver to driver).

-- 
Best regards,
Jacek Anaszewski

  reply	other threads:[~2018-05-10 19:43 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-08  5:39 [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation Baolin Wang
2018-05-08  5:39 ` [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver Baolin Wang
2018-05-08 20:54   ` Jacek Anaszewski
2018-05-09  2:34     ` Baolin Wang
2018-05-09 14:25   ` Pavel Machek
2018-05-09 19:40     ` Jacek Anaszewski
2018-05-10 11:37       ` Pavel Machek
2018-05-10 19:41         ` Jacek Anaszewski [this message]
2018-05-12  8:35           ` Pavel Machek
2018-05-12 20:44             ` Jacek Anaszewski
2018-05-13  2:19               ` Baolin Wang
2018-06-21  7:25               ` Baolin Wang
2018-05-10  3:12     ` Baolin Wang
2018-05-08 15:43 ` [PATCH v2 1/2] dt-bindings: leds: Add SC27xx breathing light controller documentation Rob Herring
2018-05-09 14:25 ` Pavel Machek
2018-05-09 20:09   ` Jacek Anaszewski
2018-05-10  1:55   ` Baolin Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6289571e-7224-ca5e-1acf-5b099be57302@gmail.com \
    --to=jacek.anaszewski@gmail.com \
    --cc=baolin.wang@linaro.org \
    --cc=broonie@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pavel@ucw.cz \
    --cc=robh+dt@kernel.org \
    --cc=xiaotong.lu@spreadtrum.com \
    --subject='Re: [PATCH v2 2/2] leds: Add Spreadtrum SC27xx breathing light controller driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).