* [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
@ 2019-08-08 20:32 Oleh Kravchenko
2019-08-08 20:32 ` [PATCH v4 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-08 20:32 UTC (permalink / raw)
To: devicetree, linux-leds; +Cc: Oleh Kravchenko
Add documentation and example for dt-bindings EL15203000.
LED board (aka RED LED board) from Crane Merchandising Systems.
Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
.../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
new file mode 100644
index 000000000000..4c2245babfdc
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
@@ -0,0 +1,47 @@
+Crane Merchandising System - el15203000 LED driver
+--------------------------------------------------
+
+This LED Board (aka RED LEDs board) is widely used in
+coffee vending machines produced by Crane Merchandising Systems.
+
+Required properties:
+- compatible : "crane,el15203000"
+- reg :
+ see Documentation/devicetree/bindings/spi/spi-bus.txt
+- spi-max-frequency : (optional)
+ see Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional LED sub-node properties:
+- label :
+ see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger :
+ see Documentation/devicetree/bindings/leds/common.txt
+
+Example
+-------
+
+led-controller@0 {
+ compatible = "crane,el15203000";
+ reg = <0>;
+ spi-max-frequency = <50000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ /* water pipe */
+ pipe@50 {
+ reg = <0x50>;
+ label = "red:pipe";
+ };
+
+ /* screen frame */
+ screen@53 {
+ reg = <0x53>;
+ label = "red:screen";
+ };
+
+ /* vending area */
+ vend@56 {
+ reg = <0x56>;
+ label = "red:vend";
+ };
+};
--
2.21.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-08 20:32 [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
@ 2019-08-08 20:32 ` Oleh Kravchenko
2019-08-13 20:28 ` Jacek Anaszewski
2019-08-18 13:47 ` Jacek Anaszewski
2019-08-13 20:31 ` Dan Murphy
2019-08-18 13:41 ` Jacek Anaszewski
2 siblings, 2 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-08 20:32 UTC (permalink / raw)
To: devicetree, linux-leds; +Cc: Oleh Kravchenko
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"
+
+ 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"
+
+ 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"
+
+ 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"
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 760f73a49c9f..0cbdb9ba5213 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -129,6 +129,19 @@ config LEDS_CR0014114
To compile this driver as a module, choose M here: the module
will be called leds-cr0014114.
+config LEDS_EL15203000
+ tristate "LED Support for Crane EL15203000"
+ depends on LEDS_CLASS
+ depends on SPI
+ depends on OF
+ help
+ This option enables support for EL15203000 LED Board
+ (aka RED LED board) which is widely used in coffee vending
+ machines produced by Crane Merchandising Systems.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-el15203000.
+
config LEDS_LM3530
tristate "LCD Backlight driver for LM3530"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 1e9702ebffee..1f193ffc2feb 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
+obj-$(CONFIG_LEDS_EL15203000) += leds-el15203000.o
# LED Userspace Drivers
obj-$(CONFIG_LEDS_USER) += uleds.o
diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
new file mode 100644
index 000000000000..c62da5ec6630
--- /dev/null
+++ b/drivers/leds/leds-el15203000.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Crane Merchandising Systems. All rights reserved.
+// Copyright (C) 2019 Oleh Kravchenko <oleg@kaa.org.ua>
+
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+#include <uapi/linux/uleds.h>
+
+/*
+ * EL15203000 SPI protocol description:
+ * +-----+------------+
+ * | LED | BRIGHTNESS |
+ * +-----+------------+
+ * | 1 | 1 |
+ * +-----+------------+
+ * (*) LEDs MCU board expects 20 msec delay per byte.
+ *
+ * LEDs:
+ * +----------+--------------+-------------------------------------------+
+ * | ID | NAME | DESCRIPTION |
+ * +----------+--------------+-------------------------------------------+
+ * | 'P' 0x50 | Pipe | Consists from 5 LEDs, controlled by board |
+ * +----------+--------------+-------------------------------------------+
+ * | 'S' 0x53 | Screen frame | Light tube around the screen |
+ * +----------+--------------+-------------------------------------------+
+ * | 'V' 0x56 | Vending area | Highlights a cup of coffee |
+ * +----------+--------------+-------------------------------------------+
+ *
+ * BRIGHTNESS:
+ * +----------+-----------------+--------------+--------------+
+ * | VALUES | PIPE | SCREEN FRAME | VENDING AREA |
+ * +----------+-----------------+--------------+--------------+
+ * | '0' 0x30 | Off |
+ * +----------+-----------------------------------------------+
+ * | '1' 0x31 | On |
+ * +----------+-----------------+--------------+--------------+
+ * | '2' 0x32 | Cascade | Breathing |
+ * +----------+-----------------+--------------+
+ * | '3' 0x33 | Inverse cascade |
+ * +----------+-----------------+
+ * | '4' 0x34 | Bounce |
+ * +----------+-----------------+
+ * | '5' 0x35 | Inverse bounce |
+ * +----------+-----------------+
+ */
+
+/* EL15203000 default settings */
+#define EL_FW_DELAY_USEC 20000
+
+enum el15203000_brightness {
+ /* for all LEDs */
+ EL_OFF = '0',
+ EL_ON = '1',
+
+ /* for Screen LED */
+ EL_SCREEN_BREATHING = '2',
+
+ /* for Pipe LED */
+ EL_PIPE_CASCADE = '2',
+ EL_PIPE_INV_CASCADE = '3',
+ EL_PIPE_BOUNCE = '4',
+ EL_PIPE_INV_BOUNCE = '5',
+};
+
+struct el15203000_led {
+ struct el15203000 *priv;
+ struct led_classdev ldev;
+ char name[LED_MAX_NAME_SIZE];
+ u8 reg;
+};
+
+struct el15203000 {
+ struct device *dev;
+ struct mutex lock;
+ struct spi_device *spi;
+ unsigned long delay;
+ size_t count;
+ struct el15203000_led leds[];
+};
+
+static int el15203000_cmd(struct el15203000_led *led, u8 brightness)
+{
+ int ret;
+ u8 cmd[2];
+ size_t i;
+ unsigned long jdelay, udelay, now;
+
+ mutex_lock(&led->priv->lock);
+
+ dev_dbg(led->priv->dev, "Set brightness of %s to %d",
+ led->name, brightness);
+
+ /* to avoid SPI mistiming with firmware we should wait some time */
+ now = jiffies;
+ if (time_after(led->priv->delay, now)) {
+ jdelay = led->priv->delay - now;
+ udelay = jiffies_to_usecs(jdelay);
+
+ dev_dbg(led->priv->dev, "Wait %luus (%lu jiffies) to sync",
+ udelay, jdelay);
+
+ usleep_range(udelay, udelay + 1);
+ }
+
+ cmd[0] = led->reg;
+ cmd[1] = brightness;
+
+ for (i = 0; i < ARRAY_SIZE(cmd); i++) {
+ if (i)
+ usleep_range(EL_FW_DELAY_USEC,
+ EL_FW_DELAY_USEC + 1);
+
+ ret = spi_write(led->priv->spi, &cmd[i], sizeof(cmd[i]));
+ if (ret) {
+ dev_err(led->priv->dev,
+ "spi_write() error %d", ret);
+ break;
+ }
+ }
+
+ led->priv->delay = jiffies + usecs_to_jiffies(EL_FW_DELAY_USEC);
+
+ mutex_unlock(&led->priv->lock);
+
+ return ret;
+}
+
+static int el15203000_set_blocking(struct led_classdev *ldev,
+ enum led_brightness brightness)
+{
+ struct el15203000_led *led = container_of(ldev,
+ struct el15203000_led,
+ ldev);
+
+ return el15203000_cmd(led, brightness == LED_OFF ? EL_OFF : EL_ON);
+}
+
+static int led_pattern_cmp(const struct led_pattern *p1, u32 p1_len,
+ const struct led_pattern *p2, u32 p2_len)
+{
+ u32 i;
+
+ if (p1_len != p2_len)
+ return -1;
+
+ for (i = 0; i < p1_len; i ++)
+ if (p1[i].delta_t != p2[i].delta_t ||
+ p1[i].brightness != p2[i].brightness)
+ return -1;
+
+ return 0;
+}
+
+int el15203000_pattern_set_S(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ u32 len, int repeat)
+{
+ struct el15203000_led *led = container_of(ldev,
+ struct el15203000_led,
+ ldev);
+ struct led_pattern scr_pattern[] = {{
+ .delta_t = 4000,
+ .brightness = 0,
+ }, {
+ .delta_t = 4000,
+ .brightness = 1,
+ }};
+
+ if (repeat > 0)
+ return -EINVAL;
+
+ if (led_pattern_cmp(scr_pattern, ARRAY_SIZE(scr_pattern), pattern, len))
+ return -EINVAL;
+
+ dev_dbg(led->priv->dev, "Breathing mode for '%s'", led->name);
+
+ return el15203000_cmd(led, EL_SCREEN_BREATHING);
+}
+
+int el15203000_pattern_set_P(struct led_classdev *ldev,
+ struct led_pattern *pattern,
+ u32 len, int repeat)
+{
+ struct el15203000_led *led = container_of(ldev,
+ struct el15203000_led,
+ ldev);
+
+ struct led_pattern cascade[] = {{
+ .delta_t = 800,
+ .brightness = 1,
+ }, {
+ .delta_t = 800,
+ .brightness = 2,
+ }, {
+ .delta_t = 800,
+ .brightness = 4,
+ }, {
+ .delta_t = 800,
+ .brightness = 8,
+ }, {
+ .delta_t = 800,
+ .brightness = 16,
+ }, {
+ .delta_t = 800,
+ .brightness = 1,
+ }, {
+ .delta_t = 800,
+ .brightness = 2,
+ }, {
+ .delta_t = 800,
+ .brightness = 4,
+ }, {
+ .delta_t = 800,
+ .brightness = 8,
+ }, {
+ .delta_t = 800,
+ .brightness = 16,
+ }};
+
+ struct led_pattern inv_cascade[] = {{
+ .delta_t = 800,
+ .brightness = 30,
+ }, {
+ .delta_t = 800,
+ .brightness = 29,
+ }, {
+ .delta_t = 800,
+ .brightness = 27,
+ }, {
+ .delta_t = 800,
+ .brightness = 23,
+ }, {
+ .delta_t = 800,
+ .brightness = 15,
+ }, {
+ .delta_t = 800,
+ .brightness = 30,
+ }, {
+ .delta_t = 800,
+ .brightness = 29,
+ }, {
+ .delta_t = 800,
+ .brightness = 27,
+ }, {
+ .delta_t = 800,
+ .brightness = 23,
+ }, {
+ .delta_t = 800,
+ .brightness = 15,
+ }};
+
+ struct led_pattern bounce[] = {{
+ .delta_t = 800,
+ .brightness = 1,
+ }, {
+ .delta_t = 800,
+ .brightness = 2,
+ }, {
+ .delta_t = 800,
+ .brightness = 4,
+ }, {
+ .delta_t = 800,
+ .brightness = 8,
+ }, {
+ .delta_t = 800,
+ .brightness = 16,
+ }, {
+ .delta_t = 800,
+ .brightness = 16,
+ }, {
+ .delta_t = 800,
+ .brightness = 8,
+ }, {
+ .delta_t = 800,
+ .brightness = 4,
+ }, {
+ .delta_t = 800,
+ .brightness = 2,
+ }, {
+ .delta_t = 800,
+ .brightness = 1,
+ }};
+
+ struct led_pattern inv_bounce[] = {{
+ .delta_t = 800,
+ .brightness = 30,
+ }, {
+ .delta_t = 800,
+ .brightness = 29,
+ }, {
+ .delta_t = 800,
+ .brightness = 27,
+ }, {
+ .delta_t = 800,
+ .brightness = 23,
+ }, {
+ .delta_t = 800,
+ .brightness = 15,
+ }, {
+ .delta_t = 800,
+ .brightness = 15,
+ }, {
+ .delta_t = 800,
+ .brightness = 23,
+ }, {
+ .delta_t = 800,
+ .brightness = 27,
+ }, {
+ .delta_t = 800,
+ .brightness = 29,
+ }, {
+ .delta_t = 800,
+ .brightness = 30,
+ }};
+
+ if (repeat > 0)
+ return -EINVAL;
+
+ if (!led_pattern_cmp(cascade, ARRAY_SIZE(cascade), pattern, len)) {
+ dev_dbg(led->priv->dev, "Cascade mode for '%s'", led->name);
+ return el15203000_cmd(led, EL_PIPE_CASCADE);
+ } else if (!led_pattern_cmp(inv_cascade, ARRAY_SIZE(inv_cascade), pattern, len)) {
+ dev_dbg(led->priv->dev, "Inverse cascade mode for '%s'", led->name);
+ return el15203000_cmd(led, EL_PIPE_INV_CASCADE);
+ } else if (!led_pattern_cmp(bounce, ARRAY_SIZE(bounce), pattern, len)) {
+ dev_dbg(led->priv->dev, "Bounce mode for '%s'", led->name);
+ return el15203000_cmd(led, EL_PIPE_BOUNCE);
+ } else if (!led_pattern_cmp(inv_bounce, ARRAY_SIZE(inv_bounce), pattern, len)) {
+ dev_dbg(led->priv->dev, "Inverse bounce mode for '%s'", led->name);
+ return el15203000_cmd(led, EL_PIPE_INV_BOUNCE);
+ }
+
+ return -EINVAL;
+}
+
+int el15203000_pattern_clear(struct led_classdev *ldev)
+{
+ struct el15203000_led *led = container_of(ldev,
+ struct el15203000_led,
+ ldev);
+
+ return el15203000_cmd(led, EL_OFF);
+}
+
+static int el15203000_probe_dt(struct el15203000 *priv)
+{
+ size_t i = 0;
+ struct el15203000_led *led;
+ struct fwnode_handle *child;
+ int ret;
+ const char *str;
+ u32 reg;
+
+ device_for_each_child_node(priv->dev, child) {
+ led = &priv->leds[i];
+
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret) {
+ dev_err(priv->dev, "LED without ID number");
+ fwnode_handle_put(child);
+
+ return ret;
+ }
+
+ if (reg > U8_MAX) {
+ dev_err(priv->dev, "LED value %d is invalid", reg);
+ fwnode_handle_put(child);
+
+ return -EINVAL;
+ }
+
+ led->reg = reg;
+
+ ret = fwnode_property_read_string(child, "label", &str);
+ if (ret)
+ str = ":";
+ snprintf(led->name, sizeof(led->name), "el15203000:%s", str);
+
+ fwnode_property_read_string(child, "linux,default-trigger",
+ &led->ldev.default_trigger);
+
+ led->ldev.max_brightness = LED_ON;
+ led->priv = priv;
+ led->ldev.name = led->name;
+ led->ldev.brightness_set_blocking = el15203000_set_blocking;
+
+ if (reg == 'S') {
+ led->ldev.pattern_set = el15203000_pattern_set_S;
+ led->ldev.pattern_clear = el15203000_pattern_clear;
+ } else if (reg == 'P') {
+ led->ldev.pattern_set = el15203000_pattern_set_P;
+ led->ldev.pattern_clear = el15203000_pattern_clear;
+ }
+
+ ret = devm_led_classdev_register(priv->dev, &led->ldev);
+ if (ret) {
+ dev_err(priv->dev,
+ "failed to register LED device %s, err %d",
+ led->name, ret);
+ fwnode_handle_put(child);
+ return ret;
+ }
+
+ i++;
+ }
+
+ return ret;
+}
+
+static int el15203000_probe(struct spi_device *spi)
+{
+ struct el15203000 *priv;
+ size_t count;
+ int ret;
+
+ count = device_get_child_node_count(&spi->dev);
+ if (!count) {
+ dev_err(&spi->dev, "LEDs are not defined in device tree!");
+ return -ENODEV;
+ }
+
+ priv = devm_kzalloc(&spi->dev, struct_size(priv, leds, count),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ mutex_init(&priv->lock);
+ priv->count = count;
+ priv->dev = &spi->dev;
+ priv->spi = spi;
+ priv->delay = jiffies -
+ usecs_to_jiffies(EL_FW_DELAY_USEC);
+
+ ret = el15203000_probe_dt(priv);
+ if (ret)
+ return ret;
+
+ spi_set_drvdata(spi, priv);
+ dev_dbg(priv->dev, "%zd LEDs registered", priv->count);
+
+ return 0;
+}
+
+static int el15203000_remove(struct spi_device *spi)
+{
+ struct el15203000 *priv = spi_get_drvdata(spi);
+
+ mutex_destroy(&priv->lock);
+
+ return 0;
+}
+
+static const struct of_device_id el15203000_dt_ids[] = {
+ { .compatible = "crane,el15203000", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, el15203000_dt_ids);
+
+static struct spi_driver el15203000_driver = {
+ .probe = el15203000_probe,
+ .remove = el15203000_remove,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = el15203000_dt_ids,
+ },
+};
+
+module_spi_driver(el15203000_driver);
+
+MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
+MODULE_DESCRIPTION("el15203000 LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:el15203000");
--
2.21.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
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
2019-08-18 13:47 ` Jacek Anaszewski
1 sibling, 0 replies; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-13 20:28 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
@ 2019-08-13 20:28 ` Jacek Anaszewski
0 siblings, 0 replies; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-13 20:28 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
2019-08-08 20:32 [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
@ 2019-08-13 20:31 ` Dan Murphy
2019-08-13 20:31 ` Dan Murphy
2019-08-18 13:41 ` Jacek Anaszewski
2 siblings, 0 replies; 24+ messages in thread
From: Dan Murphy @ 2019-08-13 20:31 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
Jacek
Need your input below
On 8/8/19 3:32 PM, Oleh Kravchenko wrote:
> Add documentation and example for dt-bindings EL15203000.
> LED board (aka RED LED board) from Crane Merchandising Systems.
>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
> .../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> new file mode 100644
> index 000000000000..4c2245babfdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,47 @@
> +Crane Merchandising System - el15203000 LED driver
> +--------------------------------------------------
> +
> +This LED Board (aka RED LEDs board) is widely used in
> +coffee vending machines produced by Crane Merchandising Systems.
> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- reg :
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-max-frequency : (optional)
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional LED sub-node properties:
> +- label :
> + see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger :
> + see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +-------
> +
> +led-controller@0 {
> + compatible = "crane,el15203000";
> + reg = <0>;
> + spi-max-frequency = <50000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* water pipe */
> + pipe@50 {
> + reg = <0x50>;
> + label = "red:pipe";
Should we use the color and function property here?
Not sure what function would be for pipe, screen or vending but there may be
comparable functions that may fit.
Dan
<snip>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
@ 2019-08-13 20:31 ` Dan Murphy
0 siblings, 0 replies; 24+ messages in thread
From: Dan Murphy @ 2019-08-13 20:31 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
Jacek
Need your input below
On 8/8/19 3:32 PM, Oleh Kravchenko wrote:
> Add documentation and example for dt-bindings EL15203000.
> LED board (aka RED LED board) from Crane Merchandising Systems.
>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
> .../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> new file mode 100644
> index 000000000000..4c2245babfdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,47 @@
> +Crane Merchandising System - el15203000 LED driver
> +--------------------------------------------------
> +
> +This LED Board (aka RED LEDs board) is widely used in
> +coffee vending machines produced by Crane Merchandising Systems.
> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- reg :
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-max-frequency : (optional)
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional LED sub-node properties:
> +- label :
> + see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger :
> + see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +-------
> +
> +led-controller@0 {
> + compatible = "crane,el15203000";
> + reg = <0>;
> + spi-max-frequency = <50000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* water pipe */
> + pipe@50 {
> + reg = <0x50>;
> + label = "red:pipe";
Should we use the color and function property here?
Not sure what function would be for pipe, screen or vending but there may be
comparable functions that may fit.
Dan
<snip>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-13 20:28 ` Jacek Anaszewski
(?)
@ 2019-08-13 20:37 ` Oleh Kravchenko
2019-08-14 18:18 ` Jacek Anaszewski
-1 siblings, 1 reply; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-13 20:37 UTC (permalink / raw)
To: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1996 bytes --]
Hello Jacek,
Thank you for your useful review,
13.08.19 23:28, Jacek Anaszewski пише:
> Hi Oleh,
>
> Thank you for the patch set.
>
> On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
>> +++ 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?
>
All pattern is predefined, you can't change them at all.
I just tried to describe real things what happened in LED board.
It's ticks every 800 milliseconds for Pipe LEDs.
>> +
>> + 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"
>
Should I cut this patterns to smaller? Or let keep it?
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-13 20:37 ` Oleh Kravchenko
@ 2019-08-14 18:18 ` Jacek Anaszewski
2019-08-14 19:46 ` Oleh Kravchenko
0 siblings, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-14 18:18 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
Hi Oleh,
On 8/13/19 10:37 PM, Oleh Kravchenko wrote:
> Hello Jacek,
>
> Thank you for your useful review,
>
> 13.08.19 23:28, Jacek Anaszewski пише:
>> Hi Oleh,
>>
>> Thank you for the patch set.
>>
>> On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
>>> +++ 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?
>>
>
> All pattern is predefined, you can't change them at all.
> I just tried to describe real things what happened in LED board.
> It's ticks every 800 milliseconds for Pipe LEDs.
It makes me wonder how you figured out the values? If you have
a documentation for this controller, could you share how the pattern
settings are documented?
>>> +
>>> + 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"
>>
>
> Should I cut this patterns to smaller? Or let keep it?
>
For the first two we could do without sequence duplication.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-14 18:18 ` Jacek Anaszewski
@ 2019-08-14 19:46 ` Oleh Kravchenko
2019-08-14 19:57 ` Jacek Anaszewski
0 siblings, 1 reply; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-14 19:46 UTC (permalink / raw)
To: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1732 bytes --]
Hello Jacek,
14.08.19 21:18, Jacek Anaszewski пише:
>>> 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?
>>>
>>
>> All pattern is predefined, you can't change them at all.
>> I just tried to describe real things what happened in LED board.
>> It's ticks every 800 milliseconds for Pipe LEDs.
>
> It makes me wonder how you figured out the values? If you have
> a documentation for this controller, could you share how the pattern
> settings are documented?
I saw the code of firmware.
Not sure if I can find any documentation for it right now.
>>>> +
>>>> + 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"
>>>
>>
>> Should I cut this patterns to smaller? Or let keep it?
>>
>
> For the first two we could do without sequence duplication.
Ok, I will reduce it.
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
2019-08-13 20:31 ` Dan Murphy
(?)
@ 2019-08-14 19:54 ` Jacek Anaszewski
-1 siblings, 0 replies; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-14 19:54 UTC (permalink / raw)
To: Dan Murphy, Oleh Kravchenko, devicetree, linux-leds
Dan,
On 8/13/19 10:31 PM, Dan Murphy wrote:
> Jacek
>
> Need your input below
>
> On 8/8/19 3:32 PM, Oleh Kravchenko wrote:
>> Add documentation and example for dt-bindings EL15203000.
>> LED board (aka RED LED board) from Crane Merchandising Systems.
>>
>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>> ---
>> .../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/leds/leds-el15203000.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> new file mode 100644
>> index 000000000000..4c2245babfdc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> @@ -0,0 +1,47 @@
>> +Crane Merchandising System - el15203000 LED driver
>> +--------------------------------------------------
>> +
>> +This LED Board (aka RED LEDs board) is widely used in
>> +coffee vending machines produced by Crane Merchandising Systems.
>> +
>> +Required properties:
>> +- compatible : "crane,el15203000"
>> +- reg :
>> + see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +- spi-max-frequency : (optional)
>> + see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Optional LED sub-node properties:
>> +- label :
>> + see Documentation/devicetree/bindings/leds/common.txt
>> +- linux,default-trigger :
>> + see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example
>> +-------
>> +
>> +led-controller@0 {
>> + compatible = "crane,el15203000";
>> + reg = <0>;
>> + spi-max-frequency = <50000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + /* water pipe */
>> + pipe@50 {
>> + reg = <0x50>;
>> + label = "red:pipe";
>
> Should we use the color and function property here?
Yes, label is already deprecated in the common LED bindings
in linux-next. We need separate color and function here.
> Not sure what function would be for pipe, screen or vending but there
> may be
>
> comparable functions that may fit.
These are functions specific to this board and it is not something
that could have some vast system-wide use. Common LED functions aim
rather to unify the naming for LEDs on standard system devices and
peripherals.
I'd not strive for creating common LED_FUNCTION macros for these.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-14 19:46 ` Oleh Kravchenko
@ 2019-08-14 19:57 ` Jacek Anaszewski
2019-08-14 20:16 ` Oleh Kravchenko
0 siblings, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-14 19:57 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
On 8/14/19 9:46 PM, Oleh Kravchenko wrote:
> Hello Jacek,
>
> 14.08.19 21:18, Jacek Anaszewski пише:
>>>> 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?
>>>>
>>>
>>> All pattern is predefined, you can't change them at all.
>>> I just tried to describe real things what happened in LED board.
>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>
>> It makes me wonder how you figured out the values? If you have
>> a documentation for this controller, could you share how the pattern
>> settings are documented?
>
> I saw the code of firmware.
> Not sure if I can find any documentation for it right now.
Have you tried to alter the values? Or check what happens when
the duplication is removed?
>>>>> +
>>>>> + 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"
>>>>
>>>
>>> Should I cut this patterns to smaller? Or let keep it?
>>>
>>
>> For the first two we could do without sequence duplication.
>
> Ok, I will reduce it.
Please hold on for a while. I will have some more remarks to the driver,
just collecting missing info for now to gain more complete view on this
device.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-14 19:57 ` Jacek Anaszewski
@ 2019-08-14 20:16 ` Oleh Kravchenko
2019-08-14 20:53 ` Jacek Anaszewski
2019-08-17 15:02 ` Pavel Machek
0 siblings, 2 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-14 20:16 UTC (permalink / raw)
To: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1627 bytes --]
Hello Jacek,
14.08.19 22:57, Jacek Anaszewski пише:
>>>>
>>>> All pattern is predefined, you can't change them at all.
>>>> I just tried to describe real things what happened in LED board.
>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>
>>> It makes me wonder how you figured out the values? If you have
>>> a documentation for this controller, could you share how the pattern
>>> settings are documented?
>>
>> I saw the code of firmware.
>> Not sure if I can find any documentation for it right now.
>
> Have you tried to alter the values? Or check what happens when
> the duplication is removed?
What do you mean alter? It doesn't make any sense.
Board is accepts only brightness level from '0' to '5'.
I'm really confused :-)
>>>
>>> For the first two we could do without sequence duplication.
>>
>> Ok, I will reduce it.
>
> Please hold on for a while. I will have some more remarks to the driver,
> just collecting missing info for now to gain more complete view on this
> device.
Here is the full story:
EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
It's provide access to 3 LEDs:
- First LED (Screen) is a light tube around big 21" screen
It's have 3 brightness levels:
* OFF
* ON
* Breathing mode (8 seconds full cycle)
- Second LED (Vending area) is highlight coffee cap
* OFF
* ON
- Third LED (Pipe) is actually virtual, because consists from 5 LEDs
* OFF for all 5 LEDs
* ON for all 5 LEDs
* Cascade
* Inverses cascade
* Bounce
* Inverses bounce
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
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
1 sibling, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-14 20:53 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
On 8/14/19 10:16 PM, Oleh Kravchenko wrote:
> Hello Jacek,
>
> 14.08.19 22:57, Jacek Anaszewski пише:
>>>>>
>>>>> All pattern is predefined, you can't change them at all.
>>>>> I just tried to describe real things what happened in LED board.
>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>
>>>> It makes me wonder how you figured out the values? If you have
>>>> a documentation for this controller, could you share how the pattern
>>>> settings are documented?
>>>
>>> I saw the code of firmware.
>>> Not sure if I can find any documentation for it right now.
>>
>> Have you tried to alter the values? Or check what happens when
>> the duplication is removed?
>
> What do you mean alter? It doesn't make any sense.
> Board is accepts only brightness level from '0' to '5'.
> I'm really confused :-)
OK, I didn't analyze the driver thoroughly enough.
Now everything is clear to me. Patterns are triggered just
by writing two-byte command.
>>>>
>>>> For the first two we could do without sequence duplication.
>>>
>>> Ok, I will reduce it.
>>
>> Please hold on for a while. I will have some more remarks to the driver,
>> just collecting missing info for now to gain more complete view on this
>> device.
>
> Here is the full story:
>
> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
> It's provide access to 3 LEDs:
>
> - First LED (Screen) is a light tube around big 21" screen
> It's have 3 brightness levels:
> * OFF
> * ON
> * Breathing mode (8 seconds full cycle)
OK, so this is LED string. We would need to bend rules to make it
appearing as a single LED class device, but allowing non-synchronous
blinking of LEDs in the string.
> - Second LED (Vending area) is highlight coffee cap
> * OFF
> * ON
> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
> * OFF for all 5 LEDs
> * ON for all 5 LEDs
> * Cascade
> * Inverses cascade
> * Bounce
> * Inverses bounce
Similarly in case of this one. I will give you a feedback on how
to define the patterns within few days.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-14 20:53 ` Jacek Anaszewski
@ 2019-08-14 20:58 ` Oleh Kravchenko
0 siblings, 0 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-14 20:58 UTC (permalink / raw)
To: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 2422 bytes --]
By the way,
14.08.19 23:53, Jacek Anaszewski пише:
> On 8/14/19 10:16 PM, Oleh Kravchenko wrote:
>> Hello Jacek,
>>
>> 14.08.19 22:57, Jacek Anaszewski пише:
>>>>>>
>>>>>> All pattern is predefined, you can't change them at all.
>>>>>> I just tried to describe real things what happened in LED board.
>>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>>
>>>>> It makes me wonder how you figured out the values? If you have
>>>>> a documentation for this controller, could you share how the pattern
>>>>> settings are documented?
>>>>
>>>> I saw the code of firmware.
>>>> Not sure if I can find any documentation for it right now.
>>>
>>> Have you tried to alter the values? Or check what happens when
>>> the duplication is removed?
>>
>> What do you mean alter? It doesn't make any sense.
>> Board is accepts only brightness level from '0' to '5'.
>> I'm really confused :-)
>
> OK, I didn't analyze the driver thoroughly enough.
> Now everything is clear to me. Patterns are triggered just
> by writing two-byte command.
>
>>>>>
>>>>> For the first two we could do without sequence duplication.
>>>>
>>>> Ok, I will reduce it.
>>>
>>> Please hold on for a while. I will have some more remarks to the driver,
>>> just collecting missing info for now to gain more complete view on this
>>> device.
>>
>> Here is the full story:
>>
>> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
>> It's provide access to 3 LEDs:
>>
>> - First LED (Screen) is a light tube around big 21" screen
>> It's have 3 brightness levels:
>> * OFF
>> * ON
>> * Breathing mode (8 seconds full cycle)
>
> OK, so this is LED string. We would need to bend rules to make it
> appearing as a single LED class device, but allowing non-synchronous
> blinking of LEDs in the string.
>
>> - Second LED (Vending area) is highlight coffee cap
>> * OFF
>> * ON
>> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>> * OFF for all 5 LEDs
>> * ON for all 5 LEDs
>> * Cascade
>> * Inverses cascade
>> * Bounce
>> * Inverses bounce
Full cycle takes 8 seconds too.
So actually it's splitted into 10 stages, each takes 800ms.
> Similarly in case of this one. I will give you a feedback on how
> to define the patterns within few days.
>
Take you time.
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-14 20:16 ` Oleh Kravchenko
2019-08-14 20:53 ` Jacek Anaszewski
@ 2019-08-17 15:02 ` Pavel Machek
2019-08-20 7:46 ` Oleh Kravchenko
1 sibling, 1 reply; 24+ messages in thread
From: Pavel Machek @ 2019-08-17 15:02 UTC (permalink / raw)
To: Oleh Kravchenko; +Cc: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1: Type: text/plain, Size: 1902 bytes --]
Hi!
> >>>> All pattern is predefined, you can't change them at all.
> >>>> I just tried to describe real things what happened in LED board.
> >>>> It's ticks every 800 milliseconds for Pipe LEDs.
> >>>
> >>> It makes me wonder how you figured out the values? If you have
> >>> a documentation for this controller, could you share how the pattern
> >>> settings are documented?
> >>
> >> I saw the code of firmware.
> >> Not sure if I can find any documentation for it right now.
> >
> > Have you tried to alter the values? Or check what happens when
> > the duplication is removed?
>
> What do you mean alter? It doesn't make any sense.
> Board is accepts only brightness level from '0' to '5'.
> I'm really confused :-)
For the record, what you did seems ok to me.
> >> Ok, I will reduce it.
> >
> > Please hold on for a while. I will have some more remarks to the driver,
> > just collecting missing info for now to gain more complete view on this
> > device.
>
> Here is the full story:
>
> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
> It's provide access to 3 LEDs:
>
> - First LED (Screen) is a light tube around big 21" screen
> It's have 3 brightness levels:
> * OFF
> * ON
> * Breathing mode (8 seconds full cycle)
> - Second LED (Vending area) is highlight coffee cap
> * OFF
> * ON
Again, this is ok.
> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
> * OFF for all 5 LEDs
> * ON for all 5 LEDs
> * Cascade
> * Inverses cascade
> * Bounce
> * Inverses bounce
But having one virtual LED that is really five LEDs scares me a tiny
bit. But I guess its ok for your case...
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
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:31 ` Dan Murphy
@ 2019-08-18 13:41 ` Jacek Anaszewski
2019-08-20 7:50 ` Oleh Kravchenko
2 siblings, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-18 13:41 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
Hi Oleh,
On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
> Add documentation and example for dt-bindings EL15203000.
> LED board (aka RED LED board) from Crane Merchandising Systems.
>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
> .../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
> 1 file changed, 47 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> new file mode 100644
> index 000000000000..4c2245babfdc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,47 @@
> +Crane Merchandising System - el15203000 LED driver
> +--------------------------------------------------
> +
> +This LED Board (aka RED LEDs board) is widely used in
> +coffee vending machines produced by Crane Merchandising Systems.
> +
> +Required properties:
> +- compatible : "crane,el15203000"
> +- reg :
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-max-frequency : (optional)
> + see Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional LED sub-node properties:
> +- label :
> + see Documentation/devicetree/bindings/leds/common.txt
Please change this label description to the below:
- function: see Documentation/devicetree/bindings/leds/common.txt.
- color: See Documentation/devicetree/bindings/leds/common.txt.
- label: See Documentation/devicetree/bindings/leds/common.txt (deprecated).
> +- linux,default-trigger :
> + see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example
> +-------
#include <dt-bindings/leds/common.h>
> +led-controller@0 {
> + compatible = "crane,el15203000";
> + reg = <0>;
> + spi-max-frequency = <50000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + /* water pipe */
> + pipe@50 {
s/pipe/led/
> + reg = <0x50>;
> + label = "red:pipe";
label is now deprecated.
Please use function and color:
function = "pipe";
color = <LED_COLOR_ID_RED>;
> + };
> +
> + /* screen frame */
> + screen@53 {
s/screen/led/
> + reg = <0x53>;
> + label = "red:screen";
function = "screen";
color = <LED_COLOR_ID_RED>;
> + };
> +
> + /* vending area */
> + vend@56 {
s/vend/led/
> + reg = <0x56>;
> + label = "red:vend";
function = "vend";
color = <LED_COLOR_ID_RED>;
> + };
> +};
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
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
@ 2019-08-18 13:47 ` Jacek Anaszewski
2019-08-27 20:43 ` Oleh Kravchenko
1 sibling, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-18 13:47 UTC (permalink / raw)
To: Oleh Kravchenko, devicetree, linux-leds
Hi Oleh,
Please find my review remarks below.
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"
There's no point in repeating same sequence. Just require to provide it
once:
"1 800 2 800 4 800 8 800 16 800"
> +
> + 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"
The same story:
"30 800 29 800 27 800 23 800 15 800"
> +
> + 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"
> +
> + 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"
These can be left as they are now.
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 760f73a49c9f..0cbdb9ba5213 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -129,6 +129,19 @@ config LEDS_CR0014114
> To compile this driver as a module, choose M here: the module
> will be called leds-cr0014114.
>
> +config LEDS_EL15203000
> + tristate "LED Support for Crane EL15203000"
> + depends on LEDS_CLASS
> + depends on SPI
> + depends on OF
> + help
> + This option enables support for EL15203000 LED Board
> + (aka RED LED board) which is widely used in coffee vending
> + machines produced by Crane Merchandising Systems.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called leds-el15203000.
> +
> config LEDS_LM3530
> tristate "LCD Backlight driver for LM3530"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 1e9702ebffee..1f193ffc2feb 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> +obj-$(CONFIG_LEDS_EL15203000) += leds-el15203000.o
>
> # LED Userspace Drivers
> obj-$(CONFIG_LEDS_USER) += uleds.o
> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
> new file mode 100644
> index 000000000000..c62da5ec6630
> --- /dev/null
> +++ b/drivers/leds/leds-el15203000.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Crane Merchandising Systems. All rights reserved.
> +// Copyright (C) 2019 Oleh Kravchenko <oleg@kaa.org.ua>
> +
> +#include <linux/delay.h>
> +#include <linux/leds.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <uapi/linux/uleds.h>
> +
> +/*
> + * EL15203000 SPI protocol description:
> + * +-----+------------+
> + * | LED | BRIGHTNESS |
> + * +-----+------------+
> + * | 1 | 1 |
> + * +-----+------------+
> + * (*) LEDs MCU board expects 20 msec delay per byte.
> + *
> + * LEDs:
> + * +----------+--------------+-------------------------------------------+
> + * | ID | NAME | DESCRIPTION |
> + * +----------+--------------+-------------------------------------------+
> + * | 'P' 0x50 | Pipe | Consists from 5 LEDs, controlled by board |
> + * +----------+--------------+-------------------------------------------+
> + * | 'S' 0x53 | Screen frame | Light tube around the screen |
> + * +----------+--------------+-------------------------------------------+
> + * | 'V' 0x56 | Vending area | Highlights a cup of coffee |
> + * +----------+--------------+-------------------------------------------+
> + *
> + * BRIGHTNESS:
> + * +----------+-----------------+--------------+--------------+
> + * | VALUES | PIPE | SCREEN FRAME | VENDING AREA |
> + * +----------+-----------------+--------------+--------------+
> + * | '0' 0x30 | Off |
> + * +----------+-----------------------------------------------+
> + * | '1' 0x31 | On |
> + * +----------+-----------------+--------------+--------------+
> + * | '2' 0x32 | Cascade | Breathing |
> + * +----------+-----------------+--------------+
> + * | '3' 0x33 | Inverse cascade |
> + * +----------+-----------------+
> + * | '4' 0x34 | Bounce |
> + * +----------+-----------------+
> + * | '5' 0x35 | Inverse bounce |
> + * +----------+-----------------+
> + */
> +
> +/* EL15203000 default settings */
> +#define EL_FW_DELAY_USEC 20000
> +
> +enum el15203000_brightness {
Please change 'brightness' to 'command' or so. Brightness is misleading
since this device supports in fact only one level of brightness for
non-pattern use case.
> + /* for all LEDs */
> + EL_OFF = '0',
> + EL_ON = '1',
> +
> + /* for Screen LED */
> + EL_SCREEN_BREATHING = '2',
> +
> + /* for Pipe LED */
> + EL_PIPE_CASCADE = '2',
> + EL_PIPE_INV_CASCADE = '3',
> + EL_PIPE_BOUNCE = '4',
> + EL_PIPE_INV_BOUNCE = '5',
> +};
> +
> +struct el15203000_led {
> + struct el15203000 *priv;
> + struct led_classdev ldev;
> + char name[LED_MAX_NAME_SIZE];
This field will not be needed if we switch to
devm_led_classdev_register_ext().
> + u8 reg;
> +};
> +
> +struct el15203000 {
> + struct device *dev;
> + struct mutex lock;
> + struct spi_device *spi;
> + unsigned long delay;
> + size_t count;
> + struct el15203000_led leds[];
> +};
> +
> +static int el15203000_cmd(struct el15203000_led *led, u8 brightness)
> +{
> + int ret;
> + u8 cmd[2];
> + size_t i;
> + unsigned long jdelay, udelay, now;
> +
> + mutex_lock(&led->priv->lock);
> +
> + dev_dbg(led->priv->dev, "Set brightness of %s to %d",
> + led->name, brightness);
> +
> + /* to avoid SPI mistiming with firmware we should wait some time */
> + now = jiffies;
> + if (time_after(led->priv->delay, now)) {
> + jdelay = led->priv->delay - now;
> + udelay = jiffies_to_usecs(jdelay);
> +
> + dev_dbg(led->priv->dev, "Wait %luus (%lu jiffies) to sync",
> + udelay, jdelay);
> +
> + usleep_range(udelay, udelay + 1);
> + }
> +
> + cmd[0] = led->reg;
> + cmd[1] = brightness;
> +
> + for (i = 0; i < ARRAY_SIZE(cmd); i++) {
> + if (i)
> + usleep_range(EL_FW_DELAY_USEC,
> + EL_FW_DELAY_USEC + 1);
> +
> + ret = spi_write(led->priv->spi, &cmd[i], sizeof(cmd[i]));
> + if (ret) {
> + dev_err(led->priv->dev,
> + "spi_write() error %d", ret);
> + break;
> + }
> + }
> +
> + led->priv->delay = jiffies + usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> + mutex_unlock(&led->priv->lock);
> +
> + return ret;
> +}
> +
> +static int el15203000_set_blocking(struct led_classdev *ldev,
> + enum led_brightness brightness)
> +{
> + struct el15203000_led *led = container_of(ldev,
> + struct el15203000_led,
> + ldev);
> +
> + return el15203000_cmd(led, brightness == LED_OFF ? EL_OFF : EL_ON);
> +}
> +
> +static int led_pattern_cmp(const struct led_pattern *p1, u32 p1_len,
> + const struct led_pattern *p2, u32 p2_len)
> +{
> + u32 i;
> +
> + if (p1_len != p2_len)
> + return -1;
> +
> + for (i = 0; i < p1_len; i ++)
> + if (p1[i].delta_t != p2[i].delta_t ||
> + p1[i].brightness != p2[i].brightness)
> + return -1;
> +
> + return 0;
> +}
> +
> +int el15203000_pattern_set_S(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct el15203000_led *led = container_of(ldev,
> + struct el15203000_led,
> + ldev);
> + struct led_pattern scr_pattern[] = {{
> + .delta_t = 4000,
> + .brightness = 0,
> + }, {
> + .delta_t = 4000,
> + .brightness = 1,
> + }};
> +
> + if (repeat > 0)
> + return -EINVAL;
> +
> + if (led_pattern_cmp(scr_pattern, ARRAY_SIZE(scr_pattern), pattern, len))
> + return -EINVAL;
> +
> + dev_dbg(led->priv->dev, "Breathing mode for '%s'", led->name);
> +
> + return el15203000_cmd(led, EL_SCREEN_BREATHING);
> +}
> +
> +int el15203000_pattern_set_P(struct led_classdev *ldev,
> + struct led_pattern *pattern,
> + u32 len, int repeat)
> +{
> + struct el15203000_led *led = container_of(ldev,
> + struct el15203000_led,
> + ldev);
> +
> + struct led_pattern cascade[] = {{
> + .delta_t = 800,
> + .brightness = 1,
> + }, {
> + .delta_t = 800,
> + .brightness = 2,
> + }, {
> + .delta_t = 800,
> + .brightness = 4,
> + }, {
> + .delta_t = 800,
> + .brightness = 8,
> + }, {
> + .delta_t = 800,
> + .brightness = 16,
> + }, {
> + .delta_t = 800,
> + .brightness = 1,
> + }, {
> + .delta_t = 800,
> + .brightness = 2,
> + }, {
> + .delta_t = 800,
> + .brightness = 4,
> + }, {
> + .delta_t = 800,
> + .brightness = 8,
> + }, {
> + .delta_t = 800,
> + .brightness = 16,
> + }};
> +
> + struct led_pattern inv_cascade[] = {{
> + .delta_t = 800,
> + .brightness = 30,
> + }, {
> + .delta_t = 800,
> + .brightness = 29,
> + }, {
> + .delta_t = 800,
> + .brightness = 27,
> + }, {
> + .delta_t = 800,
> + .brightness = 23,
> + }, {
> + .delta_t = 800,
> + .brightness = 15,
> + }, {
> + .delta_t = 800,
> + .brightness = 30,
> + }, {
> + .delta_t = 800,
> + .brightness = 29,
> + }, {
> + .delta_t = 800,
> + .brightness = 27,
> + }, {
> + .delta_t = 800,
> + .brightness = 23,
> + }, {
> + .delta_t = 800,
> + .brightness = 15,
> + }};
> +
> + struct led_pattern bounce[] = {{
> + .delta_t = 800,
> + .brightness = 1,
> + }, {
> + .delta_t = 800,
> + .brightness = 2,
> + }, {
> + .delta_t = 800,
> + .brightness = 4,
> + }, {
> + .delta_t = 800,
> + .brightness = 8,
> + }, {
> + .delta_t = 800,
> + .brightness = 16,
> + }, {
> + .delta_t = 800,
> + .brightness = 16,
> + }, {
> + .delta_t = 800,
> + .brightness = 8,
> + }, {
> + .delta_t = 800,
> + .brightness = 4,
> + }, {
> + .delta_t = 800,
> + .brightness = 2,
> + }, {
> + .delta_t = 800,
> + .brightness = 1,
> + }};
> +
> + struct led_pattern inv_bounce[] = {{
> + .delta_t = 800,
> + .brightness = 30,
> + }, {
> + .delta_t = 800,
> + .brightness = 29,
> + }, {
> + .delta_t = 800,
> + .brightness = 27,
> + }, {
> + .delta_t = 800,
> + .brightness = 23,
> + }, {
> + .delta_t = 800,
> + .brightness = 15,
> + }, {
> + .delta_t = 800,
> + .brightness = 15,
> + }, {
> + .delta_t = 800,
> + .brightness = 23,
> + }, {
> + .delta_t = 800,
> + .brightness = 27,
> + }, {
> + .delta_t = 800,
> + .brightness = 29,
> + }, {
> + .delta_t = 800,
> + .brightness = 30,
> + }};
There is no need for these arrays. We can come up with formulas for
validating the pattern tuples. Basically all of them involve differences
in brightness being powers of two:
"1 800 2 800 4 800 8 800 16 800"
Here each next step should be equal to (1 << i) staring from i = 0.
"30 800 29 800 27 800 23 800 15 800"
brightness = 30;
for (i = 0; i < pattern_len; i++) {
br_diff = (i > 0) ? (1 << (i - 1)) : 0;
brightness -= br_diff;
}
"1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
"30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
Above two can be handled by slightly modifying what I proposed above.
And for delta_t just require by default always 800.
> + if (repeat > 0)
> + return -EINVAL;
> +
> + if (!led_pattern_cmp(cascade, ARRAY_SIZE(cascade), pattern, len)) {
> + dev_dbg(led->priv->dev, "Cascade mode for '%s'", led->name);
> + return el15203000_cmd(led, EL_PIPE_CASCADE);
> + } else if (!led_pattern_cmp(inv_cascade, ARRAY_SIZE(inv_cascade), pattern, len)) {
> + dev_dbg(led->priv->dev, "Inverse cascade mode for '%s'", led->name);
> + return el15203000_cmd(led, EL_PIPE_INV_CASCADE);
> + } else if (!led_pattern_cmp(bounce, ARRAY_SIZE(bounce), pattern, len)) {
> + dev_dbg(led->priv->dev, "Bounce mode for '%s'", led->name);
> + return el15203000_cmd(led, EL_PIPE_BOUNCE);
> + } else if (!led_pattern_cmp(inv_bounce, ARRAY_SIZE(inv_bounce), pattern, len)) {
> + dev_dbg(led->priv->dev, "Inverse bounce mode for '%s'", led->name);
> + return el15203000_cmd(led, EL_PIPE_INV_BOUNCE);
> + }
> +
> + return -EINVAL;
> +}
> +
> +int el15203000_pattern_clear(struct led_classdev *ldev)
> +{
> + struct el15203000_led *led = container_of(ldev,
> + struct el15203000_led,
> + ldev);
> +
> + return el15203000_cmd(led, EL_OFF);
> +}
> +
> +static int el15203000_probe_dt(struct el15203000 *priv)
> +{
> + size_t i = 0;
> + struct el15203000_led *led;
> + struct fwnode_handle *child;
> + int ret;
> + const char *str;
> + u32 reg;
> +
> + device_for_each_child_node(priv->dev, child) {
struct led_init_data init_data = {};
Please add above declaration here.
> + led = &priv->leds[i];
> +
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret) {
> + dev_err(priv->dev, "LED without ID number");
> + fwnode_handle_put(child);
> +
> + return ret;
> + }
> +
> + if (reg > U8_MAX) {
> + dev_err(priv->dev, "LED value %d is invalid", reg);
> + fwnode_handle_put(child);
> +
> + return -EINVAL;
> + }
> +
> + led->reg = reg;
> +
> + ret = fwnode_property_read_string(child, "label", &str);
> + if (ret)
> + str = ":";
> + snprintf(led->name, sizeof(led->name), "el15203000:%s", str);
And instead of the four above lines please just do the below assignment:
init_data.fwnode = child;
but let's do it right before calling devm_led_classdev_register_ext().
> +
> + fwnode_property_read_string(child, "linux,default-trigger",
> + &led->ldev.default_trigger);
> +
> + led->ldev.max_brightness = LED_ON;
> + led->priv = priv;
> + led->ldev.name = led->name;
Please also remove the above line.
> + led->ldev.brightness_set_blocking = el15203000_set_blocking;
> +
> + if (reg == 'S') {
> + led->ldev.pattern_set = el15203000_pattern_set_S;
> + led->ldev.pattern_clear = el15203000_pattern_clear;
> + } else if (reg == 'P') {
> + led->ldev.pattern_set = el15203000_pattern_set_P;
> + led->ldev.pattern_clear = el15203000_pattern_clear;
> + }
> +
> + ret = devm_led_classdev_register(priv->dev, &led->ldev);
And here let's switch to the new LED registration API:
ret = devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);
> + if (ret) {
> + dev_err(priv->dev,
> + "failed to register LED device %s, err %d",
> + led->name, ret);
> + fwnode_handle_put(child);
> + return ret;
> + }
> +
> + i++;
> + }
> +
> + return ret;
> +}
> +
> +static int el15203000_probe(struct spi_device *spi)
> +{
> + struct el15203000 *priv;
> + size_t count;
> + int ret;
> +
> + count = device_get_child_node_count(&spi->dev);
> + if (!count) {
> + dev_err(&spi->dev, "LEDs are not defined in device tree!");
> + return -ENODEV;
> + }
> +
> + priv = devm_kzalloc(&spi->dev, struct_size(priv, leds, count),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + mutex_init(&priv->lock);
> + priv->count = count;
> + priv->dev = &spi->dev;
> + priv->spi = spi;
> + priv->delay = jiffies -
> + usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> + ret = el15203000_probe_dt(priv);
> + if (ret)
> + return ret;
> +
> + spi_set_drvdata(spi, priv);
> + dev_dbg(priv->dev, "%zd LEDs registered", priv->count);
Please remove this debug log.
> +
> + return 0;
> +}
> +
> +static int el15203000_remove(struct spi_device *spi)
> +{
> + struct el15203000 *priv = spi_get_drvdata(spi);
> +
> + mutex_destroy(&priv->lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id el15203000_dt_ids[] = {
> + { .compatible = "crane,el15203000", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, el15203000_dt_ids);
> +
> +static struct spi_driver el15203000_driver = {
> + .probe = el15203000_probe,
> + .remove = el15203000_remove,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + .of_match_table = el15203000_dt_ids,
> + },
> +};
> +
> +module_spi_driver(el15203000_driver);
> +
> +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
> +MODULE_DESCRIPTION("el15203000 LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:el15203000");
>
Please also fix the issues checkpatch.pl complains about.
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-17 15:02 ` Pavel Machek
@ 2019-08-20 7:46 ` Oleh Kravchenko
0 siblings, 0 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-20 7:46 UTC (permalink / raw)
To: Pavel Machek; +Cc: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1929 bytes --]
Hello Pavel,
17.08.19 18:02, Pavel Machek пише:
> Hi!
>
>>>>>> All pattern is predefined, you can't change them at all.
>>>>>> I just tried to describe real things what happened in LED board.
>>>>>> It's ticks every 800 milliseconds for Pipe LEDs.
>>>>>
>>>>> It makes me wonder how you figured out the values? If you have
>>>>> a documentation for this controller, could you share how the pattern
>>>>> settings are documented?
>>>>
>>>> I saw the code of firmware.
>>>> Not sure if I can find any documentation for it right now.
>>>
>>> Have you tried to alter the values? Or check what happens when
>>> the duplication is removed?
>>
>> What do you mean alter? It doesn't make any sense.
>> Board is accepts only brightness level from '0' to '5'.
>> I'm really confused :-)
>
> For the record, what you did seems ok to me.
Thanks :)
>>>> Ok, I will reduce it.
>>>
>>> Please hold on for a while. I will have some more remarks to the driver,
>>> just collecting missing info for now to gain more complete view on this
>>> device.
>>
>> Here is the full story:
>>
>> EL15203000 LEDs board (aka RED LEDs board, because it has only RED LEDs).
>> It's provide access to 3 LEDs:
>>
>> - First LED (Screen) is a light tube around big 21" screen
>> It's have 3 brightness levels:
>> * OFF
>> * ON
>> * Breathing mode (8 seconds full cycle)
>> - Second LED (Vending area) is highlight coffee cap
>> * OFF
>> * ON
>
> Again, this is ok.
>
>> - Third LED (Pipe) is actually virtual, because consists from 5 LEDs
>> * OFF for all 5 LEDs
>> * ON for all 5 LEDs
>> * Cascade
>> * Inverses cascade
>> * Bounce
>> * Inverses bounce
>
> But having one virtual LED that is really five LEDs scares me a tiny
> bit. But I guess its ok for your case...
>
> Best regards,
> Pavel
>
>
>
>
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000
2019-08-18 13:41 ` Jacek Anaszewski
@ 2019-08-20 7:50 ` Oleh Kravchenko
0 siblings, 0 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-20 7:50 UTC (permalink / raw)
To: Jacek Anaszewski, devicetree, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 2790 bytes --]
Hello Jacek,
thanks for your review.
18.08.19 16:41, Jacek Anaszewski пише:
> Hi Oleh,
>
> On 8/8/19 10:32 PM, Oleh Kravchenko wrote:
>> Add documentation and example for dt-bindings EL15203000.
>> LED board (aka RED LED board) from Crane Merchandising Systems.
>>
>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>> ---
>> .../bindings/leds/leds-el15203000.txt | 47 +++++++++++++++++++
>> 1 file changed, 47 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> new file mode 100644
>> index 000000000000..4c2245babfdc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
>> @@ -0,0 +1,47 @@
>> +Crane Merchandising System - el15203000 LED driver
>> +--------------------------------------------------
>> +
>> +This LED Board (aka RED LEDs board) is widely used in
>> +coffee vending machines produced by Crane Merchandising Systems.
>> +
>> +Required properties:
>> +- compatible : "crane,el15203000"
>> +- reg :
>> + see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +- spi-max-frequency : (optional)
>> + see Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Optional LED sub-node properties:
>> +- label :
>> + see Documentation/devicetree/bindings/leds/common.txt
>
> Please change this label description to the below:
>
> - function: see Documentation/devicetree/bindings/leds/common.txt.
> - color: See Documentation/devicetree/bindings/leds/common.txt.
> - label: See Documentation/devicetree/bindings/leds/common.txt (deprecated).
>
>> +- linux,default-trigger :
>> + see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example
>> +-------
>
> #include <dt-bindings/leds/common.h>
>
>> +led-controller@0 {
>> + compatible = "crane,el15203000";
>> + reg = <0>;
>> + spi-max-frequency = <50000>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + /* water pipe */
>> + pipe@50 {
>
> s/pipe/led/
>
>> + reg = <0x50>;
>> + label = "red:pipe";
>
> label is now deprecated.
>
> Please use function and color:
>
> function = "pipe";
> color = <LED_COLOR_ID_RED>;
>
>
>> + };
>> +
>> + /* screen frame */
>> + screen@53 {
>
> s/screen/led/
>
>> + reg = <0x53>;
>> + label = "red:screen";
>
> function = "screen";
> color = <LED_COLOR_ID_RED>;
>
>> + };
>> +
>> + /* vending area */
>> + vend@56 {
>
> s/vend/led/
>
>> + reg = <0x56>;
>> + label = "red:vend";
>
> function = "vend";
> color = <LED_COLOR_ID_RED>;
>
>> + };
>> +};
>>
>
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-18 13:47 ` Jacek Anaszewski
@ 2019-08-27 20:43 ` Oleh Kravchenko
2019-08-27 20:56 ` Jacek Anaszewski
0 siblings, 1 reply; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-27 20:43 UTC (permalink / raw)
To: Jacek Anaszewski, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 19065 bytes --]
Hello Jacek,
what status of linux-leds.git?
git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
Is it outdated? Because I have a errors:
drivers/leds/leds-el15203000.c:234:9: error: variable ‘init_data’ has initializer but incomplete type
struct led_init_data init_data = {};
^~~~~~~~~~~~~
18.08.19 16:47, Jacek Anaszewski пише:
> Hi Oleh,
>
> Please find my review remarks below.
>
> 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"
>
> There's no point in repeating same sequence. Just require to provide it
> once:
>
> "1 800 2 800 4 800 8 800 16 800"
>
>> +
>> + 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"
>
> The same story:
>
> "30 800 29 800 27 800 23 800 15 800"
>
>> +
>> + 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"
>> +
>> + 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"
>
> These can be left as they are now.
>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 760f73a49c9f..0cbdb9ba5213 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -129,6 +129,19 @@ config LEDS_CR0014114
>> To compile this driver as a module, choose M here: the module
>> will be called leds-cr0014114.
>>
>> +config LEDS_EL15203000
>> + tristate "LED Support for Crane EL15203000"
>> + depends on LEDS_CLASS
>> + depends on SPI
>> + depends on OF
>> + help
>> + This option enables support for EL15203000 LED Board
>> + (aka RED LED board) which is widely used in coffee vending
>> + machines produced by Crane Merchandising Systems.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called leds-el15203000.
>> +
>> config LEDS_LM3530
>> tristate "LCD Backlight driver for LM3530"
>> depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 1e9702ebffee..1f193ffc2feb 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o
>> # LED SPI Drivers
>> obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o
>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>> +obj-$(CONFIG_LEDS_EL15203000) += leds-el15203000.o
>>
>> # LED Userspace Drivers
>> obj-$(CONFIG_LEDS_USER) += uleds.o
>> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
>> new file mode 100644
>> index 000000000000..c62da5ec6630
>> --- /dev/null
>> +++ b/drivers/leds/leds-el15203000.c
>> @@ -0,0 +1,478 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019 Crane Merchandising Systems. All rights reserved.
>> +// Copyright (C) 2019 Oleh Kravchenko <oleg@kaa.org.ua>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/leds.h>
>> +#include <linux/limits.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/spi/spi.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +/*
>> + * EL15203000 SPI protocol description:
>> + * +-----+------------+
>> + * | LED | BRIGHTNESS |
>> + * +-----+------------+
>> + * | 1 | 1 |
>> + * +-----+------------+
>> + * (*) LEDs MCU board expects 20 msec delay per byte.
>> + *
>> + * LEDs:
>> + * +----------+--------------+-------------------------------------------+
>> + * | ID | NAME | DESCRIPTION |
>> + * +----------+--------------+-------------------------------------------+
>> + * | 'P' 0x50 | Pipe | Consists from 5 LEDs, controlled by board |
>> + * +----------+--------------+-------------------------------------------+
>> + * | 'S' 0x53 | Screen frame | Light tube around the screen |
>> + * +----------+--------------+-------------------------------------------+
>> + * | 'V' 0x56 | Vending area | Highlights a cup of coffee |
>> + * +----------+--------------+-------------------------------------------+
>> + *
>> + * BRIGHTNESS:
>> + * +----------+-----------------+--------------+--------------+
>> + * | VALUES | PIPE | SCREEN FRAME | VENDING AREA |
>> + * +----------+-----------------+--------------+--------------+
>> + * | '0' 0x30 | Off |
>> + * +----------+-----------------------------------------------+
>> + * | '1' 0x31 | On |
>> + * +----------+-----------------+--------------+--------------+
>> + * | '2' 0x32 | Cascade | Breathing |
>> + * +----------+-----------------+--------------+
>> + * | '3' 0x33 | Inverse cascade |
>> + * +----------+-----------------+
>> + * | '4' 0x34 | Bounce |
>> + * +----------+-----------------+
>> + * | '5' 0x35 | Inverse bounce |
>> + * +----------+-----------------+
>> + */
>> +
>> +/* EL15203000 default settings */
>> +#define EL_FW_DELAY_USEC 20000
>> +
>> +enum el15203000_brightness {
>
> Please change 'brightness' to 'command' or so. Brightness is misleading
> since this device supports in fact only one level of brightness for
> non-pattern use case.
>
>> + /* for all LEDs */
>> + EL_OFF = '0',
>> + EL_ON = '1',
>> +
>> + /* for Screen LED */
>> + EL_SCREEN_BREATHING = '2',
>> +
>> + /* for Pipe LED */
>> + EL_PIPE_CASCADE = '2',
>> + EL_PIPE_INV_CASCADE = '3',
>> + EL_PIPE_BOUNCE = '4',
>> + EL_PIPE_INV_BOUNCE = '5',
>> +};
>> +
>> +struct el15203000_led {
>> + struct el15203000 *priv;
>> + struct led_classdev ldev;
>> + char name[LED_MAX_NAME_SIZE];
>
> This field will not be needed if we switch to
> devm_led_classdev_register_ext().
>
>> + u8 reg;
>> +};
>> +
>> +struct el15203000 {
>> + struct device *dev;
>> + struct mutex lock;
>> + struct spi_device *spi;
>> + unsigned long delay;
>> + size_t count;
>> + struct el15203000_led leds[];
>> +};
>> +
>> +static int el15203000_cmd(struct el15203000_led *led, u8 brightness)
>> +{
>> + int ret;
>> + u8 cmd[2];
>> + size_t i;
>> + unsigned long jdelay, udelay, now;
>> +
>> + mutex_lock(&led->priv->lock);
>> +
>> + dev_dbg(led->priv->dev, "Set brightness of %s to %d",
>> + led->name, brightness);
>> +
>> + /* to avoid SPI mistiming with firmware we should wait some time */
>> + now = jiffies;
>> + if (time_after(led->priv->delay, now)) {
>> + jdelay = led->priv->delay - now;
>> + udelay = jiffies_to_usecs(jdelay);
>> +
>> + dev_dbg(led->priv->dev, "Wait %luus (%lu jiffies) to sync",
>> + udelay, jdelay);
>> +
>> + usleep_range(udelay, udelay + 1);
>> + }
>> +
>> + cmd[0] = led->reg;
>> + cmd[1] = brightness;
>> +
>> + for (i = 0; i < ARRAY_SIZE(cmd); i++) {
>> + if (i)
>> + usleep_range(EL_FW_DELAY_USEC,
>> + EL_FW_DELAY_USEC + 1);
>> +
>> + ret = spi_write(led->priv->spi, &cmd[i], sizeof(cmd[i]));
>> + if (ret) {
>> + dev_err(led->priv->dev,
>> + "spi_write() error %d", ret);
>> + break;
>> + }
>> + }
>> +
>> + led->priv->delay = jiffies + usecs_to_jiffies(EL_FW_DELAY_USEC);
>> +
>> + mutex_unlock(&led->priv->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int el15203000_set_blocking(struct led_classdev *ldev,
>> + enum led_brightness brightness)
>> +{
>> + struct el15203000_led *led = container_of(ldev,
>> + struct el15203000_led,
>> + ldev);
>> +
>> + return el15203000_cmd(led, brightness == LED_OFF ? EL_OFF : EL_ON);
>> +}
>> +
>> +static int led_pattern_cmp(const struct led_pattern *p1, u32 p1_len,
>> + const struct led_pattern *p2, u32 p2_len)
>> +{
>> + u32 i;
>> +
>> + if (p1_len != p2_len)
>> + return -1;
>> +
>> + for (i = 0; i < p1_len; i ++)
>> + if (p1[i].delta_t != p2[i].delta_t ||
>> + p1[i].brightness != p2[i].brightness)
>> + return -1;
>> +
>> + return 0;
>> +}
>> +
>> +int el15203000_pattern_set_S(struct led_classdev *ldev,
>> + struct led_pattern *pattern,
>> + u32 len, int repeat)
>> +{
>> + struct el15203000_led *led = container_of(ldev,
>> + struct el15203000_led,
>> + ldev);
>> + struct led_pattern scr_pattern[] = {{
>> + .delta_t = 4000,
>> + .brightness = 0,
>> + }, {
>> + .delta_t = 4000,
>> + .brightness = 1,
>> + }};
>> +
>> + if (repeat > 0)
>> + return -EINVAL;
>> +
>> + if (led_pattern_cmp(scr_pattern, ARRAY_SIZE(scr_pattern), pattern, len))
>> + return -EINVAL;
>> +
>> + dev_dbg(led->priv->dev, "Breathing mode for '%s'", led->name);
>> +
>> + return el15203000_cmd(led, EL_SCREEN_BREATHING);
>> +}
>> +
>> +int el15203000_pattern_set_P(struct led_classdev *ldev,
>> + struct led_pattern *pattern,
>> + u32 len, int repeat)
>> +{
>> + struct el15203000_led *led = container_of(ldev,
>> + struct el15203000_led,
>> + ldev);
>> +
>> + struct led_pattern cascade[] = {{
>> + .delta_t = 800,
>> + .brightness = 1,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 2,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 4,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 8,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 16,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 1,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 2,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 4,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 8,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 16,
>> + }};
>> +
>> + struct led_pattern inv_cascade[] = {{
>> + .delta_t = 800,
>> + .brightness = 30,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 29,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 27,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 23,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 15,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 30,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 29,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 27,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 23,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 15,
>> + }};
>> +
>> + struct led_pattern bounce[] = {{
>> + .delta_t = 800,
>> + .brightness = 1,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 2,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 4,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 8,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 16,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 16,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 8,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 4,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 2,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 1,
>> + }};
>> +
>> + struct led_pattern inv_bounce[] = {{
>> + .delta_t = 800,
>> + .brightness = 30,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 29,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 27,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 23,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 15,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 15,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 23,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 27,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 29,
>> + }, {
>> + .delta_t = 800,
>> + .brightness = 30,
>> + }};
>
> There is no need for these arrays. We can come up with formulas for
> validating the pattern tuples. Basically all of them involve differences
> in brightness being powers of two:
>
> "1 800 2 800 4 800 8 800 16 800"
>
> Here each next step should be equal to (1 << i) staring from i = 0.
>
> "30 800 29 800 27 800 23 800 15 800"
>
> brightness = 30;
> for (i = 0; i < pattern_len; i++) {
> br_diff = (i > 0) ? (1 << (i - 1)) : 0;
> brightness -= br_diff;
> }
>
> "1 800 2 800 4 800 8 800 16 800 16 800 8 800 4 800 2 800 1 800"
>
> "30 800 29 800 27 800 23 800 15 800 15 800 23 800 27 800 29 800 30 800"
>
> Above two can be handled by slightly modifying what I proposed above.
>
> And for delta_t just require by default always 800.
>
>> + if (repeat > 0)
>> + return -EINVAL;
>> +
>> + if (!led_pattern_cmp(cascade, ARRAY_SIZE(cascade), pattern, len)) {
>> + dev_dbg(led->priv->dev, "Cascade mode for '%s'", led->name);
>> + return el15203000_cmd(led, EL_PIPE_CASCADE);
>> + } else if (!led_pattern_cmp(inv_cascade, ARRAY_SIZE(inv_cascade), pattern, len)) {
>> + dev_dbg(led->priv->dev, "Inverse cascade mode for '%s'", led->name);
>> + return el15203000_cmd(led, EL_PIPE_INV_CASCADE);
>> + } else if (!led_pattern_cmp(bounce, ARRAY_SIZE(bounce), pattern, len)) {
>> + dev_dbg(led->priv->dev, "Bounce mode for '%s'", led->name);
>> + return el15203000_cmd(led, EL_PIPE_BOUNCE);
>> + } else if (!led_pattern_cmp(inv_bounce, ARRAY_SIZE(inv_bounce), pattern, len)) {
>> + dev_dbg(led->priv->dev, "Inverse bounce mode for '%s'", led->name);
>> + return el15203000_cmd(led, EL_PIPE_INV_BOUNCE);
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +int el15203000_pattern_clear(struct led_classdev *ldev)
>> +{
>> + struct el15203000_led *led = container_of(ldev,
>> + struct el15203000_led,
>> + ldev);
>> +
>> + return el15203000_cmd(led, EL_OFF);
>> +}
>> +
>> +static int el15203000_probe_dt(struct el15203000 *priv)
>> +{
>> + size_t i = 0;
>> + struct el15203000_led *led;
>> + struct fwnode_handle *child;
>> + int ret;
>> + const char *str;
>> + u32 reg;
>> +
>> + device_for_each_child_node(priv->dev, child) {
> struct led_init_data init_data = {};
>
> Please add above declaration here.
>
>> + led = &priv->leds[i];
>> +
>> + ret = fwnode_property_read_u32(child, "reg", ®);
>> + if (ret) {
>> + dev_err(priv->dev, "LED without ID number");
>> + fwnode_handle_put(child);
>> +
>> + return ret;
>> + }
>> +
>> + if (reg > U8_MAX) {
>> + dev_err(priv->dev, "LED value %d is invalid", reg);
>> + fwnode_handle_put(child);
>> +
>> + return -EINVAL;
>> + }
>> +
>> + led->reg = reg;
>> +
>> + ret = fwnode_property_read_string(child, "label", &str);
>> + if (ret)
>> + str = ":";
>> + snprintf(led->name, sizeof(led->name), "el15203000:%s", str);
>
> And instead of the four above lines please just do the below assignment:
>
> init_data.fwnode = child;
>
> but let's do it right before calling devm_led_classdev_register_ext().
>
>> +
>> + fwnode_property_read_string(child, "linux,default-trigger",
>> + &led->ldev.default_trigger);
>> +
>> + led->ldev.max_brightness = LED_ON;
>> + led->priv = priv;
>> + led->ldev.name = led->name;
>
> Please also remove the above line.
>
>> + led->ldev.brightness_set_blocking = el15203000_set_blocking;
>> +
>> + if (reg == 'S') {
>> + led->ldev.pattern_set = el15203000_pattern_set_S;
>> + led->ldev.pattern_clear = el15203000_pattern_clear;
>> + } else if (reg == 'P') {
>> + led->ldev.pattern_set = el15203000_pattern_set_P;
>> + led->ldev.pattern_clear = el15203000_pattern_clear;
>> + }
>> +
>> + ret = devm_led_classdev_register(priv->dev, &led->ldev);
>
> And here let's switch to the new LED registration API:
>
> ret = devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);
>
>
>> + if (ret) {
>> + dev_err(priv->dev,
>> + "failed to register LED device %s, err %d",
>> + led->name, ret);
>> + fwnode_handle_put(child);
>> + return ret;
>> + }
>> +
>> + i++;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int el15203000_probe(struct spi_device *spi)
>> +{
>> + struct el15203000 *priv;
>> + size_t count;
>> + int ret;
>> +
>> + count = device_get_child_node_count(&spi->dev);
>> + if (!count) {
>> + dev_err(&spi->dev, "LEDs are not defined in device tree!");
>> + return -ENODEV;
>> + }
>> +
>> + priv = devm_kzalloc(&spi->dev, struct_size(priv, leds, count),
>> + GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + mutex_init(&priv->lock);
>> + priv->count = count;
>> + priv->dev = &spi->dev;
>> + priv->spi = spi;
>> + priv->delay = jiffies -
>> + usecs_to_jiffies(EL_FW_DELAY_USEC);
>> +
>> + ret = el15203000_probe_dt(priv);
>> + if (ret)
>> + return ret;
>> +
>> + spi_set_drvdata(spi, priv);
>> + dev_dbg(priv->dev, "%zd LEDs registered", priv->count);
>
> Please remove this debug log.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int el15203000_remove(struct spi_device *spi)
>> +{
>> + struct el15203000 *priv = spi_get_drvdata(spi);
>> +
>> + mutex_destroy(&priv->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id el15203000_dt_ids[] = {
>> + { .compatible = "crane,el15203000", },
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, el15203000_dt_ids);
>> +
>> +static struct spi_driver el15203000_driver = {
>> + .probe = el15203000_probe,
>> + .remove = el15203000_remove,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .of_match_table = el15203000_dt_ids,
>> + },
>> +};
>> +
>> +module_spi_driver(el15203000_driver);
>> +
>> +MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
>> +MODULE_DESCRIPTION("el15203000 LED driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("spi:el15203000");
>>
>
> Please also fix the issues checkpatch.pl complains about.
>
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-27 20:43 ` Oleh Kravchenko
@ 2019-08-27 20:56 ` Jacek Anaszewski
2019-08-27 21:11 ` Oleh Kravchenko
0 siblings, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-27 20:56 UTC (permalink / raw)
To: Oleh Kravchenko, linux-leds
Hi Oleh,
On 8/27/19 10:43 PM, Oleh Kravchenko wrote:
> Hello Jacek,
>
> what status of linux-leds.git?
> git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>
> Is it outdated? Because I have a errors:
> drivers/leds/leds-el15203000.c:234:9: error: variable ‘init_data’ has initializer but incomplete type
> struct led_init_data init_data = {};
I've just compiled drivers/leds/leds-aat1290.c driver that contains the
same declaration and it went just fine.
I know this is trivial, but did you "#include <linux/leds.h>" ?
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-27 20:56 ` Jacek Anaszewski
@ 2019-08-27 21:11 ` Oleh Kravchenko
2019-08-27 21:18 ` Jacek Anaszewski
0 siblings, 1 reply; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-27 21:11 UTC (permalink / raw)
To: Jacek Anaszewski, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1264 bytes --]
Yes it has this include :)
$ git remote -v
origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (fetch)
origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (push)
$ git fetch --all -p
Fetching origin
$ git log origin/master -1
commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b (tag: v5.3-rc1, origin/master, origin/HEAD)
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun Jul 21 14:05:38 2019 -0700
Linus 5.3-rc1
$ grep -R led_init_data --include='*.h'
returns nothing..
27.08.19 23:56, Jacek Anaszewski пише:
> Hi Oleh,
>
> On 8/27/19 10:43 PM, Oleh Kravchenko wrote:
>> Hello Jacek,
>>
>> what status of linux-leds.git?
>> git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>>
>> Is it outdated? Because I have a errors:
>> drivers/leds/leds-el15203000.c:234:9: error: variable ‘init_data’ has initializer but incomplete type
>> struct led_init_data init_data = {};
>
> I've just compiled drivers/leds/leds-aat1290.c driver that contains the
> same declaration and it went just fine.
>
> I know this is trivial, but did you "#include <linux/leds.h>" ?
>
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-27 21:11 ` Oleh Kravchenko
@ 2019-08-27 21:18 ` Jacek Anaszewski
2019-08-27 21:26 ` Oleh Kravchenko
0 siblings, 1 reply; 24+ messages in thread
From: Jacek Anaszewski @ 2019-08-27 21:18 UTC (permalink / raw)
To: Oleh Kravchenko, linux-leds
Oleh,
On 8/27/19 11:11 PM, Oleh Kravchenko wrote:
> Yes it has this include :)
>
> $ git remote -v
> origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (fetch)
> origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (push)
>
> $ git fetch --all -p
> Fetching origin
>
> $ git log origin/master -1
> commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b (tag: v5.3-rc1, origin/master, origin/HEAD)
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sun Jul 21 14:05:38 2019 -0700
You need to checkout for-next branch.
> Linus 5.3-rc1
>
> $ grep -R led_init_data --include='*.h'
>
> returns nothing..
>
> 27.08.19 23:56, Jacek Anaszewski пише:
>> Hi Oleh,
>>
>> On 8/27/19 10:43 PM, Oleh Kravchenko wrote:
>>> Hello Jacek,
>>>
>>> what status of linux-leds.git?
>>> git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>>>
>>> Is it outdated? Because I have a errors:
>>> drivers/leds/leds-el15203000.c:234:9: error: variable ‘init_data’ has initializer but incomplete type
>>> struct led_init_data init_data = {};
>>
>> I've just compiled drivers/leds/leds-aat1290.c driver that contains the
>> same declaration and it went just fine.
>>
>> I know this is trivial, but did you "#include <linux/leds.h>" ?
>>
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 2/2] leds: add LED driver for EL15203000 board
2019-08-27 21:18 ` Jacek Anaszewski
@ 2019-08-27 21:26 ` Oleh Kravchenko
0 siblings, 0 replies; 24+ messages in thread
From: Oleh Kravchenko @ 2019-08-27 21:26 UTC (permalink / raw)
To: Jacek Anaszewski, linux-leds
[-- Attachment #1.1: Type: text/plain, Size: 1521 bytes --]
Thanks :-)
28.08.19 00:18, Jacek Anaszewski пише:
> Oleh,
>
> On 8/27/19 11:11 PM, Oleh Kravchenko wrote:
>> Yes it has this include :)
>>
>> $ git remote -v
>> origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (fetch)
>> origin git://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git (push)
>>
>> $ git fetch --all -p
>> Fetching origin
>>
>> $ git log origin/master -1
>> commit 5f9e832c137075045d15cd6899ab0505cfb2ca4b (tag: v5.3-rc1, origin/master, origin/HEAD)
>> Author: Linus Torvalds <torvalds@linux-foundation.org>
>> Date: Sun Jul 21 14:05:38 2019 -0700
>
> You need to checkout for-next branch.
>
>> Linus 5.3-rc1
>>
>> $ grep -R led_init_data --include='*.h'
>>
>> returns nothing..
>>
>> 27.08.19 23:56, Jacek Anaszewski пише:
>>> Hi Oleh,
>>>
>>> On 8/27/19 10:43 PM, Oleh Kravchenko wrote:
>>>> Hello Jacek,
>>>>
>>>> what status of linux-leds.git?
>>>> git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git
>>>>
>>>> Is it outdated? Because I have a errors:
>>>> drivers/leds/leds-el15203000.c:234:9: error: variable ‘init_data’ has initializer but incomplete type
>>>> struct led_init_data init_data = {};
>>>
>>> I've just compiled drivers/leds/leds-aat1290.c driver that contains the
>>> same declaration and it went just fine.
>>>
>>> I know this is trivial, but did you "#include <linux/leds.h>" ?
>>>
>>
>
--
Best regards,
Oleh Kravchenko
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2019-08-27 21:26 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2019-08-13 20:28 ` Jacek Anaszewski
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-13 20:31 ` Dan Murphy
2019-08-14 19:54 ` Jacek Anaszewski
2019-08-18 13:41 ` Jacek Anaszewski
2019-08-20 7:50 ` Oleh Kravchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.