[PATCH] leds: add LED driver for CR0014114 board

[PATCH] leds: add LED driver for CR0014114 board

[Oleh Kravchenko]

This patch adds a LED class driver for the RGB LEDs found on
the Crane Merchandising System CR0014114 LEDs board.

Driver creates LED devices with name written using the following
pattern "LABEL-{N}:{red,green,blue}:".

Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
 .../devicetree/bindings/leds/leds-cr0014114.txt    |  23 ++
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 drivers/leds/Kconfig                               |  12 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-cr0014114.c                      | 354 +++++++++++++++++++++
 5 files changed, 391 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-cr0014114.txt
 create mode 100644 drivers/leds/leds-cr0014114.c

diff --git a/Documentation/devicetree/bindings/leds/leds-cr0014114.txt b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
new file mode 100644
index 000000000000..6a399cb65388
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-cr0014114.txt
@@ -0,0 +1,23 @@
+Crane Merchandising System SPI LED board CR0014114
+==================================================
+
+Required properties:
+- compatible: "cms,cr0014114"
+
+Optional properties:
+- label: prefix for LED names. If omitted the label is taken from the node name.
+- leds-per-board: RGB leds per board, minimal value is 6
+- num-boards: number of LED boards on SPI bus, usually 1
+
+Example
+-------
+
+cr0014114@0 {
+	compatible = "ccs,cr0014114";
+	reg = <0>;
+	spi-max-frequency = <50000>;
+	spi-cpha;
+	label = "cr0014114";
+	leds-per-board = /bits/ 8 <6>;
+	num-boards = /bits/ 8 <1>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 7e5754ecb095..ad72b278d307 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -67,6 +67,7 @@ chunghwa	Chunghwa Picture Tubes Ltd.
 ciaa	Computadora Industrial Abierta Argentina
 cirrus	Cirrus Logic, Inc.
 cloudengines	Cloud Engines, Inc.
+cms	Crane Merchandising System
 cnm	Chips&Media, Inc.
 cnxt	Conexant Systems, Inc.
 compulab	CompuLab Ltd.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 594b24d410c3..2d1527fc7643 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -76,6 +76,18 @@ config LEDS_BCM6358
 	  This option enables support for LEDs connected to the BCM6358
 	  LED HW controller accessed via MMIO registers.
 
+config LEDS_CR0014114
+	tristate "LED Support for Crane Merchandising Systems CR0014114"
+	depends on LEDS_CLASS
+	depends on SPI
+	depends on OF
+	help
+	  The CR0014114 LED board used in vending machines produced
+	  by Crane Merchandising Systems.
+
+	  This driver creates LED devices with name written using the
+	  following pattern "LABEL-{N}:{red,green,blue}:".
+
 config LEDS_CPCAP
 	tristate "LED Support for Motorola CPCAP"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 909dae62ba05..fd886a30267d 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -73,6 +73,7 @@ obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
 obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 
 # LED SPI Drivers
+obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
 
 # LED Userspace Drivers
diff --git a/drivers/leds/leds-cr0014114.c b/drivers/leds/leds-cr0014114.c
new file mode 100644
index 000000000000..1c3c90fcbd62
--- /dev/null
+++ b/drivers/leds/leds-cr0014114.c
@@ -0,0 +1,354 @@
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <linux/spinlock.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
+
+/* CR0014114 SPI commands */
+#define CR0014114_SET_BRIGHTNESS	0x80
+#define CR0014114_INIT_REENUMERATE	0x81
+#define CR0014114_NEXT_REENUMERATE	0x82
+
+/* default settings */
+#define CR0014114_LEDS_PER_BOARD	6
+#define CR0014114_MAX_BRIGHTNESS	GENMASK(6, 0)
+#define CR0014114_NUM_BOARDS		1
+#define CR0014114_FPS			50
+#define CR0014114_FW_DELAY_MSEC		10
+#define CR0014114_RECOUNT_DELAY		(HZ * 3600)
+
+struct cr0014114 {
+	struct spi_device	*spi;
+	struct device		*dev;
+	const char		*label;
+	u8			leds_per_board;
+	u8			num_boards;
+	size_t			leds_count;
+	char			wq_name[64];
+	struct workqueue_struct	*wq;
+	unsigned long		leds_delay;
+	struct work_struct	leds_work;
+	struct timer_list	recount_timer;
+	struct work_struct	recount_work;
+	struct cr0014114_led	*leds;
+};
+
+struct cr0014114_led {
+	char			name[64];
+	struct cr0014114	*priv;
+	struct led_classdev	ldev;
+	u8			brightness;
+};
+
+static void cr0014114_calc_crc(u8 *buf, const size_t len)
+{
+	u8	crc;
+	size_t	i;
+
+	for (i = 1, crc = 1; i < len - 1; i++)
+		crc += buf[i];
+
+	crc |= BIT(7);
+
+	/* special case when CRC matches to SPI commands */
+	switch (crc) {
+	case CR0014114_SET_BRIGHTNESS:
+	case CR0014114_INIT_REENUMERATE:
+	case CR0014114_NEXT_REENUMERATE:
+		crc = 0xfe;
+		break;
+	}
+
+	buf[len - 1] = crc;
+}
+
+static void cr0014114_leds_work(struct work_struct *work)
+{
+	int			ret;
+	size_t			i;
+	struct cr0014114	*priv = container_of(work,
+					struct cr0014114, leds_work);
+	u8			data[priv->leds_count + 2];
+	unsigned long		udelay, now = jiffies;
+
+	/* to avoid SPI mistiming with firmware we should wait some time */
+	if (time_after(priv->leds_delay, now)) {
+		udelay = jiffies_to_usecs(priv->leds_delay - now);
+		usleep_range(udelay, udelay + 1);
+	}
+
+	data[0] = CR0014114_SET_BRIGHTNESS;
+	for (i = 0; i < priv->leds_count; i++)
+		data[i + 1] = priv->leds[i].brightness;
+	cr0014114_calc_crc(data, sizeof(data));
+
+	ret = spi_write(priv->spi, data, sizeof(data));
+	if (ret)
+		dev_err(priv->dev, "spi_write() error %d\n", ret);
+
+	priv->leds_delay = jiffies +
+				msecs_to_jiffies(CR0014114_FW_DELAY_MSEC);
+}
+
+static void cr0014114_test(struct cr0014114 *priv)
+{
+	unsigned int		mdelay;
+	size_t			i;
+	struct led_classdev	*ldev;
+
+	/* blink all LEDs in 500 milliseconds */
+	mdelay = 500 / priv->leds_count - CR0014114_FW_DELAY_MSEC;
+	if (mdelay < CR0014114_FW_DELAY_MSEC)
+		mdelay = CR0014114_FW_DELAY_MSEC;
+
+	for (i = 0; i < priv->leds_count; i++) {
+		ldev = &priv->leds[i].ldev;
+
+		ldev->brightness_set(ldev, CR0014114_MAX_BRIGHTNESS);
+		msleep(mdelay);
+		ldev->brightness_set(ldev, LED_OFF);
+	}
+}
+
+static void cr0014114_set_brightness(struct led_classdev *ldev,
+				     enum led_brightness brightness)
+{
+	struct cr0014114_led	*led = container_of(ldev,
+					struct cr0014114_led, ldev);
+
+	led->brightness = brightness & CR0014114_MAX_BRIGHTNESS;
+	queue_work(led->priv->wq, &led->priv->leds_work);
+}
+
+static int cr0014114_led_create(struct cr0014114 *priv,
+				struct cr0014114_led *led,
+				size_t id,
+				const char *color)
+{
+	int ret;
+
+	led->priv = priv;
+	snprintf(led->name, sizeof(led->name), "%s-%zd:%s:",
+		 priv->label, id, color);
+
+	led->ldev.name			= led->name;
+	led->ldev.brightness		= LED_OFF;
+	led->ldev.max_brightness	= CR0014114_MAX_BRIGHTNESS;
+	led->ldev.brightness_set	= cr0014114_set_brightness;
+
+	ret = led_classdev_register(priv->dev, &led->ldev);
+	if (ret)
+		dev_err(priv->dev, "can't register LED '%s', error %d\n",
+			led->name, ret);
+
+	return ret;
+}
+
+static int cr0014114_recount(struct cr0014114 *priv)
+{
+	size_t		i;
+	int		ret;
+	u8		cmd_init = CR0014114_INIT_REENUMERATE;
+	u8		cmd_next = CR0014114_NEXT_REENUMERATE;
+
+	dev_dbg(priv->dev, "recount of LEDs is started\n");
+
+	msleep(CR0014114_FW_DELAY_MSEC);
+
+	ret = spi_write(priv->spi, &cmd_init, sizeof(cmd_init));
+	if (ret)
+		return ret;
+
+	for (i = 0; i < priv->leds_count; i++) {
+		msleep(CR0014114_FW_DELAY_MSEC);
+
+		ret = spi_write(priv->spi, &cmd_next, sizeof(cmd_next));
+		if (ret)
+			return ret;
+	}
+
+	priv->leds_delay = jiffies +
+				msecs_to_jiffies(CR0014114_FW_DELAY_MSEC);
+
+	dev_dbg(priv->dev, "recount of LEDs is complete\n");
+
+	return 0;
+}
+
+void cr0014114_recount_timer(unsigned long data)
+{
+	struct cr0014114 *priv = (struct cr0014114 *)data;
+
+	queue_work(priv->wq, &priv->recount_work);
+	mod_timer(&priv->recount_timer, jiffies + CR0014114_RECOUNT_DELAY);
+}
+
+static void cr0014114_recount_work(struct work_struct *work)
+{
+	struct cr0014114 *priv = container_of(work, struct cr0014114,
+					recount_work);
+
+	cr0014114_recount(priv);
+
+	/* after recount we should restore brightness */
+	queue_work(priv->wq, &priv->leds_work);
+}
+
+static int cr0014114_probe_dp(struct cr0014114 *priv)
+{
+	if (device_property_read_string(priv->dev, "label", &priv->label))
+		priv->label = priv->dev->of_node->name;
+	if (!priv->label || !*priv->label) {
+		dev_err(priv->dev, "'label' can't be empty\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u8(priv->dev, "leds-per-board",
+				    &priv->leds_per_board))
+		priv->leds_per_board = CR0014114_LEDS_PER_BOARD;
+	if (priv->leds_per_board < CR0014114_LEDS_PER_BOARD) {
+		dev_err(priv->dev,
+			"'leds-per-board' should be at least %d\n",
+			CR0014114_LEDS_PER_BOARD
+		);
+		return -EINVAL;
+	}
+
+	if (device_property_read_u8(priv->dev, "num-boards",
+				    &priv->num_boards))
+		priv->num_boards = CR0014114_NUM_BOARDS;
+	if (priv->num_boards < CR0014114_NUM_BOARDS) {
+		dev_err(priv->dev, "'num-boards' should be at least %d\n",
+			CR0014114_NUM_BOARDS
+		);
+		return -EINVAL;
+	}
+
+	priv->leds_count = priv->leds_per_board * priv->num_boards * 3;
+	priv->leds = devm_kzalloc(priv->dev, sizeof(*priv->leds) *
+			priv->leds_count, GFP_KERNEL);
+
+	if (!priv->leds)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static int cr0014114_probe(struct spi_device *spi)
+{
+	struct cr0014114	*priv;
+	size_t			i, id;
+	int			ret;
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->spi = spi;
+	priv->dev = &spi->dev;
+	INIT_WORK(&priv->leds_work, cr0014114_leds_work);
+	INIT_WORK(&priv->recount_work, cr0014114_recount_work);
+
+	ret = cr0014114_probe_dp(priv);
+	if (ret)
+		return ret;
+
+	ret = cr0014114_recount(priv);
+	if (ret) {
+		dev_err(priv->dev, "Recount failed with error %d\n", ret);
+
+		return ret;
+	}
+
+	snprintf(priv->wq_name, sizeof(priv->wq_name), "%s/work", priv->label);
+	priv->wq = create_singlethread_workqueue(priv->wq_name);
+	if (!priv->wq)
+		return -ENOMEM;
+
+	i = 0;
+	id = 0;
+
+	while (i < priv->leds_count) {
+		ret = cr0014114_led_create(priv, &priv->leds[i], id, "red");
+		if (ret)
+			goto fail;
+		i++;
+
+		ret = cr0014114_led_create(priv, &priv->leds[i], id, "green");
+		if (ret)
+			goto fail;
+		i++;
+
+		ret = cr0014114_led_create(priv, &priv->leds[i], id, "blue");
+		if (ret)
+			goto fail;
+		i++;
+		id++;
+	}
+
+	cr0014114_test(priv);
+	spi_set_drvdata(spi, priv);
+	setup_timer(&priv->recount_timer, cr0014114_recount_timer,
+		    (unsigned long)priv);
+
+	/* setup recount timer to workaround buggy firmware */
+	mod_timer(&priv->recount_timer, jiffies + CR0014114_RECOUNT_DELAY);
+
+	dev_info(priv->dev,
+		 "%s (Boards: %u, LEDs per board: %u, Total channels: %zd)\n",
+		 priv->label,
+		 priv->num_boards,
+		 priv->leds_per_board,
+		 priv->leds_count);
+
+	return 0;
+
+fail:
+	while (i--)
+		led_classdev_unregister(&priv->leds[i].ldev);
+
+	destroy_workqueue(priv->wq);
+
+	return ret;
+}
+
+static int cr0014114_remove(struct spi_device *spi)
+{
+	struct cr0014114	*priv = spi_get_drvdata(spi);
+	size_t			i;
+
+	for (i = 0; i < priv->leds_count; i++)
+		led_classdev_unregister(&priv->leds[i].ldev);
+
+	del_timer_sync(&priv->recount_timer);
+	destroy_workqueue(priv->wq);
+
+	return 0;
+}
+
+static const struct of_device_id cr0014114_dt_ids[] = {
+	{ .compatible = "cms,cr0014114", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, cr0014114_dt_ids);
+
+static struct spi_driver cr0014114_driver = {
+	.probe		= cr0014114_probe,
+	.remove		= cr0014114_remove,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= cr0014114_dt_ids,
+	},
+};
+
+module_spi_driver(cr0014114_driver);
+
+MODULE_AUTHOR("Oleh Kravchenko <oleg@kaa.org.ua>");
+MODULE_DESCRIPTION("cr0014114 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:cr0014114");
-- 
2.13.0

Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2472 bytes --]

Hi!

On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
> This patch adds a LED class driver for the RGB LEDs found on
> the Crane Merchandising System CR0014114 LEDs board.

What kind of hardware is this? 

> Driver creates LED devices with name written using the following
> pattern "LABEL-{N}:{red,green,blue}:".

Is the "-{N} suffix needed?

> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  23 ++
>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +

You'll need to cc device tree maintainers to get their acks. Also you
should probably cc: Jacek and me.

> @@ -76,6 +76,18 @@ config LEDS_BCM6358
>  	  This option enables support for LEDs connected to the BCM6358
>  	  LED HW controller accessed via MMIO registers.
>  
> +config LEDS_CR0014114
> +	tristate "LED Support for Crane Merchandising Systems CR0014114"
> +	depends on LEDS_CLASS
> +	depends on SPI
> +	depends on OF
> +	help
> +	  The CR0014114 LED board used in vending machines produced
> +	  by Crane Merchandising Systems.
> +
> +	  This driver creates LED devices with name written using the
> +	  following pattern "LABEL-{N}:{red,green,blue}:".

How special hardware is this? Does it make sense to ask this question
for x86 users, for example?

> +/* CR0014114 SPI commands */
> +#define CR0014114_SET_BRIGHTNESS	0x80
> +#define CR0014114_INIT_REENUMERATE	0x81
> +#define CR0014114_NEXT_REENUMERATE	0x82

can we s/cr0014114/cr00/g, or something? There are local to the
module, so they can be shorter...

> +static void cr0014114_test(struct cr0014114 *priv)
> +{
> +	unsigned int		mdelay;
> +	size_t			i;
> +	struct led_classdev	*ldev;
> +
> +	/* blink all LEDs in 500 milliseconds */
> +	mdelay = 500 / priv->leds_count - CR0014114_FW_DELAY_MSEC;
> +	if (mdelay < CR0014114_FW_DELAY_MSEC)
> +		mdelay = CR0014114_FW_DELAY_MSEC;
> +
> +	for (i = 0; i < priv->leds_count; i++) {
> +		ldev = &priv->leds[i].ldev;
> +
> +		ldev->brightness_set(ldev, CR0014114_MAX_BRIGHTNESS);
> +		msleep(mdelay);
> +		ldev->brightness_set(ldev, LED_OFF);
> +	}
> +}

I'd remove this.

> +fail:
> +	while (i--)
> +		led_classdev_unregister(&priv->leds[i].ldev);

Can devm_* be used to simplify this?

Thanks,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Oleh Kravchenko]

