[v5,1/2] dt-bindings: Add docs for EL15203000
diff mbox series

Message ID 20190830224619.15728-1-oleg@kaa.org.ua
State Not Applicable
Headers show
Series
  • [v5,1/2] dt-bindings: Add docs for EL15203000
Related show

Checks

Context Check Description
robh/checkpatch success

Commit Message

Oleh Kravchenko Aug. 30, 2019, 10:46 p.m. UTC
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

Comments

Rob Herring Sept. 1, 2019, 7:18 p.m. UTC | #1
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.
Oleh Kravchenko Sept. 1, 2019, 7:26 p.m. UTC | #2
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.
Jacek Anaszewski Sept. 4, 2019, 9:23 p.m. UTC | #3
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", &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");
>
Jacek Anaszewski Sept. 4, 2019, 9:23 p.m. UTC | #4
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.
Oleh Kravchenko Sept. 5, 2019, 6:17 a.m. UTC | #5
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", &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
Jacek Anaszewski Sept. 5, 2019, 7:18 p.m. UTC | #6
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", &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
>

Patch
diff mbox series

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