diff mbox series

[1/2] dt-bindings: Add vendor prefix and docs for EL15203000

Message ID 20190607114823.3735-1-oleg@kaa.org.ua
State Superseded, archived
Headers show
Series [1/2] dt-bindings: Add vendor prefix and docs for EL15203000 | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Oleh Kravchenko June 7, 2019, 11:48 a.m. UTC 
Add documentation and example for dt-bindings EL15203000.

Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
 .../bindings/leds/leds-el15203000.txt         | 48 +++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-el15203000.txt

Comments

Dan Murphy June 7, 2019, 2:41 p.m. UTC | #1 
Oleh

On 6/7/19 6:48 AM, Oleh Kravchenko wrote:
> Add documentation and example for dt-bindings EL15203000.
>
> Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
> ---
>   .../bindings/leds/leds-el15203000.txt         | 48 +++++++++++++++++++
>   1 file changed, 48 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..f47ee91cb419
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
> @@ -0,0 +1,48 @@
> +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"
> +
> +Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
> +apply. In particular, "reg" and "spi-max-frequency" properties must be given.
> +

Can you please add this instead?

reg : see Documentation/devicetree/bindings/spi/spi-bus.txt

spi-max-frequency : see Documentation/devicetree/bindings/spi/spi-bus.txt


> +LED sub-node properties:
> +- label :
> +	see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger : (optional)
> +	see Documentation/devicetree/bindings/leds/common.txt
> +

max-brightness?

That appears to be optional

N


> +Example
> +-------
> +
> +led-controller@0 {
> +	compatible = "crane,el15203000";
> +	reg = <0>;
> +	spi-max-frequency = <50000>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	/* water pipe */
> +	pipe@50 {
> +		reg = <0x50>;
> +		label = "red:pipe";
> +		max-brightness = <2>;
> +	};
> +
> +	/* screen frame */
> +	screen@53 {
> +		reg = <0x53>;
> +		label = "red:screen";
> +		max-brightness = <2>;
> +	};
> +
> +	/* vending area */
> +	vend@56 {
> +		reg = <0x56>;
> +		label = "red:vend";
> +	};
> +};
Dan Murphy June 7, 2019, 2:59 p.m. UTC | #2 
Oleh

On 6/7/19 6:48 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>
> ---
>   drivers/leds/Kconfig           |  13 ++
>   drivers/leds/Makefile          |   1 +
>   drivers/leds/leds-el15203000.c | 236 +++++++++++++++++++++++++++++++++
>   3 files changed, 250 insertions(+)
>   create mode 100644 drivers/leds/leds-el15203000.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 71be87bdb926..ae293a0f7598 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -128,6 +128,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 which
> +	  is widely used in coffee vending machines produced by
> +	  Crane Merchandising Systems.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called leds-el15203000.
> +
>   config LEDS_LM3530
>   	tristate "LCD Backlight driver for LM3530"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 1e9702ebffee..1f193ffc2feb 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -85,6 +85,7 @@ obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
>   # LED SPI Drivers
>   obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
>   obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> +obj-$(CONFIG_LEDS_EL15203000)		+= leds-el15203000.o
>   
>   # LED Userspace Drivers
>   obj-$(CONFIG_LEDS_USER)			+= uleds.o
> diff --git a/drivers/leds/leds-el15203000.c b/drivers/leds/leds-el15203000.c
> new file mode 100644
> index 000000000000..0f211871e63c
> --- /dev/null
> +++ b/drivers/leds/leds-el15203000.c
> @@ -0,0 +1,236 @@
> +// 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/limits.h>
I do not see any #defines used from this file
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <linux/workqueue.h>

It does not look like you are using any work queues.


