Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
From: Jacek Anaszewski <jacek.anaszewski@gmail.com>
To: Oleh Kravchenko <oleg@kaa.org.ua>,
	devicetree@vger.kernel.org, linux-leds@vger.kernel.org
Subject: Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
Date: Tue, 13 Aug 2019 22:28:41 +0200
Message-ID: <c2b006bb-2bb6-98b0-c907-190fd9fc41c7@gmail.com> (raw)
In-Reply-To: <20190808203204.8614-2-oleg@kaa.org.ua>

Hi Oleh,

Thank you for the patch set.

On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System EL15203000 LEDs board
> (aka RED LEDs board).
> 
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../testing/sysfs-class-led-driver-el15203000 |  22 +
>  drivers/leds/Kconfig                          |  13 +
>  drivers/leds/Makefile                         |   1 +
>  drivers/leds/leds-el15203000.c                | 478 ++++++++++++++++++
>  4 files changed, 514 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>  create mode 100644 drivers/leds/leds-el15203000.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-el15203000 b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> new file mode 100644
> index 000000000000..91a483e493d9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
> @@ -0,0 +1,22 @@
> +What:		/sys/class/leds/<led>/hw_pattern
> +Date:		August 2019
> +KernelVersion:	5.3
> +Description:
> +		Specify a hardware pattern for the EL15203000 LED.
> +		The LEDs board supports only predefined patterns by firmware
> +		for specific LEDs.
> +
> +		Breathing mode for Screen frame light tube:
> +		"0 4000 1 4000"
> +
> +		Cascade mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 1 800 2 800 4 800 8 800 16 800"

Why the sequence "1 800 2 800 4 800 8 800 16 800" is duplicated here?
It seems redundant. But aside of that - aren't the timings modifiable?
In other words - are these all patterns somehow configurable or they are
pre-programmed?

> +
> +		Inverted cascade mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 30 800 29 800 27 800 23 800 15 800"

Similar duplication here.

> +
> +		Bounce mode for Pipe LED:
> +		"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"

Instead of two repeating "16 800" you could provide "16 1600".
But here again is the question whether these values are configurable.
From what I can see in your driver implementation you're expecting
exactly the values provided in these examples to enable given hardware
pattern (led_pattern_cmp()).

> +
> +		Inverted bounce mode for Pipe LED:
> +		"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"

-- 
Best regards,
Jacek Anaszewski

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-08 20:32 [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
2019-08-08 20:32 ` [PATCH v4 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
2019-08-13 20:28   ` Jacek Anaszewski [this message]
2019-08-13 20:37     ` Oleh Kravchenko
2019-08-14 18:18       ` Jacek Anaszewski
2019-08-14 19:46         ` Oleh Kravchenko
2019-08-14 19:57           ` Jacek Anaszewski
2019-08-14 20:16             ` Oleh Kravchenko
2019-08-14 20:53               ` Jacek Anaszewski
2019-08-14 20:58                 ` Oleh Kravchenko
2019-08-17 15:02               ` Pavel Machek
2019-08-20  7:46                 ` Oleh Kravchenko
2019-08-18 13:47   ` Jacek Anaszewski
2019-08-27 20:43     ` Oleh Kravchenko
2019-08-27 20:56       ` Jacek Anaszewski
2019-08-27 21:11         ` Oleh Kravchenko
2019-08-27 21:18           ` Jacek Anaszewski
2019-08-27 21:26             ` Oleh Kravchenko
2019-08-13 20:31 ` [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Dan Murphy
2019-08-14 19:54   ` Jacek Anaszewski
2019-08-18 13:41 ` Jacek Anaszewski
2019-08-20  7:50   ` Oleh Kravchenko

Reply instructions:

You may reply publically 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=c2b006bb-2bb6-98b0-c907-190fd9fc41c7@gmail.com \
    --to=jacek.anaszewski@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=oleg@kaa.org.ua \
    /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

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox