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@xxxxxxxxxx>
> ---
> .../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@xxxxxxxxxx>
> +
> +#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@xxxxxxxxxx>");
> +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