> +#include <uapi/linux/uleds.h>
> +
> +/*
> + * EL15203000 SPI protocol descrtiption:
> + * +-----+------------+
> + * | LED | BRIGHTNESS |
> + * +-----+------------+
> + * |  1  |      1     |
> + * +-----+------------+
> + *
> + * LED		ID, known values is 0x50 (Pipe), 0x53 (Screen Frame) and
> + * 		0x56 (Vending Area);
> + * BRIGHTNESS	Can be 0x30 (OFF), 0x31 (ON).
> + * 		0x32 (Effect) can be used for 0x50 (leaking) and
> + * 		for 0x53 (blinking).
> + *

These can be #defined which makes them desctiptive

#define EL15203000_LED_OFF     0x30

#define EL15203000_LED_ON     0x31

and so on


> + * LEDs MCU board expects 20 msec delay per byte.
> + */
> +
> +/* EL15203000 default settings */
> +#define EL_MAX_BRIGHTNESS	2
> +#define EL_FW_DELAY_USEC	20000
> +
> +struct el15203000_led {
> +	char			name[LED_MAX_NAME_SIZE];
> +	struct el15203000	*priv;
> +	struct led_classdev	ldev;
> +	u8			reg;
> +};
> +
> +struct el15203000 {
> +	size_t			count;
> +	struct device		*dev;
> +	struct mutex		lock;
> +	struct spi_device	*spi;
> +	unsigned long		delay;
> +	struct el15203000_led	leds[];
> +};
> +
> +static int el15203000_set_sync(struct led_classdev *ldev,
> +			      enum led_brightness brightness)
> +{
> +	int			ret;
> +	u8			cmd[2];
> +	size_t			i;
> +	unsigned long		udelay, now;
> +	struct el15203000_led	*led = container_of(ldev,
> +						    struct el15203000_led,
> +						    ldev);
> +
> +	mutex_lock(&led->priv->lock);
> +
> +	dev_dbg(led->priv->dev, "Set brightness of %s to %d",
> +		led->name, brightness);
> +
> +	/* to avoid SPI mistiming with firmware we should wait some time */
> +	now = jiffies;
> +	if (time_after(led->priv->delay, now)) {
> +		udelay = jiffies_to_usecs(led->priv->delay - now);
> +
> +		dev_dbg(led->priv->dev, "Wait %luus to synch", udelay);
> +		usleep_range(udelay, udelay + 1);
> +	}
> +
> +	cmd[0] = led->reg;
> +	cmd[1] = 0x30 + (u8)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\n", ret);
> +			break;
> +		}
> +	}
> +
> +	led->priv->delay = jiffies + usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> +	mutex_unlock(&led->priv->lock);
> +
> +	return ret;
> +}
> +
> +static int el15203000_probe_dt(struct el15203000 *priv)
> +{
> +	size_t			i = 0;
> +	struct el15203000_led	*led;
> +	struct fwnode_handle	*child;
> +	struct device_node	*np;
> +	int			ret;
> +	const char		*str;
> +	u32			reg;
> +
> +	device_for_each_child_node(priv->dev, child) {
> +		np = to_of_node(child);
> +		led = &priv->leds[i];
> +
> +		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 -ENODEV;
-EINVAL
> +		}
> +		led->reg = reg;
> +
> +		ret = fwnode_property_read_string(child, "label", &str);
> +		if (ret)
> +			snprintf(led->name, sizeof(led->name),
> +				 "el15203000::");
> +		else
> +			snprintf(led->name, sizeof(led->name),
> +				 "el15203000:%s", str);
> +
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +					    &led->ldev.default_trigger);
> +
> +		led->priv			  = priv;
> +		led->ldev.name			  = led->name;
> +		led->ldev.max_brightness	  = LED_ON;
Do not need this as it should be stored when doing the fwnode read.
> +		led->ldev.brightness_set_blocking = el15203000_set_sync;
> +
> +		ret = fwnode_property_read_u32(child, "max-brightness",
> +					       &led->ldev.max_brightness);
> +		if (led->ldev.max_brightness > EL_MAX_BRIGHTNESS) {
> +			dev_err(priv->dev, "invalid max brightness %d",
> +				led->ldev.max_brightness);
> +			fwnode_handle_put(child);
> +
> +			return -ENODEV;

-EINVAL


> +		}
> +
> +		ret = devm_of_led_classdev_register(priv->dev, np,
> +						    &led->ldev);

please use devm_led_classdev_register then there is no need to set np or 
store it.

Dan

