| Message ID | 20190830224619.15728-1-oleg@kaa.org.ua |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v5,1/2] dt-bindings: Add docs for EL15203000 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On Sat, 31 Aug 2019 01:46:18 +0300, 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 | 54 +++++++++++++++++++ > 1 file changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt > Please add Acked-by/Reviewed-by tags when posting new versions. However, there's no need to repost patches *only* to add the tags. The upstream maintainer will do that for acks received on the version they apply. If a tag was not added on purpose, please state why and what changed.
Hello Rob! 01.09.19 22:18, Rob Herring пише: > On Sat, 31 Aug 2019 01:46:18 +0300, 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 | 54 +++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt >> > > Please add Acked-by/Reviewed-by tags when posting new versions. However, > there's no need to repost patches *only* to add the tags. The upstream > maintainer will do that for acks received on the version they apply. I will do it next time. > If a tag was not added on purpose, please state why and what changed. Sorry I don't wanted to bother anyone.
Hi Oleh, Thank you for the updated set. Now it looks really good. Just few minor issues left. And side note - please address the patches also to maintainers, not only to the list. On 8/31/19 12:46 AM, 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 | 362 ++++++++++++++++++ > 4 files changed, 398 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..767763409125 > --- /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 Now it will be September and 5.5. It is late even for 5.4. > +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" > + > + Inverted cascade mode for Pipe LED: > + "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 1988de1d64c0..6e7703fd03d0 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -132,6 +132,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 41fb073a39c1..2da39e896ce8 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -89,6 +89,7 @@ obj-$(CONFIG_LEDS_LM36274) += leds-lm36274.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..9de81dee3618 > --- /dev/null > +++ b/drivers/leds/leds-el15203000.c > @@ -0,0 +1,362 @@ > +// 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/module.h> > +#include <linux/of_device.h> > +#include <linux/spi/spi.h> > + > +/* > + * EL15203000 SPI protocol description: > + * +-----+---------+ > + * | LED | COMMAND | > + * +-----+---------+ > + * | 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 | > + * +----------+--------------+-------------------------------------------+ > + * > + * COMMAND: > + * +----------+-----------------+--------------+--------------+ > + * | 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 20000ul > +#define EL_PATTERN_DELAY_MSEC 800u > +#define EL_PATTERN_LEN 10u > +#define EL_PATTERN_HALF_LEN (EL_PATTERN_LEN / 2) > + > +enum el15203000_command { > + /* 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; > + 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; > + > + mutex_lock(&led->priv->lock); > + > + dev_dbg(led->priv->dev, "Set brightness of 0x%02x(%c) to 0x%02x(%c)", > + led->reg, led->reg, brightness, brightness); > + > + /* to avoid SPI mistiming with firmware we should wait some time */ > + if (time_after(led->priv->delay, jiffies)) { > + dev_dbg(led->priv->dev, "Wait %luus to sync", > + EL_FW_DELAY_USEC); > + > + usleep_range(EL_FW_DELAY_USEC, > + EL_FW_DELAY_USEC + 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 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); > + > + if (repeat > 0 || len != 2 || > + pattern[0].delta_t != 4000 || pattern[0].brightness != 0 || > + pattern[1].delta_t != 4000 || pattern[1].brightness != 1) > + return -EINVAL; > + > + dev_dbg(led->priv->dev, "Breathing mode for 0x%02x(%c)", > + led->reg, led->reg); > + > + return el15203000_cmd(led, EL_SCREEN_BREATHING); > +} > + > +static bool is_cascade(const struct led_pattern *pattern, u32 len, > + bool inv, bool right) > +{ > + int val, t; > + u32 i; > + > + if (len != EL_PATTERN_HALF_LEN) > + return false; > + > + val = right ? BIT(4) : BIT(0); > + > + for (i = 0; i < len; i++) { > + t = inv ? ~val & GENMASK(4, 0) : val; > + > + if (pattern[i].delta_t != EL_PATTERN_DELAY_MSEC || > + pattern[i].brightness != t) > + return false; > + > + val = right ? val >> 1 : val << 1; > + } Nice! > + > + return true; > +} > + > +static bool is_bounce(const struct led_pattern *pattern, u32 len, bool inv) > +{ > + if (len != EL_PATTERN_LEN) > + return false; > + > + return is_cascade(pattern, EL_PATTERN_HALF_LEN, inv, false) && > + is_cascade(pattern + EL_PATTERN_HALF_LEN, > + EL_PATTERN_HALF_LEN, inv, true); > +} > + > +static 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); > + > + if (repeat > 0) This is wrong. Repeat has to be -1 or > 0. If all patterns supported by your device are infinite, then you should expect here -1. Either way this needs to be covered in the ABI documentation too. > + return -EINVAL; > + > + if (is_cascade(pattern, len, false, false)) { > + dev_dbg(led->priv->dev, "Cascade mode for 0x%02x(%c)", > + led->reg, led->reg); > + > + return el15203000_cmd(led, EL_PIPE_CASCADE); > + } else if (is_cascade(pattern, len, true, false)) { > + dev_dbg(led->priv->dev, "Inverse cascade mode for 0x%02x(%c)", > + led->reg, led->reg); > + > + return el15203000_cmd(led, EL_PIPE_INV_CASCADE); > + } else if (is_bounce(pattern, len, false)) { > + dev_dbg(led->priv->dev, "Bounce mode for 0x%02x(%c)", > + led->reg, led->reg); > + > + return el15203000_cmd(led, EL_PIPE_BOUNCE); > + } else if (is_bounce(pattern, len, true)) { > + dev_dbg(led->priv->dev, "Inverse bounce mode for 0x%02x(%c)", > + led->reg, led->reg); > + > + return el15203000_cmd(led, EL_PIPE_INV_BOUNCE); > + } > + > + return -EINVAL; > +} > + > +static 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; > + u32 reg; > + struct led_init_data init_data = {}; > + > + device_for_each_child_node(priv->dev, child) { Please move above init_data initialization here. > + led = &priv->leds[i]; You can increment 'i' here, it is not used below and you will save one LOC: led = &priv->leds[i++]; > + > + init_data.fwnode = child; Please move it to where it will be needed. i.e. one line above devm_led_classdev_register_ext(). > + > + 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; I'd just pass &led->reg directly to fwnode_property_read_u32(). Then you'll be able to drop local reg variable. > + > + fwnode_property_read_string(child, "linux,default-trigger", > + &led->ldev.default_trigger); > + > + led->priv = priv; > + led->ldev.max_brightness = LED_ON; > + 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_ext(priv->dev, &led->ldev, > + &init_data); > + if (ret) { > + dev_err(priv->dev, > + "failed to register LED device %s, err %d", > + led->ldev.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); > + > + 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"); >
On 9/1/19 9:18 PM, Rob Herring wrote: > On Sat, 31 Aug 2019 01:46:18 +0300, 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 | 54 +++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt >> > > Please add Acked-by/Reviewed-by tags when posting new versions. However, > there's no need to repost patches *only* to add the tags. The upstream > maintainer will do that for acks received on the version they apply. > > If a tag was not added on purpose, please state why and what changed. > Actually there has been some rework involved after acked v3, so dropping the tag was justified.
Hello Jacek, Few question from my side just for better understanding :) > 5 вер. 2019 р. о 12:23 дп Jacek Anaszewski <jacek.anaszewski@gmail.com> написав(ла): > > Hi Oleh, > > Thank you for the updated set. > > Now it looks really good. Just few minor issues left. > > And side note - please address the patches also to maintainers, > not only to the list. > > On 8/31/19 12:46 AM, 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 | 362 ++++++++++++++++++ >> 4 files changed, 398 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..767763409125 >> --- /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 > > Now it will be September and 5.5. It is late even for 5.4. Here should month of patch creation or when it will be reviewed? :) Because I sent in 31 August. > >> +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" >> + >> + Inverted cascade mode for Pipe LED: >> + "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 1988de1d64c0..6e7703fd03d0 100644 >> --- a/drivers/leds/Kconfig >> +++ b/drivers/leds/Kconfig >> @@ -132,6 +132,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 41fb073a39c1..2da39e896ce8 100644 >> --- a/drivers/leds/Makefile >> +++ b/drivers/leds/Makefile >> @@ -89,6 +89,7 @@ obj-$(CONFIG_LEDS_LM36274) += leds-lm36274.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..9de81dee3618 >> --- /dev/null >> +++ b/drivers/leds/leds-el15203000.c >> @@ -0,0 +1,362 @@ >> +// 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/module.h> >> +#include <linux/of_device.h> >> +#include <linux/spi/spi.h> >> + >> +/* >> + * EL15203000 SPI protocol description: >> + * +-----+---------+ >> + * | LED | COMMAND | >> + * +-----+---------+ >> + * | 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 | >> + * +----------+--------------+-------------------------------------------+ >> + * >> + * COMMAND: >> + * +----------+-----------------+--------------+--------------+ >> + * | 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 20000ul >> +#define EL_PATTERN_DELAY_MSEC 800u >> +#define EL_PATTERN_LEN 10u >> +#define EL_PATTERN_HALF_LEN (EL_PATTERN_LEN / 2) >> + >> +enum el15203000_command { >> + /* 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; >> + 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; >> + >> + mutex_lock(&led->priv->lock); >> + >> + dev_dbg(led->priv->dev, "Set brightness of 0x%02x(%c) to 0x%02x(%c)", >> + led->reg, led->reg, brightness, brightness); >> + >> + /* to avoid SPI mistiming with firmware we should wait some time */ >> + if (time_after(led->priv->delay, jiffies)) { >> + dev_dbg(led->priv->dev, "Wait %luus to sync", >> + EL_FW_DELAY_USEC); >> + >> + usleep_range(EL_FW_DELAY_USEC, >> + EL_FW_DELAY_USEC + 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 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); >> + >> + if (repeat > 0 || len != 2 || >> + pattern[0].delta_t != 4000 || pattern[0].brightness != 0 || >> + pattern[1].delta_t != 4000 || pattern[1].brightness != 1) >> + return -EINVAL; >> + >> + dev_dbg(led->priv->dev, "Breathing mode for 0x%02x(%c)", >> + led->reg, led->reg); >> + >> + return el15203000_cmd(led, EL_SCREEN_BREATHING); >> +} >> + >> +static bool is_cascade(const struct led_pattern *pattern, u32 len, >> + bool inv, bool right) >> +{ >> + int val, t; >> + u32 i; >> + >> + if (len != EL_PATTERN_HALF_LEN) >> + return false; >> + >> + val = right ? BIT(4) : BIT(0); >> + >> + for (i = 0; i < len; i++) { >> + t = inv ? ~val & GENMASK(4, 0) : val; >> + >> + if (pattern[i].delta_t != EL_PATTERN_DELAY_MSEC || >> + pattern[i].brightness != t) >> + return false; >> + >> + val = right ? val >> 1 : val << 1; >> + } > > Nice! > >> + >> + return true; >> +} >> + >> +static bool is_bounce(const struct led_pattern *pattern, u32 len, bool inv) >> +{ >> + if (len != EL_PATTERN_LEN) >> + return false; >> + >> + return is_cascade(pattern, EL_PATTERN_HALF_LEN, inv, false) && >> + is_cascade(pattern + EL_PATTERN_HALF_LEN, >> + EL_PATTERN_HALF_LEN, inv, true); >> +} >> + >> +static 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); >> + >> + if (repeat > 0) > > This is wrong. Repeat has to be -1 or > 0. If all patterns supported > by your device are infinite, then you should expect here -1. Ok, then we have bug in led pattern trigger. echo -1 > /…/repeat doesn’t work as expected and return error. So if I tried to change repeat from sysfs, it will never be -1 again. > > Either way this needs to be covered in the ABI documentation too. > >> + return -EINVAL; >> + >> + if (is_cascade(pattern, len, false, false)) { >> + dev_dbg(led->priv->dev, "Cascade mode for 0x%02x(%c)", >> + led->reg, led->reg); >> + >> + return el15203000_cmd(led, EL_PIPE_CASCADE); >> + } else if (is_cascade(pattern, len, true, false)) { >> + dev_dbg(led->priv->dev, "Inverse cascade mode for 0x%02x(%c)", >> + led->reg, led->reg); >> + >> + return el15203000_cmd(led, EL_PIPE_INV_CASCADE); >> + } else if (is_bounce(pattern, len, false)) { >> + dev_dbg(led->priv->dev, "Bounce mode for 0x%02x(%c)", >> + led->reg, led->reg); >> + >> + return el15203000_cmd(led, EL_PIPE_BOUNCE); >> + } else if (is_bounce(pattern, len, true)) { >> + dev_dbg(led->priv->dev, "Inverse bounce mode for 0x%02x(%c)", >> + led->reg, led->reg); >> + >> + return el15203000_cmd(led, EL_PIPE_INV_BOUNCE); >> + } >> + >> + return -EINVAL; >> +} >> + >> +static 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; >> + u32 reg; >> + struct led_init_data init_data = {}; >> + >> + device_for_each_child_node(priv->dev, child) { > > Please move above init_data initialization here. No problem. > > >> + led = &priv->leds[i]; > > You can increment 'i' here, it is not used below and you will save one > LOC: > > led = &priv->leds[i++]; Agree. > >> + >> + init_data.fwnode = child; > > Please move it to where it will be needed. i.e. one line above > devm_led_classdev_register_ext(). Will do. > >> + >> + 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; > > I'd just pass &led->reg directly to fwnode_property_read_u32(). > Then you'll be able to drop local reg variable. For this device/driver reg value is just one byte You are ok to waste 3 bytes for every EL15203000 LED? > >> + >> + fwnode_property_read_string(child, "linux,default-trigger", >> + &led->ldev.default_trigger); >> + >> + led->priv = priv; >> + led->ldev.max_brightness = LED_ON; >> + 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_ext(priv->dev, &led->ldev, >> + &init_data); >> + if (ret) { >> + dev_err(priv->dev, >> + "failed to register LED device %s, err %d", >> + led->ldev.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); >> + >> + 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"); >> > > -- > Best regards, > Jacek Anaszewski
Hi Oleh, On 9/5/19 8:17 AM, Oleh Kravchenko wrote: > Hello Jacek, > Few question from my side just for better understanding :) > >> 5 вер. 2019 р. о 12:23 дп Jacek Anaszewski <jacek.anaszewski@gmail.com> написав(ла): >> >> Hi Oleh, >> >> Thank you for the updated set. >> >> Now it looks really good. Just few minor issues left. >> >> And side note - please address the patches also to maintainers, >> not only to the list. >> >> On 8/31/19 12:46 AM, 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 | 362 ++++++++++++++++++ >>> 4 files changed, 398 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..767763409125 >>> --- /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 >> >> Now it will be September and 5.5. It is late even for 5.4. > > Here should month of patch creation or when it will be reviewed? :) > Because I sent in 31 August. It should reflect the kernel version in which the ABI is supposed to appear. >>> +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" >>> + >>> + Inverted cascade mode for Pipe LED: >>> + "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 1988de1d64c0..6e7703fd03d0 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -132,6 +132,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 41fb073a39c1..2da39e896ce8 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -89,6 +89,7 @@ obj-$(CONFIG_LEDS_LM36274) += leds-lm36274.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..9de81dee3618 >>> --- /dev/null >>> +++ b/drivers/leds/leds-el15203000.c >>> @@ -0,0 +1,362 @@ >>> +// 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/module.h> >>> +#include <linux/of_device.h> >>> +#include <linux/spi/spi.h> >>> + >>> +/* >>> + * EL15203000 SPI protocol description: >>> + * +-----+---------+ >>> + * | LED | COMMAND | >>> + * +-----+---------+ >>> + * | 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 | >>> + * +----------+--------------+-------------------------------------------+ >>> + * >>> + * COMMAND: >>> + * +----------+-----------------+--------------+--------------+ >>> + * | 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 20000ul >>> +#define EL_PATTERN_DELAY_MSEC 800u >>> +#define EL_PATTERN_LEN 10u >>> +#define EL_PATTERN_HALF_LEN (EL_PATTERN_LEN / 2) >>> + >>> +enum el15203000_command { >>> + /* 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; >>> + 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; >>> + >>> + mutex_lock(&led->priv->lock); >>> + >>> + dev_dbg(led->priv->dev, "Set brightness of 0x%02x(%c) to 0x%02x(%c)", >>> + led->reg, led->reg, brightness, brightness); >>> + >>> + /* to avoid SPI mistiming with firmware we should wait some time */ >>> + if (time_after(led->priv->delay, jiffies)) { >>> + dev_dbg(led->priv->dev, "Wait %luus to sync", >>> + EL_FW_DELAY_USEC); >>> + >>> + usleep_range(EL_FW_DELAY_USEC, >>> + EL_FW_DELAY_USEC + 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 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); >>> + >>> + if (repeat > 0 || len != 2 || >>> + pattern[0].delta_t != 4000 || pattern[0].brightness != 0 || >>> + pattern[1].delta_t != 4000 || pattern[1].brightness != 1) >>> + return -EINVAL; >>> + >>> + dev_dbg(led->priv->dev, "Breathing mode for 0x%02x(%c)", >>> + led->reg, led->reg); >>> + >>> + return el15203000_cmd(led, EL_SCREEN_BREATHING); >>> +} >>> + >>> +static bool is_cascade(const struct led_pattern *pattern, u32 len, >>> + bool inv, bool right) >>> +{ >>> + int val, t; >>> + u32 i; >>> + >>> + if (len != EL_PATTERN_HALF_LEN) >>> + return false; >>> + >>> + val = right ? BIT(4) : BIT(0); >>> + >>> + for (i = 0; i < len; i++) { >>> + t = inv ? ~val & GENMASK(4, 0) : val; >>> + >>> + if (pattern[i].delta_t != EL_PATTERN_DELAY_MSEC || >>> + pattern[i].brightness != t) >>> + return false; >>> + >>> + val = right ? val >> 1 : val << 1; >>> + } >> >> Nice! >> >>> + >>> + return true; >>> +} >>> + >>> +static bool is_bounce(const struct led_pattern *pattern, u32 len, bool inv) >>> +{ >>> + if (len != EL_PATTERN_LEN) >>> + return false; >>> + >>> + return is_cascade(pattern, EL_PATTERN_HALF_LEN, inv, false) && >>> + is_cascade(pattern + EL_PATTERN_HALF_LEN, >>> + EL_PATTERN_HALF_LEN, inv, true); >>> +} >>> + >>> +static 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); >>> + >>> + if (repeat > 0) >> >> This is wrong. Repeat has to be -1 or > 0. If all patterns supported >> by your device are infinite, then you should expect here -1. > > Ok, then we have bug in led pattern trigger. > echo -1 > /…/repeat doesn’t work as expected and return error. > So if I tried to change repeat from sysfs, it will never be -1 again. Yes, I see now that we probably have a bug in repeat_store() in drivers/leds/trigger/ledtrig-pattern.c.. It assumes that we have already some pattern tuples defined and works properly only then. It is not prepared for a case when we want to set repeat count before defining the pattern and attempts to immediately start pattern via pattern_trig_start_pattern(), which returns error when no proper pattern is defined. To fix that we could perhaps attempt to (re)start pattern in repeat_store() only if data->npatterns > 0. It would be even consequent with what we have there already few lines above where led_cdev->pattern_clear(led_cdev) is guarded similarly. >> >> Either way this needs to be covered in the ABI documentation too. >> >>> + return -EINVAL; >>> + >>> + if (is_cascade(pattern, len, false, false)) { >>> + dev_dbg(led->priv->dev, "Cascade mode for 0x%02x(%c)", >>> + led->reg, led->reg); >>> + >>> + return el15203000_cmd(led, EL_PIPE_CASCADE); >>> + } else if (is_cascade(pattern, len, true, false)) { >>> + dev_dbg(led->priv->dev, "Inverse cascade mode for 0x%02x(%c)", >>> + led->reg, led->reg); >>> + >>> + return el15203000_cmd(led, EL_PIPE_INV_CASCADE); >>> + } else if (is_bounce(pattern, len, false)) { >>> + dev_dbg(led->priv->dev, "Bounce mode for 0x%02x(%c)", >>> + led->reg, led->reg); >>> + >>> + return el15203000_cmd(led, EL_PIPE_BOUNCE); >>> + } else if (is_bounce(pattern, len, true)) { >>> + dev_dbg(led->priv->dev, "Inverse bounce mode for 0x%02x(%c)", >>> + led->reg, led->reg); >>> + >>> + return el15203000_cmd(led, EL_PIPE_INV_BOUNCE); >>> + } >>> + >>> + return -EINVAL; >>> +} >>> + >>> +static 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; >>> + u32 reg; >>> + struct led_init_data init_data = {}; >>> + >>> + device_for_each_child_node(priv->dev, child) { >> >> Please move above init_data initialization here. > > No problem. > >> >> >>> + led = &priv->leds[i]; >> >> You can increment 'i' here, it is not used below and you will save one >> LOC: >> >> led = &priv->leds[i++]; > > Agree. > >> >>> + >>> + init_data.fwnode = child; >> >> Please move it to where it will be needed. i.e. one line above >> devm_led_classdev_register_ext(). > > Will do. > >> >>> + >>> + 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; >> >> I'd just pass &led->reg directly to fwnode_property_read_u32(). >> Then you'll be able to drop local reg variable. > > For this device/driver reg value is just one byte > You are ok to waste 3 bytes for every EL15203000 LED? You're wasting them anyway due to alignment. If you really cared about that you could add __packed modifier to the struct definition but I don't think it makes big difference in this case. >> >>> + >>> + fwnode_property_read_string(child, "linux,default-trigger", >>> + &led->ldev.default_trigger); >>> + >>> + led->priv = priv; >>> + led->ldev.max_brightness = LED_ON; >>> + 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_ext(priv->dev, &led->ldev, >>> + &init_data); >>> + if (ret) { >>> + dev_err(priv->dev, >>> + "failed to register LED device %s, err %d", >>> + led->ldev.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); >>> + >>> + 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"); >>> >> >> -- >> Best regards, >> Jacek Anaszewski >
diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt new file mode 100644 index 000000000000..c00e1b55db97 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt @@ -0,0 +1,54 @@ +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: +- 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) + +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 */ + led@50 { + reg = <0x50>; + function = "pipe"; + color = <LED_COLOR_ID_RED>; + }; + + /* screen frame */ + led@53 { + reg = <0x53>; + function = "screen"; + color = <LED_COLOR_ID_RED>; + }; + + /* vending area */ + led@56 { + reg = <0x56>; + function = "vend"; + color = <LED_COLOR_ID_RED>; + }; +};
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 | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt