* [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; 22+ 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] 22+ 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 ` [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Dan Murphy 2019-08-18 13:41 ` Jacek Anaszewski 2 siblings, 2 replies; 22+ 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] 22+ 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-13 20:37 ` Oleh Kravchenko 2019-08-18 13:47 ` Jacek Anaszewski 1 sibling, 1 reply; 22+ 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] 22+ 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 0 siblings, 1 reply; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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; 22+ 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] 22+ 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-14 19:54 ` Jacek Anaszewski 2019-08-18 13:41 ` Jacek Anaszewski 2 siblings, 1 reply; 22+ 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] 22+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 2019-08-13 20:31 ` [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Dan Murphy @ 2019-08-14 19:54 ` Jacek Anaszewski 0 siblings, 0 replies; 22+ 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] 22+ 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 ` [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Dan Murphy @ 2019-08-18 13:41 ` Jacek Anaszewski 2019-08-20 7:50 ` Oleh Kravchenko 2 siblings, 1 reply; 22+ 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] 22+ 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; 22+ 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] 22+ messages in thread
end of thread, other threads:[~2019-08-27 21:26 UTC | newest] Thread overview: 22+ 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:37 ` Oleh Kravchenko 2019-08-14 18:18 ` Jacek Anaszewski 2019-08-14 19:46 ` Oleh Kravchenko 2019-08-14 19:57 ` Jacek Anaszewski 2019-08-14 20:16 ` Oleh Kravchenko 2019-08-14 20:53 ` Jacek Anaszewski 2019-08-14 20:58 ` Oleh Kravchenko 2019-08-17 15:02 ` Pavel Machek 2019-08-20 7:46 ` Oleh Kravchenko 2019-08-18 13:47 ` Jacek Anaszewski 2019-08-27 20:43 ` Oleh Kravchenko 2019-08-27 20:56 ` Jacek Anaszewski 2019-08-27 21:11 ` Oleh Kravchenko 2019-08-27 21:18 ` Jacek Anaszewski 2019-08-27 21:26 ` Oleh Kravchenko 2019-08-13 20:31 ` [PATCH v4 1/2] dt-bindings: Add docs for EL15203000 Dan Murphy 2019-08-14 19:54 ` Jacek Anaszewski 2019-08-18 13:41 ` Jacek Anaszewski 2019-08-20 7:50 ` Oleh Kravchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).