> +		if (ret) {
> +			dev_err(priv->dev,
> +				"failed to register LED device %s, err %d",
> +				led->name, ret);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		led->ldev.dev->of_node = np;
> +
> +		i++;
> +	}
> +
> +	return ret;
> +}
> +
> +static int el15203000_probe(struct spi_device *spi)
> +{
> +	struct el15203000	*priv;
> +	size_t			count;
> +	int			ret;
> +
> +	count = device_get_child_node_count(&spi->dev);
> +	if (!count) {
> +		dev_err(&spi->dev, "LEDs are not defined in device tree!");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(&spi->dev, struct_size(priv, leds, count),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->lock);
> +	priv->count	= count;
> +	priv->dev	= &spi->dev;
> +	priv->spi	= spi;
> +	priv->delay	= jiffies -
> +			  usecs_to_jiffies(EL_FW_DELAY_USEC);
> +
> +	ret = el15203000_probe_dt(priv);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, priv);
> +	dev_dbg(priv->dev, "%zd LEDs registered", priv->count);
> +
> +	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");
Oleh Kravchenko June 7, 2019, 5:46 p.m. UTC | #3 
Hello Dan,
thank you for your review!

07.06.19 17:59, Dan Murphy пише:
> Oleh
> 
> On 6/7/19 6:48 AM, Oleh Kravchenko wrote:

>> +#include <linux/delay.h>
>> +#include <linux/leds.h>
>> +#include <linux/limits.h>
> I do not see any #defines used from this file

For U8_MAX, please see below.

>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/workqueue.h>
> 
> It does not look like you are using any work queues.
 
My bad, thank you!

>> + * LED        ID, known values is 0x50 (Pipe), 0x53 (Screen Frame) and
>> + *         0x56 (Vending Area);
>> + * BRIGHTNESS    Can be 0x30 (OFF), 0x31 (ON).
>> + *         0x32 (Effect) can be used for 0x50 (leaking) and
>> + *         for 0x53 (blinking).
>> + *
> 
> These can be #defined which makes them desctiptive
> 
> #define EL15203000_LED_OFF     0x30
> 
> #define EL15203000_LED_ON     0x31
> 
> and so on

Hm, but just adding 0x30 not will be more clear and faster?
I think switch .. case or if .. else, will be more hard to read :)

>> +        if (reg > U8_MAX) {
>> +            dev_err(priv->dev, "LED value %d is invalid", reg);
>> +            fwnode_handle_put(child);
>> +
>> +            return -ENODEV;
> -EINVAL

Done

>> +        }
>> +        led->reg = reg;
>> +
>> +        ret = fwnode_property_read_string(child, "label", &str);
>> +        if (ret)
>> +            snprintf(led->name, sizeof(led->name),
>> +                 "el15203000::");
>> +        else
>> +            snprintf(led->name, sizeof(led->name),
>> +                 "el15203000:%s", str);
>> +
>> +        fwnode_property_read_string(child, "linux,default-trigger",
>> +                        &led->ldev.default_trigger);
>> +
>> +        led->priv              = priv;
>> +        led->ldev.name              = led->name;
>> +        led->ldev.max_brightness      = LED_ON;
> Do not need this as it should be stored when doing the fwnode read.

This is default value 1, by dtb we can override it to 2.

>> +        led->ldev.brightness_set_blocking = el15203000_set_sync;
>> +
>> +        ret = fwnode_property_read_u32(child, "max-brightness",
>> +                           &led->ldev.max_brightness);
>> +        if (led->ldev.max_brightness > EL_MAX_BRIGHTNESS) {
>> +            dev_err(priv->dev, "invalid max brightness %d",
>> +                led->ldev.max_brightness);
>> +            fwnode_handle_put(child);
>> +
>> +            return -ENODEV;
> 
> -EINVAL

Done
 
>> +        ret = devm_of_led_classdev_register(priv->dev, np,
>> +                            &led->ldev);
> 
> please use devm_led_classdev_register then there is no need to set np or store it.
> 
> Dan
>
Dan Murphy June 7, 2019, 7:41 p.m. UTC | #4 
Oleh

On 6/7/19 12:46 PM, Oleh Kravchenko wrote:
> Hello Dan,
> thank you for your review!
>
> 07.06.19 17:59, Dan Murphy пише:
>> Oleh
>>
>> On 6/7/19 6:48 AM, Oleh Kravchenko wrote:
>>> +#include <linux/delay.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/limits.h>
>> I do not see any #defines used from this file
> For U8_MAX, please see below.
>
>>> +#include <linux/module.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/workqueue.h>
>> It does not look like you are using any work queues.
>   
> My bad, thank you!
>
>>> + * LED        ID, known values is 0x50 (Pipe), 0x53 (Screen Frame) and
>>> + *         0x56 (Vending Area);
>>> + * BRIGHTNESS    Can be 0x30 (OFF), 0x31 (ON).
>>> + *         0x32 (Effect) can be used for 0x50 (leaking) and
>>> + *         for 0x53 (blinking).
>>> + *
>> These can be #defined which makes them desctiptive
>>
>> #define EL15203000_LED_OFF     0x30
>>
>> #define EL15203000_LED_ON     0x31
>>
>> and so on
> Hm, but just adding 0x30 not will be more clear and faster?
> I think switch .. case or if .. else, will be more hard to read :)

Huh?

You had to explain what the value 0x3X meant in a comment.  So clarity 
is not there.  Faster?

