[PATCH v2 1/2] dt-bindings: leds: Add binding for spi-byte LED.

[PATCH v2 1/2] dt-bindings: leds: Add binding for spi-byte LED.

[oss]

From: Christian Mauderer <oss@c-mauderer.de>

This patch adds the binding documentation for a simple SPI based LED
controller which use only one byte for setting the brightness.

Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
---

Changes compared to v1:
- rename ubnt-spi to leds-spi-byte
- rename "ubnt-spi,off_bright" and "ubnt-spi,max_bright" to
  "leds-spi-byte,off-value" and "leds-spi-byte,max-value" and mark them required
- rename led-controller node to "led-controller"
- extend description
- remove SPI controller
- use "white:status" for the example label


 .../bindings/leds/leds-spi-byte.txt           | 47 +++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-spi-byte.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-spi-byte.txt b/Documentation/devicetree/bindings/leds/leds-spi-byte.txt
new file mode 100644
index 000000000000..1dd6ab03a56d
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-spi-byte.txt
@@ -0,0 +1,47 @@
+* Single Byte SPI LED Device Driver.
+
+The driver can be used for controllers with a very simple SPI protocol: Only one
+byte will be sent. The value of the byte can be any value between the off-value
+and max-value defined in the properties.
+
+One example where the driver can be used is the controller in Ubiquiti airCube
+ISP devices. That LED controller is based on a 8 bit microcontroller (SONiX
+8F26E611LA) that has been programmed to control the single LED of the device.
+The controller supports four modes depending on the highest two bits in a byte:
+One setting for brightness, the other three provide different blink patterns.
+With the leds-spi-byte driver a basic support for the brightness mode of that
+controller can be easily achieved by setting the minimum and maximum to the
+brightness modes minimum and maximum byte value.
+
+Required properties:
+- compatible:		Should be "leds-spi-byte".
+
+Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
+apply. In particular, "reg" and "spi-max-frequency" properties must be given.
+
+The driver currently only supports one LED. The properties of the LED are
+configured in a sub-node in the device node.
+
+LED sub-node properties:
+- label:
+	see Documentation/devicetree/bindings/leds/common.txt
+- leds-spi-byte,off-value:
+	The SPI byte value that should be sent to switch the LED off. Has to be
+	smaller than max-value. Range: 0 to 254.
+- leds-spi-byte,max-value:
+	The SPI byte value that should be sent to set the LED to the maximum
+	brightness. Has to be bigger than off-value. Range: 1 to 255.
+
+Example:
+
+led-controller@0 {
+	compatible = "leds-spi-byte";
+	reg = <0>;
+	spi-max-frequency = <100000>;
+
+	led {
+		label = "white:status";
+		leds-spi-byte,off-value = /bits/ 8 <0>;
+		leds-spi-byte,max-value = /bits/ 8 <63>;
+	};
+};
-- 
2.21.0

[PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[oss]

From: Christian Mauderer <oss@c-mauderer.de>

This driver adds support for simple SPI based LED controller which use
only one byte for setting the brightness.

Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
---

Changes compared to v1:
- rename ubnt-spi to leds-spi-byte
- rework probe to get all parameters before allocating anything -> error checks
  all collected together and initializing all fields of the device structure is
  more obvious
- fix some unsteady indentations during variable declaration
- rework comment with protocol explanation
- handle case of off_bright > max_bright
- fix spelling in commit message
- mutex_destroy in remove
- change label to use either use the given one without a prefix or a default one
  NOTE: Should the label be a mandatory parameter instead of the default label?


 drivers/leds/Kconfig         |  12 ++++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-spi-byte.c | 133 +++++++++++++++++++++++++++++++++++
 3 files changed, 146 insertions(+)
 create mode 100644 drivers/leds/leds-spi-byte.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..0866c55e8004 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -766,6 +766,18 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_SPI_BYTE
+	tristate "LED support for SPI LED controller with a single byte"
+	depends on LEDS_CLASS
+	depends on SPI
+	depends on OF
+	help
+	  This option enables support for LED controller which use a single byte
+	  for controlling the brightness. The minimum and maximum value of the
+	  byte can be configured via a device tree. The driver can be used for
+	  example for the microcontroller based LED controller in the Ubiquiti
+	  airCube ISP devices.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..1786d7e2c236 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -75,6 +75,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
 obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
+obj-$(CONFIG_LEDS_SPI_BYTE)		+= leds-spi-byte.o
 obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
new file mode 100644
index 000000000000..19a68bce1a7c
--- /dev/null
+++ b/drivers/leds/leds-spi-byte.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Christian Mauderer <oss@c-mauderer.de>
+
+/*
+ * The driver can be used for controllers with a very simple SPI protocol: Only
+ * one byte between an off and a max value (defined by devicetree) will be sent.
+ */
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+#include <linux/mutex.h>
+#include <uapi/linux/uleds.h>
+
+struct spi_byte_led {
+	struct led_classdev	ldev;
+	struct spi_device	*spi;
+	char			name[LED_MAX_NAME_SIZE];
+	struct mutex		mutex;
+	u8			off_value;
+	u8			max_value;
+};
+
+static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
+					    enum led_brightness brightness)
+{
+	struct spi_byte_led *led = container_of(dev, struct spi_byte_led, ldev);
+	u8 value;
+	int ret;
+
+	value = (u8) brightness + led->off_value;
+
+	mutex_lock(&led->mutex);
+	ret = spi_write(led->spi, &value, sizeof(value));
+	mutex_unlock(&led->mutex);
+
+	return ret;
+}
+
+static int spi_byte_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct device_node *child;
+	struct spi_byte_led *led;
+	int ret;
+	const char *default_name = "leds-spi-byte::";
+	const char *name;
+	u8 off_value;
+	u8 max_value;
+
+	if (!dev->of_node)
+		return -ENODEV;
+
+	if (of_get_child_count(dev->of_node) != 1) {
+		dev_err(dev, "Device must have exactly one LED sub-node.");
+		return -EINVAL;
+	}
+	child = of_get_next_child(dev->of_node, NULL);
+
+	ret = of_property_read_string(child, "label", &name);
+	if (ret != 0)
+		name = default_name;
+
+	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
+	if (ret != 0) {
+		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
+		return -EINVAL;
+	}
+
+	ret = of_property_read_u8(child, "leds-spi-byte,max-value", &max_value);
+	if (ret != 0) {
+		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
+		return -EINVAL;
+	}
+
+	if (off_value >= max_value) {
+		dev_err(dev, "off-value has to be smaller than max-value.");
+		return -EINVAL;
+	}
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->spi = spi;
+	strlcpy(led->name, name, sizeof(led->name));
+	mutex_init(&led->mutex);
+	led->off_value = off_value;
+	led->max_value = max_value;
+	led->ldev.name = led->name;
+	led->ldev.brightness = LED_OFF;
+	led->ldev.max_brightness = led->max_value - led->off_value;
+	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
+	ret = led_classdev_register(&spi->dev, &led->ldev);
+	if (ret >= 0)
+		spi_set_drvdata(spi, led);
+
+	return ret;
+}
+
+static int spi_byte_remove(struct spi_device *spi)
+{
+	struct spi_byte_led	*led = spi_get_drvdata(spi);
+
+	led_classdev_unregister(&led->ldev);
+	mutex_destroy(&led->mutex);
+
+	return 0;
+}
+
+static const struct of_device_id spi_byte_dt_ids[] = {
+	{ .compatible = "leds-spi-byte", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
+
+static struct spi_driver spi_byte_driver = {
+	.probe		= spi_byte_probe,
+	.remove		= spi_byte_remove,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= spi_byte_dt_ids,
+	},
+};
+
+module_spi_driver(spi_byte_driver);
+
+MODULE_AUTHOR("Christian Mauderer <oss@c-mauderer.de>");
+MODULE_DESCRIPTION("single byte SPI LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:leds-spi-byte");
-- 
2.21.0

Re: [PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Jacek Anaszewski]

Hi Christian,

Thank you for the update. I have some trivial nits,
please refer below.

On 5/5/19 2:52 PM, oss@c-mauderer.de wrote:
> From: Christian Mauderer <oss@c-mauderer.de>
> 
> This driver adds support for simple SPI based LED controller which use
> only one byte for setting the brightness.
> 
> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
> ---
> 
> Changes compared to v1:
> - rename ubnt-spi to leds-spi-byte
> - rework probe to get all parameters before allocating anything -> error checks
>    all collected together and initializing all fields of the device structure is
>    more obvious
> - fix some unsteady indentations during variable declaration
> - rework comment with protocol explanation
> - handle case of off_bright > max_bright
> - fix spelling in commit message
> - mutex_destroy in remove
> - change label to use either use the given one without a prefix or a default one
>    NOTE: Should the label be a mandatory parameter instead of the default label?
> 
> 
>   drivers/leds/Kconfig         |  12 ++++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-spi-byte.c | 133 +++++++++++++++++++++++++++++++++++
>   3 files changed, 146 insertions(+)
>   create mode 100644 drivers/leds/leds-spi-byte.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a72f97fca57b..0866c55e8004 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -766,6 +766,18 @@ config LEDS_NIC78BX
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called leds-nic78bx.
>   
> +config LEDS_SPI_BYTE
> +	tristate "LED support for SPI LED controller with a single byte"
> +	depends on LEDS_CLASS
> +	depends on SPI
> +	depends on OF
> +	help
> +	  This option enables support for LED controller which use a single byte
> +	  for controlling the brightness. The minimum and maximum value of the
> +	  byte can be configured via a device tree. The driver can be used for
> +	  example for the microcontroller based LED controller in the Ubiquiti
> +	  airCube ISP devices.
> +
>   comment "LED Triggers"
>   source "drivers/leds/trigger/Kconfig"
>   
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 4c1b0054f379..1786d7e2c236 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>   obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
>   obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
>   obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
> +obj-$(CONFIG_LEDS_SPI_BYTE)		+= leds-spi-byte.o
>   obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
>   obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
>   obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
> diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
> new file mode 100644
> index 000000000000..19a68bce1a7c
> --- /dev/null
> +++ b/drivers/leds/leds-spi-byte.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Christian Mauderer <oss@c-mauderer.de>
> +
> +/*
> + * The driver can be used for controllers with a very simple SPI protocol: Only
> + * one byte between an off and a max value (defined by devicetree) will be sent.
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/spi/spi.h>
> +#include <linux/mutex.h>
> +#include <uapi/linux/uleds.h>
> +
> +struct spi_byte_led {
> +	struct led_classdev	ldev;
> +	struct spi_device	*spi;
> +	char			name[LED_MAX_NAME_SIZE];
> +	struct mutex		mutex;
> +	u8			off_value;
> +	u8			max_value;
> +};
> +
> +static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
> +					    enum led_brightness brightness)
> +{
> +	struct spi_byte_led *led = container_of(dev, struct spi_byte_led, ldev);
> +	u8 value;
> +	int ret;
> +
> +	value = (u8) brightness + led->off_value;
> +
> +	mutex_lock(&led->mutex);
> +	ret = spi_write(led->spi, &value, sizeof(value));
> +	mutex_unlock(&led->mutex);
> +
> +	return ret;
> +}
> +
> +static int spi_byte_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct device_node *child;
> +	struct spi_byte_led *led;
> +	int ret;
> +	const char *default_name = "leds-spi-byte::";
> +	const char *name;
> +	u8 off_value;
> +	u8 max_value;
> +
> +	if (!dev->of_node)
> +		return -ENODEV;
> +
> +	if (of_get_child_count(dev->of_node) != 1) {
> +		dev_err(dev, "Device must have exactly one LED sub-node.");
> +		return -EINVAL;
> +	}
> +	child = of_get_next_child(dev->of_node, NULL);
> +
> +	ret = of_property_read_string(child, "label", &name);
> +	if (ret != 0)

It is more customary to have "if (ret)" in such cases.
Applies to all occurrences below.

> +		name = default_name;
> +
> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
> +	if (ret != 0) {
> +		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u8(child, "leds-spi-byte,max-value", &max_value);
> +	if (ret != 0) {
> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
> +		return -EINVAL;
> +	}
> +
> +	if (off_value >= max_value) {
> +		dev_err(dev, "off-value has to be smaller than max-value.");
> +		return -EINVAL;
> +	}
> +
> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	led->spi = spi;
> +	strlcpy(led->name, name, sizeof(led->name));
> +	mutex_init(&led->mutex);
> +	led->off_value = off_value;
> +	led->max_value = max_value;
> +	led->ldev.name = led->name;
> +	led->ldev.brightness = LED_OFF;

This line is redundant - already zeroed by kzalloc.

> +	led->ldev.max_brightness = led->max_value - led->off_value;
> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
> +	ret = led_classdev_register(&spi->dev, &led->ldev);

Please use devm_led_classdev_register().

> +	if (ret >= 0)
> +		spi_set_drvdata(spi, led);

This looks quite odd. Please shape it as below:

	if (ret)
		return ret

	spi_set_drvdata(spi, led);

> +
> +	return ret;

	s/ret/0/

> +}
> +
> +static int spi_byte_remove(struct spi_device *spi)
> +{
> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
> +
> +	led_classdev_unregister(&led->ldev);
> +	mutex_destroy(&led->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id spi_byte_dt_ids[] = {
> +	{ .compatible = "leds-spi-byte", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
> +
> +static struct spi_driver spi_byte_driver = {
> +	.probe		= spi_byte_probe,
> +	.remove		= spi_byte_remove,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= spi_byte_dt_ids,
> +	},
> +};
> +
> +module_spi_driver(spi_byte_driver);
> +
> +MODULE_AUTHOR("Christian Mauderer <oss@c-mauderer.de>");
> +MODULE_DESCRIPTION("single byte SPI LED driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:leds-spi-byte");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Christian Mauderer]

On 05/05/2019 16:48, Jacek Anaszewski wrote:
> Hi Christian,
> 
> Thank you for the update. I have some trivial nits,
> please refer below.

Hello Jacek,

thanks for having a look. I don't have problems with the changes and
will integrate them into a v3. Sorry for the style nitpicks for example
with the (ret != 0). It's different for each project and I haven't
written much code with Linux style yet.

I'll wait for another few hours in case someone else wants additional
changes before posting v3.

Best regards

Christian

> 
> On 5/5/19 2:52 PM, oss@c-mauderer.de wrote:
>> From: Christian Mauderer <oss@c-mauderer.de>
>>
>> This driver adds support for simple SPI based LED controller which use
>> only one byte for setting the brightness.
>>
>> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
>> ---
>>
>> Changes compared to v1:
>> - rename ubnt-spi to leds-spi-byte
>> - rework probe to get all parameters before allocating anything ->
>> error checks
>>    all collected together and initializing all fields of the device
>> structure is
>>    more obvious
>> - fix some unsteady indentations during variable declaration
>> - rework comment with protocol explanation
>> - handle case of off_bright > max_bright
>> - fix spelling in commit message
>> - mutex_destroy in remove
>> - change label to use either use the given one without a prefix or a
>> default one
>>    NOTE: Should the label be a mandatory parameter instead of the
>> default label?
>>
>>
>>   drivers/leds/Kconfig         |  12 ++++
>>   drivers/leds/Makefile        |   1 +
>>   drivers/leds/leds-spi-byte.c | 133 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 146 insertions(+)
>>   create mode 100644 drivers/leds/leds-spi-byte.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index a72f97fca57b..0866c55e8004 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -766,6 +766,18 @@ config LEDS_NIC78BX
>>         To compile this driver as a module, choose M here: the module
>>         will be called leds-nic78bx.
>>   +config LEDS_SPI_BYTE
>> +    tristate "LED support for SPI LED controller with a single byte"
>> +    depends on LEDS_CLASS
>> +    depends on SPI
>> +    depends on OF
>> +    help
>> +      This option enables support for LED controller which use a
>> single byte
>> +      for controlling the brightness. The minimum and maximum value
>> of the
>> +      byte can be configured via a device tree. The driver can be
>> used for
>> +      example for the microcontroller based LED controller in the
>> Ubiquiti
>> +      airCube ISP devices.
>> +
>>   comment "LED Triggers"
>>   source "drivers/leds/trigger/Kconfig"
>>   diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 4c1b0054f379..1786d7e2c236 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -75,6 +75,7 @@ obj-$(CONFIG_LEDS_PM8058)        += leds-pm8058.o
>>   obj-$(CONFIG_LEDS_MLXCPLD)        += leds-mlxcpld.o
>>   obj-$(CONFIG_LEDS_MLXREG)        += leds-mlxreg.o
>>   obj-$(CONFIG_LEDS_NIC78BX)        += leds-nic78bx.o
>> +obj-$(CONFIG_LEDS_SPI_BYTE)        += leds-spi-byte.o
>>   obj-$(CONFIG_LEDS_MT6323)        += leds-mt6323.o
>>   obj-$(CONFIG_LEDS_LM3692X)        += leds-lm3692x.o
>>   obj-$(CONFIG_LEDS_SC27XX_BLTC)        += leds-sc27xx-bltc.o
>> diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
>> new file mode 100644
>> index 000000000000..19a68bce1a7c
>> --- /dev/null
>> +++ b/drivers/leds/leds-spi-byte.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019 Christian Mauderer <oss@c-mauderer.de>
>> +
>> +/*
>> + * The driver can be used for controllers with a very simple SPI
>> protocol: Only
>> + * one byte between an off and a max value (defined by devicetree)
>> will be sent.
>> + */
>> +
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/mutex.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +struct spi_byte_led {
>> +    struct led_classdev    ldev;
>> +    struct spi_device    *spi;
>> +    char            name[LED_MAX_NAME_SIZE];
>> +    struct mutex        mutex;
>> +    u8            off_value;
>> +    u8            max_value;
>> +};
>> +
>> +static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
>> +                        enum led_brightness brightness)
>> +{
>> +    struct spi_byte_led *led = container_of(dev, struct spi_byte_led,
>> ldev);
>> +    u8 value;
>> +    int ret;
>> +
>> +    value = (u8) brightness + led->off_value;
>> +
>> +    mutex_lock(&led->mutex);
>> +    ret = spi_write(led->spi, &value, sizeof(value));
>> +    mutex_unlock(&led->mutex);
>> +
>> +    return ret;
>> +}
>> +
>> +static int spi_byte_probe(struct spi_device *spi)
>> +{
>> +    struct device *dev = &spi->dev;
>> +    struct device_node *child;
>> +    struct spi_byte_led *led;
>> +    int ret;
>> +    const char *default_name = "leds-spi-byte::";
>> +    const char *name;
>> +    u8 off_value;
>> +    u8 max_value;
>> +
>> +    if (!dev->of_node)
>> +        return -ENODEV;
>> +
>> +    if (of_get_child_count(dev->of_node) != 1) {
>> +        dev_err(dev, "Device must have exactly one LED sub-node.");
>> +        return -EINVAL;
>> +    }
>> +    child = of_get_next_child(dev->of_node, NULL);
>> +
>> +    ret = of_property_read_string(child, "label", &name);
>> +    if (ret != 0)
> 
> It is more customary to have "if (ret)" in such cases.
> Applies to all occurrences below.
> 
>> +        name = default_name;
>> +
>> +    ret = of_property_read_u8(child, "leds-spi-byte,off-value",
>> &off_value);
>> +    if (ret != 0) {
>> +        dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>> +        return -EINVAL;
>> +    }
>> +
>> +    ret = of_property_read_u8(child, "leds-spi-byte,max-value",
>> &max_value);
>> +    if (ret != 0) {
>> +        dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (off_value >= max_value) {
>> +        dev_err(dev, "off-value has to be smaller than max-value.");
>> +        return -EINVAL;
>> +    }
>> +
>> +    led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> +    if (!led)
>> +        return -ENOMEM;
>> +
>> +    led->spi = spi;
>> +    strlcpy(led->name, name, sizeof(led->name));
>> +    mutex_init(&led->mutex);
>> +    led->off_value = off_value;
>> +    led->max_value = max_value;
>> +    led->ldev.name = led->name;
>> +    led->ldev.brightness = LED_OFF;
> 
> This line is redundant - already zeroed by kzalloc.
> 
>> +    led->ldev.max_brightness = led->max_value - led->off_value;
>> +    led->ldev.brightness_set_blocking =
>> spi_byte_brightness_set_blocking;
>> +    ret = led_classdev_register(&spi->dev, &led->ldev);
> 
> Please use devm_led_classdev_register().
> 
>> +    if (ret >= 0)
>> +        spi_set_drvdata(spi, led);
> 
> This looks quite odd. Please shape it as below:
> 
>     if (ret)
>         return ret
> 
>     spi_set_drvdata(spi, led);
> 
>> +
>> +    return ret;
> 
>     s/ret/0/
> 
>> +}
>> +
>> +static int spi_byte_remove(struct spi_device *spi)
>> +{
>> +    struct spi_byte_led    *led = spi_get_drvdata(spi);
>> +
>> +    led_classdev_unregister(&led->ldev);
>> +    mutex_destroy(&led->mutex);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id spi_byte_dt_ids[] = {
>> +    { .compatible = "leds-spi-byte", },
>> +    {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
>> +
>> +static struct spi_driver spi_byte_driver = {
>> +    .probe        = spi_byte_probe,
>> +    .remove        = spi_byte_remove,
>> +    .driver = {
>> +        .name        = KBUILD_MODNAME,
>> +        .of_match_table    = spi_byte_dt_ids,
>> +    },
>> +};
>> +
>> +module_spi_driver(spi_byte_driver);
>> +
>> +MODULE_AUTHOR("Christian Mauderer <oss@c-mauderer.de>");
>> +MODULE_DESCRIPTION("single byte SPI LED driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("spi:leds-spi-byte");
>>
> 

Re: [PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Pavel Machek]

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

Hi!

> >+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> >+	if (!led)
> >+		return -ENOMEM;
> >+
> >+	led->spi = spi;
> >+	strlcpy(led->name, name, sizeof(led->name));
> >+	mutex_init(&led->mutex);
> >+	led->off_value = off_value;
> >+	led->max_value = max_value;
> >+	led->ldev.name = led->name;
> >+	led->ldev.brightness = LED_OFF;
> 
> This line is redundant - already zeroed by kzalloc.

Actually I'd prefer to leave it in. Yes, LED_OFF == 0, and will
probably stay == 0 in future, but...
									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 v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Christian Mauderer]

On 05/05/2019 22:12, Pavel Machek wrote:
> Hi!
> 
>>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>> +	if (!led)
>>> +		return -ENOMEM;
>>> +
>>> +	led->spi = spi;
>>> +	strlcpy(led->name, name, sizeof(led->name));
>>> +	mutex_init(&led->mutex);
>>> +	led->off_value = off_value;
>>> +	led->max_value = max_value;
>>> +	led->ldev.name = led->name;
>>> +	led->ldev.brightness = LED_OFF;
>>
>> This line is redundant - already zeroed by kzalloc.
> 
> Actually I'd prefer to leave it in. Yes, LED_OFF == 0, and will
> probably stay == 0 in future, but...
> 									Pavel
> 

Before I send v4: Currently the initial value isn't written to the LED
anywhere. The state that is set by U-Boot is just kept till the first
write to the brightness file.

I didn't implement "default-state" because the OpenWRT user space sets
the LED anyway a few seconds later (which is still my use case for that
driver). But now I noted that there is a remark in the documentation of
the option "default-state" in devicetree/bindings/leds/common.txt: "The
default is off if this property is not present."

Should I send an initial value to the device during initialization or is
it OK to just keep the original state?

Best regards

Christian

Re: [PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Jacek Anaszewski]

Hi Christian,

On 5/6/19 10:48 AM, Christian Mauderer wrote:
> On 05/05/2019 22:12, Pavel Machek wrote:
>> Hi!
>>
>>>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>>> +	if (!led)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	led->spi = spi;
>>>> +	strlcpy(led->name, name, sizeof(led->name));
>>>> +	mutex_init(&led->mutex);
>>>> +	led->off_value = off_value;
>>>> +	led->max_value = max_value;
>>>> +	led->ldev.name = led->name;
>>>> +	led->ldev.brightness = LED_OFF;
>>>
>>> This line is redundant - already zeroed by kzalloc.
>>
>> Actually I'd prefer to leave it in. Yes, LED_OFF == 0, and will
>> probably stay == 0 in future, but...
>> 									Pavel
>>
> 
> Before I send v4: Currently the initial value isn't written to the LED
> anywhere. The state that is set by U-Boot is just kept till the first
> write to the brightness file.
> 
> I didn't implement "default-state" because the OpenWRT user space sets
> the LED anyway a few seconds later (which is still my use case for that
> driver). But now I noted that there is a remark in the documentation of
> the option "default-state" in devicetree/bindings/leds/common.txt: "The
> default is off if this property is not present."
> 
> Should I send an initial value to the device during initialization or is
> it OK to just keep the original state?

Yes, I would make use of default-state in this case.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 2/2] leds: spi-byte: add single byte SPI LED driver

[Jacek Anaszewski]

On 5/5/19 10:12 PM, Pavel Machek wrote:
> Hi!
> 
>>> +	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>> +	if (!led)
>>> +		return -ENOMEM;
>>> +
>>> +	led->spi = spi;
>>> +	strlcpy(led->name, name, sizeof(led->name));
>>> +	mutex_init(&led->mutex);
>>> +	led->off_value = off_value;
>>> +	led->max_value = max_value;
>>> +	led->ldev.name = led->name;
>>> +	led->ldev.brightness = LED_OFF;
>>
>> This line is redundant - already zeroed by kzalloc.
> 
> Actually I'd prefer to leave it in. Yes, LED_OFF == 0, and will
> probably stay == 0 in future, but...

but what? I don't really see a sufficient justification for
leaving it. Just redundant line. In other place in my LED naming
patch set you wondered if it wouldn't have been better to initialize
a struct partly using initialization list, I suspect to save few
LOC. So here you seem to be inconsistent :-)

-- 
Best regards,
Jacek Anaszewski