[-- Attachment #1.1: Type: text/plain, Size: 2963 bytes --]

Hello Pavel!

Thank you for your reply.

On 12.08.17 12:30, Pavel Machek wrote:
> Hi!
> 
> On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
>> This patch adds a LED class driver for the RGB LEDs found on
>> the Crane Merchandising System CR0014114 LEDs board.
> 
> What kind of hardware is this? 

It's from vending machines produced by Crane Merchandising System http://cranems.com/
 
>> Driver creates LED devices with name written using the following
>> pattern "LABEL-{N}:{red,green,blue}:".
> 
> Is the "-{N} suffix needed?

It's number of RGB LED, board has 6 LEDs.

>> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
>> ---
>>  .../devicetree/bindings/leds/leds-cr0014114.txt    |  23 ++
>>  .../devicetree/bindings/vendor-prefixes.txt        |   1 +
> 
> You'll need to cc device tree maintainers to get their acks. Also you
> should probably cc: Jacek and me.

Okay, I will add them.
 
>> @@ -76,6 +76,18 @@ config LEDS_BCM6358
>>  	  This option enables support for LEDs connected to the BCM6358
>>  	  LED HW controller accessed via MMIO registers.
>>  
>> +config LEDS_CR0014114
>> +	tristate "LED Support for Crane Merchandising Systems CR0014114"
>> +	depends on LEDS_CLASS
>> +	depends on SPI
>> +	depends on OF
>> +	help
>> +	  The CR0014114 LED board used in vending machines produced
>> +	  by Crane Merchandising Systems.
>> +
>> +	  This driver creates LED devices with name written using the
>> +	  following pattern "LABEL-{N}:{red,green,blue}:".
> 
> How special hardware is this? Does it make sense to ask this question
> for x86 users, for example?

I think it's possible to connect it to x86 hardware, but I don't try it.
Board need not only SPI connection, but good power supply.
 
>> +/* CR0014114 SPI commands */
>> +#define CR0014114_SET_BRIGHTNESS	0x80
>> +#define CR0014114_INIT_REENUMERATE	0x81
>> +#define CR0014114_NEXT_REENUMERATE	0x82
> 
> can we s/cr0014114/cr00/g, or something? There are local to the
> module, so they can be shorter...

Sure, may be CMD_SET_BRIGHTNESS
 
>> +static void cr0014114_test(struct cr0014114 *priv)
>> +{
>> +	unsigned int		mdelay;
>> +	size_t			i;
>> +	struct led_classdev	*ldev;
>> +
>> +	/* blink all LEDs in 500 milliseconds */
>> +	mdelay = 500 / priv->leds_count - CR0014114_FW_DELAY_MSEC;
>> +	if (mdelay < CR0014114_FW_DELAY_MSEC)
>> +		mdelay = CR0014114_FW_DELAY_MSEC;
>> +
>> +	for (i = 0; i < priv->leds_count; i++) {
>> +		ldev = &priv->leds[i].ldev;
>> +
>> +		ldev->brightness_set(ldev, CR0014114_MAX_BRIGHTNESS);
>> +		msleep(mdelay);
>> +		ldev->brightness_set(ldev, LED_OFF);
>> +	}
>> +}
> 
> I'd remove this.

I will do it.
 
>> +fail:
>> +	while (i--)
>> +		led_classdev_unregister(&priv->leds[i].ldev);
> 
> Can devm_* be used to simplify this?

I think no, because it will cause race condition.
 
> Thanks,
> 									Pavel
> 

-- 
Best regards,
Oleh Kravchenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

Hi!

> > On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
> >> This patch adds a LED class driver for the RGB LEDs found on
> >> the Crane Merchandising System CR0014114 LEDs board.
> > 
> > What kind of hardware is this? 
> 
> It's from vending machines produced by Crane Merchandising System http://cranems.com/
>  
> >> Driver creates LED devices with name written using the following
> >> pattern "LABEL-{N}:{red,green,blue}:".
> > 
> > Is the "-{N} suffix needed?
> 
> It's number of RGB LED, board has 6 LEDs.

Normally label differentiates them...?

> > How special hardware is this? Does it make sense to ask this question
> > for x86 users, for example?
> 
> I think it's possible to connect it to x86 hardware, but I don't try it.
> Board need not only SPI connection, but good power supply.

Aha, ok.

> >> +/* CR0014114 SPI commands */
> >> +#define CR0014114_SET_BRIGHTNESS	0x80
> >> +#define CR0014114_INIT_REENUMERATE	0x81
> >> +#define CR0014114_NEXT_REENUMERATE	0x82
> > 
> > can we s/cr0014114/cr00/g, or something? There are local to the
> > module, so they can be shorter...
> 
> Sure, may be CMD_SET_BRIGHTNESS

Please do the replace for all static symbols.> >> +fail:

> >> +	while (i--)
> >> +		led_classdev_unregister(&priv->leds[i].ldev);
> > 
> > Can devm_* be used to simplify this?
> 
> I think no, because it will cause race condition.

Please take a look at devm_led_classdev_register() and friends. It
should be possible to simplify code without races.

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Oleh Kravchenko]