The #define LED_?? makes sense without having to explain the protocol.  
What is 0x30?

And I am not seeing any switch..case or if..else in the code using these 
values but if it was defined it would be easier to read.

Why would this value be in a switch..case or if..else if it is just a 
protocol value?

You have 1 line of code that uses the 0x30 so maybe you don't need to 
define LED_ON and LED_OFF but this value means something

and that should be #defined as there is no understanding what the 0x30 
is actually doing.

cmd[1] =  EL15203000_LED_???? + (u8)brightness;



>>> +        if (reg > U8_MAX) {
>>> +            dev_err(priv->dev, "LED value %d is invalid", reg);
>>> +            fwnode_handle_put(child);
>>> +
>>> +            return -ENODEV;
>> -EINVAL
> Done
>
>>> +        }
>>> +        led->reg = reg;
>>> +
>>> +        ret = fwnode_property_read_string(child, "label", &str);
>>> +        if (ret)
>>> +            snprintf(led->name, sizeof(led->name),
>>> +                 "el15203000::");
>>> +        else
>>> +            snprintf(led->name, sizeof(led->name),
>>> +                 "el15203000:%s", str);
>>> +
>>> +        fwnode_property_read_string(child, "linux,default-trigger",
>>> +                        &led->ldev.default_trigger);
>>> +
>>> +        led->priv              = priv;
>>> +        led->ldev.name              = led->name;
>>> +        led->ldev.max_brightness      = LED_ON;
>> Do not need this as it should be stored when doing the fwnode read.
> This is default value 1, by dtb we can override it to 2.

This has code some other issues.  I will comment in your v2.

Dan

>>> +        led->ldev.brightness_set_blocking = el15203000_set_sync;
>>> +
>>> +        ret = fwnode_property_read_u32(child, "max-brightness",
>>> +                           &led->ldev.max_brightness);
>>> +        if (led->ldev.max_brightness > EL_MAX_BRIGHTNESS) {
>>> +            dev_err(priv->dev, "invalid max brightness %d",
>>> +                led->ldev.max_brightness);
>>> +            fwnode_handle_put(child);
>>> +
>>> +            return -ENODEV;
>> -EINVAL
> Done
>   
>>> +        ret = devm_of_led_classdev_register(priv->dev, np,
>>> +                            &led->ldev);
>> please use devm_led_classdev_register then there is no need to set np or store it.
>>
>> Dan
>>
Oleh Kravchenko June 7, 2019, 8:05 p.m. UTC | #5 
Dan,

07.06.19 22:41, Dan Murphy пише:
> Oleh
> 
> On 6/7/19 12:46 PM, Oleh Kravchenko wrote:
>>> These can be #defined which makes them desctiptive
>>>
>>> #define EL15203000_LED_OFF     0x30
>>>
>>> #define EL15203000_LED_ON     0x31
>>>
>>> and so on
>> Hm, but just adding 0x30 not will be more clear and faster?
>> I think switch .. case or if .. else, will be more hard to read :)
> 
> Huh?
> 
> You had to explain what the value 0x3X meant in a comment.  So clarity is not there.  Faster?
> 
> The #define LED_?? makes sense without having to explain the protocol.  What is 0x30?
> 
> And I am not seeing any switch..case or if..else in the code using these values but if it was defined it would be easier to read.
> 
> Why would this value be in a switch..case or if..else if it is just a protocol value?
> 
> You have 1 line of code that uses the 0x30 so maybe you don't need to define LED_ON and LED_OFF but this value means something
> 
> and that should be #defined as there is no understanding what the 0x30 is actually doing.
> 
> cmd[1] =  EL15203000_LED_???? + (u8)brightness;

I described it in top of the file. Looks likes it's not clear.
LED board has next brightness level:
	'0' - 0x30
	'1' - 0x31
	'2' - 0x32


>>>> +        if (reg > U8_MAX) {
>>>> +            dev_err(priv->dev, "LED value %d is invalid", reg);
>>>> +            fwnode_handle_put(child);
>>>> +
>>>> +            return -ENODEV;
>>> -EINVAL
>> Done
>>
>>>> +        }
>>>> +        led->reg = reg;
>>>> +
>>>> +        ret = fwnode_property_read_string(child, "label", &str);
>>>> +        if (ret)
>>>> +            snprintf(led->name, sizeof(led->name),
>>>> +                 "el15203000::");
>>>> +        else
>>>> +            snprintf(led->name, sizeof(led->name),
>>>> +                 "el15203000:%s", str);
>>>> +
>>>> +        fwnode_property_read_string(child, "linux,default-trigger",
>>>> +                        &led->ldev.default_trigger);
>>>> +
>>>> +        led->priv              = priv;
>>>> +        led->ldev.name              = led->name;
>>>> +        led->ldev.max_brightness      = LED_ON;
>>> Do not need this as it should be stored when doing the fwnode read.
>> This is default value 1, by dtb we can override it to 2.
> 
> This has code some other issues.  I will comment in your v2.

