linux-leds.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* v7 EL15203000
@ 2019-09-09  7:16 Oleh Kravchenko
  2019-09-09  7:16 ` [PATCH v7 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
  2019-09-09  7:16 ` [PATCH v7 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
  0 siblings, 2 replies; 6+ messages in thread
From: Oleh Kravchenko @ 2019-09-09  7:16 UTC (permalink / raw)
  To: devicetree, linux-leds, jacek.anaszewski, pavel

[PATCH v7 1/2] dt-bindings: Add docs for EL15203000
[PATCH v7 2/2] leds: add LED driver for EL15203000 board

Fixed compilation error missing variable led.
Compiled and checked on real device.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v7 1/2] dt-bindings: Add docs for EL15203000
  2019-09-09  7:16 v7 EL15203000 Oleh Kravchenko
@ 2019-09-09  7:16 ` Oleh Kravchenko
  2019-09-11 19:48   ` Dan Murphy
  2019-09-09  7:16 ` [PATCH v7 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
  1 sibling, 1 reply; 6+ messages in thread
From: Oleh Kravchenko @ 2019-09-09  7:16 UTC (permalink / raw)
  To: devicetree, linux-leds, jacek.anaszewski, pavel
  Cc: Oleh Kravchenko, Rob Herring

Add documentation and example for dt-bindings EL15203000.
LED board (aka RED LED board) from Crane Merchandising Systems.

Reviewed-by: Rob Herring <robh@kernel.org>
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

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>;
+	};
+};
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v7 2/2] leds: add LED driver for EL15203000 board
  2019-09-09  7:16 v7 EL15203000 Oleh Kravchenko
  2019-09-09  7:16 ` [PATCH v7 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
@ 2019-09-09  7:16 ` Oleh Kravchenko
  2019-09-11 19:49   ` Dan Murphy
  1 sibling, 1 reply; 6+ messages in thread
From: Oleh Kravchenko @ 2019-09-09  7:16 UTC (permalink / raw)
  To: devicetree, linux-leds, jacek.anaszewski, pavel; +Cc: Oleh Kravchenko

This patch adds a LED class driver for the 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 |  32 ++
 drivers/leds/Kconfig                          |  13 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-el15203000.c                | 356 ++++++++++++++++++
 4 files changed, 402 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..5e9cbf49da59
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-driver-el15203000
@@ -0,0 +1,32 @@
+What:		/sys/class/leds/<led>/hw_pattern
+Date:		September 2019
+KernelVersion:	5.5
+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"
+
+What:		/sys/class/leds/<led>/repeat
+Date:		September 2019
+KernelVersion:	5.5
+Description:
+		EL15203000 supports only indefinitely patterns,
+		so this file should always store -1.
+
+		For more info, please see:
+		Documentation/ABI/testing/sysfs-class-led-trigger-pattern
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..1d59a53f9ecc
--- /dev/null
+++ b/drivers/leds/leds-el15203000.c
@@ -0,0 +1,356 @@
+// 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;
+	u32			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;
+	}
+
+	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)
+		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)
+{
+	struct el15203000_led	*led = priv->leds;
+	struct fwnode_handle	*child;
+	int			ret;
+
+	device_for_each_child_node(priv->dev, child) {
+		struct led_init_data	init_data = {};
+
+		ret = fwnode_property_read_u32(child, "reg", &led->reg);
+		if (ret) {
+			dev_err(priv->dev, "LED without ID number");
+			fwnode_handle_put(child);
+
+			return ret;
+		}
+
+		if (led->reg > U8_MAX) {
+			dev_err(priv->dev, "LED value %d is invalid", led->reg);
+			fwnode_handle_put(child);
+
+			return -EINVAL;
+		}
+
+		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 (led->reg == 'S') {
+			led->ldev.pattern_set	= el15203000_pattern_set_S;
+			led->ldev.pattern_clear	= el15203000_pattern_clear;
+		} else if (led->reg == 'P') {
+			led->ldev.pattern_set	= el15203000_pattern_set_P;
+			led->ldev.pattern_clear	= el15203000_pattern_clear;
+		}
+
+		init_data.fwnode = child;
+		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;
+		}
+
+		led++;
+	}
+
+	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");
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: Add docs for EL15203000
  2019-09-09  7:16 ` [PATCH v7 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
@ 2019-09-11 19:48   ` Dan Murphy
  2019-09-11 21:09     ` Jacek Anaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2019-09-11 19:48 UTC (permalink / raw)
  To: Oleh Kravchenko, devicetree, linux-leds, jacek.anaszewski, pavel
  Cc: Rob Herring

Oleh

On 9/9/19 2:16 AM, Oleh Kravchenko wrote:
> Add documentation and example for dt-bindings EL15203000.
> LED board (aka RED LED board) from Crane Merchandising Systems.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> 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
>
> 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

FYI this binding Documentation/devicetree/bindings/spi/spi-bus.txt 
points to spi-controller.yaml binding

so maybe update it to the correct binding.

> +- spi-max-frequency : (optional)
> +	see Documentation/devicetree/bindings/spi/spi-bus.txt
Your driver does not read spi-max-frequency, this is a property of the 
spi driver so I am not sure you

need to mention that here unless this property needs to be set 
specifically for this device.  If it does then it is not optional

for this device and it should be documented what the max freq is.

> +
> +Optional LED sub-node properties:
> +- function:
> +	see Documentation/devicetree/bindings/leds/common.txt

You point the user to the common file but you use hard coded function 
names in the example.

The leds/common.txt file points to the include/dt-bindings/leds/common.h 
file and the binding says

"If there is no matching LED_FUNCTION available, add a new one."

Now I know we don't want to add the pipe, screen or vend so you probably 
do not want to have the user

going to that header.

Not sure how to fix this but the documentation is misleading. Jacek?


> +- color:
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- label:
> +	see Documentation/devicetree/bindings/leds/common.txt (deprecated)

Not sure if someone asked for this here but since this is a new driver 
it should not even speak of the "label" property.

Dan

<snip>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 2/2] leds: add LED driver for EL15203000 board
  2019-09-09  7:16 ` [PATCH v7 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
@ 2019-09-11 19:49   ` Dan Murphy
  0 siblings, 0 replies; 6+ messages in thread
From: Dan Murphy @ 2019-09-11 19:49 UTC (permalink / raw)
  To: Oleh Kravchenko, devicetree, linux-leds, jacek.anaszewski, pavel

Oleh

On 9/9/19 2:16 AM, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the 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 |  32 ++
>   drivers/leds/Kconfig                          |  13 +
>   drivers/leds/Makefile                         |   1 +
>   drivers/leds/leds-el15203000.c                | 356 ++++++++++++++++++
>   4 files changed, 402 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-el15203000
>   create mode 100644 drivers/leds/leds-el15203000.c

[...]

> +
> +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)
> +		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);
> +	}
> +

nitpicking a bit not a blocker just some clean up

maybe remove the dev_dbg statements and just set a local variable to the 
pipe cmd

if (is_cascade(pattern, len, false, false))

     pipe_cmd = EL_PIPE_CASCADE;

else if (is_cascade(pattern, len, true, false))

     pipe_cmd = EL_PIPE_INV_CASCADE;

else

     return -EINVAL;


return el15203000_cmd(led, pipe_cmd0:

> +	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)
> +{
> +	struct el15203000_led	*led = priv->leds;
> +	struct fwnode_handle	*child;
> +	int			ret;
> +
> +	device_for_each_child_node(priv->dev, child) {
> +		struct led_init_data	init_data = {};
> +
> +		ret = fwnode_property_read_u32(child, "reg", &led->reg);
> +		if (ret) {
> +			dev_err(priv->dev, "LED without ID number");
> +			fwnode_handle_put(child);
> +
> +			return ret;
> +		}
> +
> +		if (led->reg > U8_MAX) {
> +			dev_err(priv->dev, "LED value %d is invalid", led->reg);
> +			fwnode_handle_put(child);
> +
> +			return -EINVAL;
> +		}
> +
> +		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 (led->reg == 'S') {
> +			led->ldev.pattern_set	= el15203000_pattern_set_S;
> +			led->ldev.pattern_clear	= el15203000_pattern_clear;
> +		} else if (led->reg == 'P') {
> +			led->ldev.pattern_set	= el15203000_pattern_set_P;
> +			led->ldev.pattern_clear	= el15203000_pattern_clear;
> +		}
> +
> +		init_data.fwnode = child;
> +		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;
> +		}
> +
> +		led++;
> +	}
> +
> +	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;

Another nitpick again just some clean up.

Set spi_set_drvdata before you call the probe_dt then return on 
el15203000_probe_dt this will allow you to eliminate the local ret variable.

so it would look like this:

spi_set_drvdata(spi, priv);

return el15203000_probe_dt(priv);

Otherwise

Reviewed-by: Dan Murphy <dmurphy@ti.com>

<snip>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 1/2] dt-bindings: Add docs for EL15203000
  2019-09-11 19:48   ` Dan Murphy
@ 2019-09-11 21:09     ` Jacek Anaszewski
  0 siblings, 0 replies; 6+ messages in thread
From: Jacek Anaszewski @ 2019-09-11 21:09 UTC (permalink / raw)
  To: Dan Murphy, Oleh Kravchenko, devicetree, linux-leds, pavel; +Cc: Rob Herring

On 9/11/19 9:48 PM, Dan Murphy wrote:
> Oleh
> 
> On 9/9/19 2:16 AM, Oleh Kravchenko wrote:
>> Add documentation and example for dt-bindings EL15203000.
>> LED board (aka RED LED board) from Crane Merchandising Systems.
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> 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
>>
>> 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
> 
> FYI this binding Documentation/devicetree/bindings/spi/spi-bus.txt
> points to spi-controller.yaml binding
> 
> so maybe update it to the correct binding.
> 
>> +- spi-max-frequency : (optional)
>> +    see Documentation/devicetree/bindings/spi/spi-bus.txt
> Your driver does not read spi-max-frequency, this is a property of the
> spi driver so I am not sure you
> 
> need to mention that here unless this property needs to be set
> specifically for this device.  If it does then it is not optional
> 
> for this device and it should be documented what the max freq is.

Right, this should have different form. We have two similar occurrences
in other LED bindings and in a few bindings pertinent to other
subsystems. This property is parsed in spi.c for each child node
of parent spi node.

It should be like below:

Property rules described in
Documentation/devicetree/bindings/spi/spi-bus.txt
apply. In particular, "reg" and "spi-max-frequency" properties must be
given.

>> +
>> +Optional LED sub-node properties:
>> +- function:
>> +    see Documentation/devicetree/bindings/leds/common.txt
> 
> You point the user to the common file but you use hard coded function
> names in the example.
> 
> The leds/common.txt file points to the include/dt-bindings/leds/common.h
> file and the binding says
> 
> "If there is no matching LED_FUNCTION available, add a new one."
> 
> Now I know we don't want to add the pipe, screen or vend so you probably
> do not want to have the user
> 
> going to that header.
> 
> Not sure how to fix this but the documentation is misleading. Jacek?

I'd just resort to common sense. If the function seems to be generic
enough then it deserves its own definition. We might change it to:

"If there is no matching LED_FUNCTION available and it is plausible
that it will be in demand in the future, add a new one."

> 
>> +- color:
>> +    see Documentation/devicetree/bindings/leds/common.txt
>> +- label:
>> +    see Documentation/devicetree/bindings/leds/common.txt (deprecated)
> 
> Not sure if someone asked for this here but since this is a new driver
> it should not even speak of the "label" property.

Agreed.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-09-11 21:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09  7:16 v7 EL15203000 Oleh Kravchenko
2019-09-09  7:16 ` [PATCH v7 1/2] dt-bindings: Add docs for EL15203000 Oleh Kravchenko
2019-09-11 19:48   ` Dan Murphy
2019-09-11 21:09     ` Jacek Anaszewski
2019-09-09  7:16 ` [PATCH v7 2/2] leds: add LED driver for EL15203000 board Oleh Kravchenko
2019-09-11 19:49   ` Dan Murphy

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).