[-- Attachment #1.1: Type: text/plain, Size: 3013 bytes --]

On 12.08.17 13:56, Pavel Machek wrote:
> Hi!
> 
>>> On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
>>>> This patch adds a LED class driver for the RGB LEDs found on
>>>> the Crane Merchandising System CR0014114 LEDs board.
>>>
>>> What kind of hardware is this? 
>>
>> It's from vending machines produced by Crane Merchandising System http://cranems.com/
>>  
>>>> Driver creates LED devices with name written using the following
>>>> pattern "LABEL-{N}:{red,green,blue}:".
>>>
>>> Is the "-{N} suffix needed?
>>
>> It's number of RGB LED, board has 6 LEDs.
> 
> Normally label differentiates them...?

For example, possible names when two boards connected is:
/sys/class/leds/board0-0:blue:
/sys/class/leds/board0-0:green:
/sys/class/leds/board0-0:red:
/sys/class/leds/board0-1:blue:
/sys/class/leds/board0-1:green:
/sys/class/leds/board0-1:red:
/sys/class/leds/board0-2:blue:
/sys/class/leds/board0-2:green:
/sys/class/leds/board0-2:red:
/sys/class/leds/board0-3:blue:
/sys/class/leds/board0-3:green:
/sys/class/leds/board0-3:red:
/sys/class/leds/board0-4:blue:
/sys/class/leds/board0-4:green:
/sys/class/leds/board0-4:red:
/sys/class/leds/board0-5:blue:
/sys/class/leds/board0-5:green:
/sys/class/leds/board0-5:red:

/sys/class/leds/board1-0:blue:
/sys/class/leds/board1-0:green:
/sys/class/leds/board1-0:red:
/sys/class/leds/board1-1:blue:
/sys/class/leds/board1-1:green:
/sys/class/leds/board1-1:red:
/sys/class/leds/board1-2:blue:
/sys/class/leds/board1-2:green:
/sys/class/leds/board1-2:red:
/sys/class/leds/board1-3:blue:
/sys/class/leds/board1-3:green:
/sys/class/leds/board1-3:red:
/sys/class/leds/board1-4:blue:
/sys/class/leds/board1-4:green:
/sys/class/leds/board1-4:red:
/sys/class/leds/board1-5:blue:
/sys/class/leds/board1-5:green:
/sys/class/leds/board1-5:red:
 
>>> How special hardware is this? Does it make sense to ask this question
>>> for x86 users, for example?
>>
>> I think it's possible to connect it to x86 hardware, but I don't try it.
>> Board need not only SPI connection, but good power supply.
> 
> Aha, ok.
> 
>>>> +/* CR0014114 SPI commands */
>>>> +#define CR0014114_SET_BRIGHTNESS	0x80
>>>> +#define CR0014114_INIT_REENUMERATE	0x81
>>>> +#define CR0014114_NEXT_REENUMERATE	0x82
>>>
>>> can we s/cr0014114/cr00/g, or something? There are local to the
>>> module, so they can be shorter...
>>
>> Sure, may be CMD_SET_BRIGHTNESS
> 
> Please do the replace for all static symbols.> >> +fail:

Of course (CMD_, DEF_, ..)
 
>>>> +	while (i--)
>>>> +		led_classdev_unregister(&priv->leds[i].ldev);
>>>
>>> Can devm_* be used to simplify this?
>>
>> I think no, because it will cause race condition.
> 
> Please take a look at devm_led_classdev_register() and friends. It
> should be possible to simplify code without races.

I don't understand how I can call destroy_workqueue() after calling unregister leds.
 
> Thanks,
> 									Pavel
> 

-- 
Best regards,
Oleh Kravchenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

On Sat 2017-08-12 14:34:02, Oleh Kravchenko wrote:
> On 12.08.17 13:56, Pavel Machek wrote:
> > Hi!
> > 
> >>> On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
> >>>> This patch adds a LED class driver for the RGB LEDs found on
> >>>> the Crane Merchandising System CR0014114 LEDs board.
> >>>
> >>> What kind of hardware is this? 
> >>
> >> It's from vending machines produced by Crane Merchandising System http://cranems.com/
> >>  
> >>>> Driver creates LED devices with name written using the following
> >>>> pattern "LABEL-{N}:{red,green,blue}:".
> >>>
> >>> Is the "-{N} suffix needed?
> >>
> >> It's number of RGB LED, board has 6 LEDs.
> > 
> > Normally label differentiates them...?
> 
> For example, possible names when two boards connected is:
> /sys/class/leds/board0-0:blue:
> /sys/class/leds/board0-0:green:
> /sys/class/leds/board0-0:red:

Hmm. Well, this works if you can't provide better names. Still "board"
is somehow generic, andsomeone else might want to use it. "board" ->
"crms"?

> >>>> +	while (i--)
> >>>> +		led_classdev_unregister(&priv->leds[i].ldev);
> >>>
> >>> Can devm_* be used to simplify this?
> >>
> >> I think no, because it will cause race condition.
> > 
> > Please take a look at devm_led_classdev_register() and friends. It
> > should be possible to simplify code without races.
> 
> I don't understand how I can call destroy_workqueue() after calling unregister leds.

Do you actually need the workqueues? It should be possible to avoid
them, using workqueue support in the core.
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Oleh Kravchenko]