Thanks

> Dan
>
Dan Murphy June 7, 2019, 8:09 p.m. UTC | #6 
Oleh

On 6/7/19 3:05 PM, Oleh Kravchenko wrote:
> Dan,
>
> 07.06.19 22:41, Dan Murphy пише:
>> Oleh
>>
>> On 6/7/19 12:46 PM, Oleh Kravchenko wrote:
>>>> These can be #defined which makes them desctiptive
>>>>
>>>> #define EL15203000_LED_OFF     0x30
>>>>
>>>> #define EL15203000_LED_ON     0x31
>>>>
>>>> and so on
>>> Hm, but just adding 0x30 not will be more clear and faster?
>>> I think switch .. case or if .. else, will be more hard to read :)
>> Huh?
>>
>> You had to explain what the value 0x3X meant in a comment.  So clarity is not there.  Faster?
>>
>> The #define LED_?? makes sense without having to explain the protocol.  What is 0x30?
>>
>> And I am not seeing any switch..case or if..else in the code using these values but if it was defined it would be easier to read.
>>
>> Why would this value be in a switch..case or if..else if it is just a protocol value?
>>
>> You have 1 line of code that uses the 0x30 so maybe you don't need to define LED_ON and LED_OFF but this value means something
>>
>> and that should be #defined as there is no understanding what the 0x30 is actually doing.
>>
>> cmd[1] =  EL15203000_LED_???? + (u8)brightness;
> I described it in top of the file. Looks likes it's not clear.
> LED board has next brightness level:
> 	'0' - 0x30
> 	'1' - 0x31
> 	'2' - 0x32
>
>
I understand it but all I am saying is that 0x30 should have a #define 
we try not to use magic numbers.

And per the DT 0x32 is not a brightness but a feature so technically 
your max_brightness is 1.  And you should expose a different

file for the effects.  Because 0x32 is not a brightness level as it does 
not affect brightness but I guess it does something special.

Dan


>>>>> +        if (reg > U8_MAX) {
>>>>> +            dev_err(priv->dev, "LED value %d is invalid", reg);
>>>>> +            fwnode_handle_put(child);
>>>>> +
>>>>> +            return -ENODEV;
>>>> -EINVAL
>>> Done
>>>
>>>>> +        }
>>>>> +        led->reg = reg;
>>>>> +
>>>>> +        ret = fwnode_property_read_string(child, "label", &str);
>>>>> +        if (ret)
>>>>> +            snprintf(led->name, sizeof(led->name),
>>>>> +                 "el15203000::");
>>>>> +        else
>>>>> +            snprintf(led->name, sizeof(led->name),
>>>>> +                 "el15203000:%s", str);
>>>>> +
>>>>> +        fwnode_property_read_string(child, "linux,default-trigger",
>>>>> +                        &led->ldev.default_trigger);
>>>>> +
>>>>> +        led->priv              = priv;
>>>>> +        led->ldev.name              = led->name;
>>>>> +        led->ldev.max_brightness      = LED_ON;
>>>> Do not need this as it should be stored when doing the fwnode read.
>>> This is default value 1, by dtb we can override it to 2.
>> This has code some other issues.  I will comment in your v2.
> Thanks
>
>> Dan
>>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-el15203000.txt b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
new file mode 100644
index 000000000000..f47ee91cb419
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-el15203000.txt
@@ -0,0 +1,48 @@ 
+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"
+
+Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
+apply. In particular, "reg" and "spi-max-frequency" properties must be given.
+
+LED sub-node properties:
+- label :
+	see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt
+
+Example
+-------
+
+led-controller@0 {
+	compatible = "crane,el15203000";
+	reg = <0>;
+	spi-max-frequency = <50000>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* water pipe */
+	pipe@50 {
+		reg = <0x50>;
+		label = "red:pipe";
+		max-brightness = <2>;
+	};
+
+	/* screen frame */
+	screen@53 {
+		reg = <0x53>;
+		label = "red:screen";
+		max-brightness = <2>;
+	};
+
+	/* vending area */
+	vend@56 {
+		reg = <0x56>;
+		label = "red:vend";
+	};
+};