[-- Attachment #1.1: Type: text/plain, Size: 2236 bytes --]

On 12.08.17 20:46, Pavel Machek wrote:
> On Sat 2017-08-12 14:34:02, Oleh Kravchenko wrote:
>> On 12.08.17 13:56, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> On Sat 2017-08-12 12:09:35, Oleh Kravchenko wrote:
>>>>>> This patch adds a LED class driver for the RGB LEDs found on
>>>>>> the Crane Merchandising System CR0014114 LEDs board.
>>>>>
>>>>> What kind of hardware is this? 
>>>>
>>>> It's from vending machines produced by Crane Merchandising System http://cranems.com/
>>>>  
>>>>>> Driver creates LED devices with name written using the following
>>>>>> pattern "LABEL-{N}:{red,green,blue}:".
>>>>>
>>>>> Is the "-{N} suffix needed?
>>>>
>>>> It's number of RGB LED, board has 6 LEDs.
>>>
>>> Normally label differentiates them...?
>>
>> For example, possible names when two boards connected is:
>> /sys/class/leds/board0-0:blue:
>> /sys/class/leds/board0-0:green:
>> /sys/class/leds/board0-0:red:
> 
> Hmm. Well, this works if you can't provide better names. Still "board"
> is somehow generic, andsomeone else might want to use it. "board" ->
> "crms"?

It's specified in device tree by defining label.
In my examples:
board@0 {
       compatible = "ccs,cr0014114";
       reg = <0>;
       spi-max-frequency = <50000>;
       spi-cpha;
       label = "board0";
};

board@1 {
       compatible = "ccs,cr0014114";
       reg = <1>;
       spi-max-frequency = <50000>;
       spi-cpha;
       label = "board1";
};
 
>>>>>> +	while (i--)
>>>>>> +		led_classdev_unregister(&priv->leds[i].ldev);
>>>>>
>>>>> Can devm_* be used to simplify this?
>>>>
>>>> I think no, because it will cause race condition.
>>>
>>> Please take a look at devm_led_classdev_register() and friends. It
>>> should be possible to simplify code without races.
>>
>> I don't understand how I can call destroy_workqueue() after calling unregister leds.
> 
> Do you actually need the workqueues? It should be possible to avoid
> them, using workqueue support in the core.

The delay between data sends to SPI board should be at least 10 ms
I think it will be bad idea to use shared workqueue from kernel,
so I create separate single threaded work queue :)

-- 
Best regards,
Oleh Kravchenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

Hi!

> > Hmm. Well, this works if you can't provide better names. Still "board"
> > is somehow generic, andsomeone else might want to use it. "board" ->
> > "crms"?
> 
> It's specified in device tree by defining label.
> In my examples:
> board@0 {
>        compatible = "ccs,cr0014114";
>        reg = <0>;
>        spi-max-frequency = <50000>;
>        spi-cpha;
>        label = "board0";
> };
> 
> board@1 {
>        compatible = "ccs,cr0014114";
>        reg = <1>;
>        spi-max-frequency = <50000>;
>        spi-cpha;
>        label = "board1";
> };

Ok, makes sense. How does the board look?

> >>>>>> +	while (i--)
> >>>>>> +		led_classdev_unregister(&priv->leds[i].ldev);
> >>>>>
> >>>>> Can devm_* be used to simplify this?
> >>>>
> >>>> I think no, because it will cause race condition.
> >>>
> >>> Please take a look at devm_led_classdev_register() and friends. It
> >>> should be possible to simplify code without races.
> >>
> >> I don't understand how I can call destroy_workqueue() after calling unregister leds.
> > 
> > Do you actually need the workqueues? It should be possible to avoid
> > them, using workqueue support in the core.
> 
> The delay between data sends to SPI board should be at least 10 ms
> I think it will be bad idea to use shared workqueue from kernel,
> so I create separate single threaded work queue :)

If workqueue support in led core can not take 10 msec wait, we should
fix it, not add hacks around...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Oleh Kravchenko]

[-- Attachment #1.1: Type: text/plain, Size: 1788 bytes --]

On 12.08.17 22:13, Pavel Machek wrote:
> Hi!
> 
>>> Hmm. Well, this works if you can't provide better names. Still "board"
>>> is somehow generic, andsomeone else might want to use it. "board" ->
>>> "crms"?
>>
>> It's specified in device tree by defining label.
>> In my examples:
>> board@0 {
>>        compatible = "ccs,cr0014114";
>>        reg = <0>;
>>        spi-max-frequency = <50000>;
>>        spi-cpha;
>>        label = "board0";
>> };
>>
>> board@1 {
>>        compatible = "ccs,cr0014114";
>>        reg = <1>;
>>        spi-max-frequency = <50000>;
>>        spi-cpha;
>>        label = "board1";
>> };
> 
> Ok, makes sense. How does the board look?

Did you mean a photo? Sure http://i.imgur.com/OShLIJx.jpg
 
>>>>>>>> +	while (i--)
>>>>>>>> +		led_classdev_unregister(&priv->leds[i].ldev);
>>>>>>>
>>>>>>> Can devm_* be used to simplify this?
>>>>>>
>>>>>> I think no, because it will cause race condition.
>>>>>
>>>>> Please take a look at devm_led_classdev_register() and friends. It
>>>>> should be possible to simplify code without races.
>>>>
>>>> I don't understand how I can call destroy_workqueue() after calling unregister leds.
>>>
>>> Do you actually need the workqueues? It should be possible to avoid
>>> them, using workqueue support in the core.
>>
>> The delay between data sends to SPI board should be at least 10 ms
>> I think it will be bad idea to use shared workqueue from kernel,
>> so I create separate single threaded work queue :)
> 
> If workqueue support in led core can not take 10 msec wait, we should
> fix it, not add hacks around...
> 									
> 

Could you please explain or give a example?
Because it's not clear for me, how I can use it.

-- 
Best regards,
Oleh Kravchenko


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] leds: add LED driver for CR0014114 board

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

Hi!

> > Ok, makes sense. How does the board look?
> 
> Did you mean a photo? Sure http://i.imgur.com/OShLIJx.jpg

Thanks.

> >> The delay between data sends to SPI board should be at least 10 ms
> >> I think it will be bad idea to use shared workqueue from kernel,
> >> so I create separate single threaded work queue :)
> > 
> > If workqueue support in led core can not take 10 msec wait, we should
> > fix it, not add hacks around...
> 
> Could you please explain or give a example?
> Because it's not clear for me, how I can use it.

You use cdev.brightness_set_blocking instead of brightness_set...

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]