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

[PATCH v3 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 v2:
- None

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 v3 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 v2:
- use "if (ret)" instead of "if (ret != 0)"
- don't initialize ldev-fields with zero
- use devm_led_classdev_register instead of led_classdev_register
- check for error instead of good case with the last if in spi_byte_probe

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


 drivers/leds/Kconfig         |  12 ++++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
 3 files changed, 147 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..8170b2da497a
--- /dev/null
+++ b/drivers/leds/leds-spi-byte.c
@@ -0,0 +1,134 @@
+// 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)
+		name = default_name;
+
+	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
+	if (ret) {
+		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) {
+		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.max_brightness = led->max_value - led->off_value;
+	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
+	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, led);
+
+	return 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");
-- 
2.21.0

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

[Jacek Anaszewski]

Christian,

Thank you for the update. One thing left after switching
to devm API. Please refer below to the remove op.

On 5/5/19 10:00 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 v2:
> - use "if (ret)" instead of "if (ret != 0)"
> - don't initialize ldev-fields with zero
> - use devm_led_classdev_register instead of led_classdev_register
> - check for error instead of good case with the last if in spi_byte_probe
> 
> 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
> 
> 
>   drivers/leds/Kconfig         |  12 ++++
>   drivers/leds/Makefile        |   1 +
>   drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>   3 files changed, 147 insertions(+)
>   create mode 100644 drivers/leds/leds-spi-byte.c

[...]
> +
> +static int spi_byte_remove(struct spi_device *spi)
> +{
> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
> +
> +	led_classdev_unregister(&led->ldev);

This is now not needed - devm, means "device managed", i.e.
all resources claimed with it will be automatically reclaimed
on device destruction.

> +	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 v3 2/2] leds: spi-byte: add single byte SPI LED driver

[Christian Mauderer]

On 05/05/2019 22:09, Jacek Anaszewski wrote:
> Christian,
> 
> Thank you for the update. One thing left after switching
> to devm API. Please refer below to the remove op.
> 
> On 5/5/19 10:00 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 v2:
>> - use "if (ret)" instead of "if (ret != 0)"
>> - don't initialize ldev-fields with zero
>> - use devm_led_classdev_register instead of led_classdev_register
>> - check for error instead of good case with the last if in spi_byte_probe
>>
>> 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
>>
>>
>>   drivers/leds/Kconfig         |  12 ++++
>>   drivers/leds/Makefile        |   1 +
>>   drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 147 insertions(+)
>>   create mode 100644 drivers/leds/leds-spi-byte.c
> 
> [...]
>> +
>> +static int spi_byte_remove(struct spi_device *spi)
>> +{
>> +    struct spi_byte_led    *led = spi_get_drvdata(spi);
>> +
>> +    led_classdev_unregister(&led->ldev);
> 
> This is now not needed - devm, means "device managed", i.e.
> all resources claimed with it will be automatically reclaimed
> on device destruction.
> 

Thanks for the hint. I've read that the devm functions are some "free
automatically" functions. But I haven't drawn the conclusion that I have
to remove this function. Seems I still need to learn a lot about the
Linux API.

I'll wait for further feedback over night and then remove it in a v4
tomorrow. Otherwise I'll spam the list with lots of new versions.

>> +    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 v3 2/2] leds: spi-byte: add single byte SPI LED driver

[Dan Murphy]

Christian

On 5/5/19 3:00 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 v2:
> - use "if (ret)" instead of "if (ret != 0)"
> - don't initialize ldev-fields with zero
> - use devm_led_classdev_register instead of led_classdev_register
> - check for error instead of good case with the last if in spi_byte_probe
> 
> 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
> 
> 
>  drivers/leds/Kconfig         |  12 ++++
>  drivers/leds/Makefile        |   1 +
>  drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>  3 files changed, 147 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..8170b2da497a
> --- /dev/null
> +++ b/drivers/leds/leds-spi-byte.c
> @@ -0,0 +1,134 @@
> +// 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;
> +

Sorry if this has been addressed but the versions moved fast.

What is the purpose of adding the off_value?

If max is 63 and say off value is 1 then this will set brightness to 64 but that is not what the LED framework will send.
if you read the brightness.

Is it safe to assume that off_value would always be 0?


> +	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)
> +		name = default_name;
> +
> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
> +	if (ret) {
> +		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) {
> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
> +		return -EINVAL;
> +	}
> +

You could probably allocate the led struct memory first and then pass in the address of those
variables as opposed to creating the stack variables.

	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
	if (!led)
		return -ENOMEM;

	ret = of_property_read_string(child, "label", &led->ldev.name);
	if (ret)
		led->ldev.name = default_name;

	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
	if (ret) {
		dev_err(dev, "LED node needs a leds-spi-byte,off-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.max_brightness = led->max_value - led->off_value;

Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
And this is not documented in the DT doc.

max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
But that is not what is described in the DT.

> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, led);

If you move this above the registration this can just become

return = devm_led_classdev_register(&spi->dev, &led->ldev);

> +
> +	return 0;
> +}
> +
> +static int spi_byte_remove(struct spi_device *spi)
> +{
> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
> +
> +	led_classdev_unregister(&led->ldev);

Don't need this with devm call

Dan

<snip>

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

[Christian Mauderer]

Hello Dan,

thanks for reviewing the patch.

On 06/05/2019 14:05, Dan Murphy wrote:
> Christian
> 
> On 5/5/19 3:00 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 v2:
>> - use "if (ret)" instead of "if (ret != 0)"
>> - don't initialize ldev-fields with zero
>> - use devm_led_classdev_register instead of led_classdev_register
>> - check for error instead of good case with the last if in spi_byte_probe
>>
>> 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
>>
>>
>>  drivers/leds/Kconfig         |  12 ++++
>>  drivers/leds/Makefile        |   1 +
>>  drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 147 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..8170b2da497a
>> --- /dev/null
>> +++ b/drivers/leds/leds-spi-byte.c
>> @@ -0,0 +1,134 @@
>> +// 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;
>> +
> 
> Sorry if this has been addressed but the versions moved fast.
> 
> What is the purpose of adding the off_value?
> 
> If max is 63 and say off value is 1 then this will set brightness to 64 but that is not what the LED framework will send.
> if you read the brightness.
> 
> Is it safe to assume that off_value would always be 0?

No it's not always 0.

In my target application (a microcontroller based LED controller from
Ubiquiti) I have the values from 0 to 63. But after some discussion I
wrote the driver to be more generic so it can cover some similar
controllers too.

So if there is a hypothetical SPI-based controller that uses a single
byte with values from 0x80 (off) to 0x8f (maximum brightness) to control
a LED, you could set the off and max values to these two and the driver
sends 0x80 if you set brightness to 0 and 0x8f if you set brightness to 15.

A more concrete application could be to let the LED slightly on to show
power by setting the off value to for example 10 but letting the max
value at 63. In that case there would be only 53 brightness levels left.

Of course it would have been possible to make it a lot more universal by
for example adding a prefix, a bit mask or other word lengths. But that
would have added a lot of complexity without any actual application.

> 
> 
>> +	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)
>> +		name = default_name;
>> +
>> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>> +	if (ret) {
>> +		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) {
>> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>> +		return -EINVAL;
>> +	}
>> +
> 
> You could probably allocate the led struct memory first and then pass in the address of those
> variables as opposed to creating the stack variables.
> 
> 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> 	if (!led)
> 		return -ENOMEM;
> 
> 	ret = of_property_read_string(child, "label", &led->ldev.name);
> 	if (ret)
> 		led->ldev.name = default_name;
> 
> 	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
> 	if (ret) {
> 		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
> 		return -EINVAL;
> 	}
> 	.
> 	.
> 	.

I had that in the first revision. Also no one objected, I noted that I
had to search whether I have initialized all fields when I added another
one. Therefore I thought it would be more readable if I initialize the
complete structure at one location. I put readability over efficiency in
that case because it's only called once during initialization. Of course
I can change that back.

> 
> 
>> +	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.max_brightness = led->max_value - led->off_value;
> 
> Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
> And this is not documented in the DT doc.
> 
> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
> But that is not what is described in the DT.

Then my description in the DT wasn't clear enough. I wanted to express
that with the sentence: "The value of the byte can be any value between
the off-value and max-value defined in the properties."

Should I add another example (beneath the Ubiquiti controller) like the
following in the description to make it more clear?

"Another example could be a controller with the following control byte
structure:
- Bit 8 to 5: always 0x8
- Bit 4 to 0: brightness value
In that case the off-value would be 0x80 and the max-value would be 0x8f."

> 
>> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	spi_set_drvdata(spi, led);
> 
> If you move this above the registration this can just become
> 
> return = devm_led_classdev_register(&spi->dev, &led->ldev);

Good point. I'll change that.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int spi_byte_remove(struct spi_device *spi)
>> +{
>> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
>> +
>> +	led_classdev_unregister(&led->ldev);
> 
> Don't need this with devm call

Thanks for the hint. Jacek told me that already. I wanted to wait for
some further feedback before spamming the list with another version.

> 
> Dan
> 
> <snip>
> 

Best regards

Christian

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

[Dan Murphy]

On 5/6/19 7:59 AM, Christian Mauderer wrote:
> Hello Dan,
> 
> thanks for reviewing the patch.
> 
> On 06/05/2019 14:05, Dan Murphy wrote:
>> Christian
>>
>> On 5/5/19 3:00 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 v2:
>>> - use "if (ret)" instead of "if (ret != 0)"
>>> - don't initialize ldev-fields with zero
>>> - use devm_led_classdev_register instead of led_classdev_register
>>> - check for error instead of good case with the last if in spi_byte_probe
>>>
>>> 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
>>>
>>>
>>>  drivers/leds/Kconfig         |  12 ++++
>>>  drivers/leds/Makefile        |   1 +
>>>  drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>>>  3 files changed, 147 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..8170b2da497a
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-spi-byte.c
>>> @@ -0,0 +1,134 @@
>>> +// 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;
>>> +
>>
>> Sorry if this has been addressed but the versions moved fast.
>>
>> What is the purpose of adding the off_value?
>>
>> If max is 63 and say off value is 1 then this will set brightness to 64 but that is not what the LED framework will send.
>> if you read the brightness.
>>
>> Is it safe to assume that off_value would always be 0?
> 
> No it's not always 0.
> 
> In my target application (a microcontroller based LED controller from
> Ubiquiti) I have the values from 0 to 63. But after some discussion I
> wrote the driver to be more generic so it can cover some similar
> controllers too.
> 
> So if there is a hypothetical SPI-based controller that uses a single
> byte with values from 0x80 (off) to 0x8f (maximum brightness) to control
> a LED, you could set the off and max values to these two and the driver
> sends 0x80 if you set brightness to 0 and 0x8f if you set brightness to 15.
> 

In this case would the max brightness just be 0x7f.
Maybe an optional DT property for brightness mask would help here.

Not sure what the msb is in the example but if this bit defines the  LED on/off control then 
maybe an optional dt property for this would be more clear.

> A more concrete application could be to let the LED slightly on to show
> power by setting the off value to for example 10 but letting the max
> value at 63. In that case there would be only 53 brightness levels left.
> 

But then the LED is not off by user space request.  When the LED is asked to be off
it should be off.  The brightness values requested by the user space should be honored and
not modified by the driver.

> Of course it would have been possible to make it a lot more universal by
> for example adding a prefix, a bit mask or other word lengths. But that
> would have added a lot of complexity without any actual application.
> 

I have to disagree here.  If this is supposed to be a universal SPI byte driver that
needs special handling then it is either needs to be created in a universal way or needs to be made
target specific.

>>
>>
>>> +	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)
>>> +		name = default_name;
>>> +
>>> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>>> +	if (ret) {
>>> +		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) {
>>> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>
>> You could probably allocate the led struct memory first and then pass in the address of those
>> variables as opposed to creating the stack variables.
>>
>> 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> 	if (!led)
>> 		return -ENOMEM;
>>
>> 	ret = of_property_read_string(child, "label", &led->ldev.name);
>> 	if (ret)
>> 		led->ldev.name = default_name;
>>
>> 	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
>> 	if (ret) {
>> 		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>> 		return -EINVAL;
>> 	}
>> 	.
>> 	.
>> 	.
> 
> I had that in the first revision. Also no one objected, I noted that I
> had to search whether I have initialized all fields when I added another
> one. Therefore I thought it would be more readable if I initialize the
> complete structure at one location. I put readability over efficiency in
> that case because it's only called once during initialization. Of course
> I can change that back.
> 

Well for readability you could also create a function that parses the node after allocating
the memory.  That way all the DT parsing and value checking is done in a single function.

>>
>>
>>> +	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.max_brightness = led->max_value - led->off_value;
>>
>> Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
>> And this is not documented in the DT doc.
>>
>> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
>> But that is not what is described in the DT.
> 
> Then my description in the DT wasn't clear enough. I wanted to express
> that with the sentence: "The value of the byte can be any value between
> the off-value and max-value defined in the properties."
> 
> Should I add another example (beneath the Ubiquiti controller) like the
> following in the description to make it more clear?
> 
> "Another example could be a controller with the following control byte
> structure:
> - Bit 8 to 5: always 0x8
> - Bit 4 to 0: brightness value
> In that case the off-value would be 0x80 and the max-value would be 0x8f."
> 

In this case max-brightness would be 0xf.  No math needed.  With the aid of a brightness mask
then the code would need to do a read before write.
This makes this driver more extensible if it truly needs to be universal and generic.

read_value = 0

if (led->brightness_mask)
	spi_read()

value = (u8) brightness & led->brightness_mask | read_value; 
// or it can also skip brightness_mask and use max_brightness
// value = (u8) brightness & led->max_brightness | read_value; 

spi_write(value)

This aligns what is declared in the DT to what is expected from the user space.

>>
>>> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>>> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	spi_set_drvdata(spi, led);
>>
>> If you move this above the registration this can just become
>>
>> return = devm_led_classdev_register(&spi->dev, &led->ldev);
> 
> Good point. I'll change that.
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int spi_byte_remove(struct spi_device *spi)
>>> +{
>>> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
>>> +
>>> +	led_classdev_unregister(&led->ldev);
>>
>> Don't need this with devm call
> 
> Thanks for the hint. Jacek told me that already. I wanted to wait for
> some further feedback before spamming the list with another version.
> 

One other thing if the LED driver is removed and the LED is on and unmanaged that is ok?

Dan

>>
>> Dan
>>
>> <snip>
>>
> 
> Best regards
> 
> Christian
> 

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

[Pavel Machek]

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

Hi!

> > Of course it would have been possible to make it a lot more universal by
> > for example adding a prefix, a bit mask or other word lengths. But that
> > would have added a lot of complexity without any actual application.
> > 
> 
> I have to disagree here.  If this is supposed to be a universal SPI byte driver that
> needs special handling then it is either needs to be created in a universal way or needs to be made
> target specific.
> 

Let him be. The driver is good.

If some hardware needs more flexibility, we add it.

No need to have 1000 releases of everything.

									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 v3 2/2] leds: spi-byte: add single byte SPI LED driver

[Christian Mauderer]

On 06/05/2019 17:15, Pavel Machek wrote:
> Hi!
> 
>>> Of course it would have been possible to make it a lot more universal by
>>> for example adding a prefix, a bit mask or other word lengths. But that
>>> would have added a lot of complexity without any actual application.
>>>
>>
>> I have to disagree here.  If this is supposed to be a universal SPI byte driver that
>> needs special handling then it is either needs to be created in a universal way or needs to be made
>> target specific.
>>
> 
> Let him be. The driver is good.
> 
> If some hardware needs more flexibility, we add it.
> 
> No need to have 1000 releases of everything.
> 
> 									Pavel
> 

Hello Pavel,

thanks for the support.

It's a pure hobby project so I have the time to add useful features or
to improve the description to make it clear what the drivers intention
is. So if we find a more useful set of features it's a good idea to
discuss it.

By the way: Although I haven't written a Linux driver yet it's not my
first open source project. So I know that there can be a lot of
different opinions and sometimes a lot of revisions. So no big risk of
scaring me away.

Best regards

Christian

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

[Dan Murphy]

Christian

On 5/6/19 10:29 AM, Christian Mauderer wrote:
> On 06/05/2019 17:15, Pavel Machek wrote:
>> Hi!
>>
>>>> Of course it would have been possible to make it a lot more universal by
>>>> for example adding a prefix, a bit mask or other word lengths. But that
>>>> would have added a lot of complexity without any actual application.
>>>>
>>>
>>> I have to disagree here.  If this is supposed to be a universal SPI byte driver that
>>> needs special handling then it is either needs to be created in a universal way or needs to be made
>>> target specific.
>>>
>>
>> Let him be. The driver is good.
>>
>> If some hardware needs more flexibility, we add it.
>>
>> No need to have 1000 releases of everything.
>>
>> 									Pavel
>>
> 
> Hello Pavel,
> 
> thanks for the support.
> 
> It's a pure hobby project so I have the time to add useful features or
> to improve the description to make it clear what the drivers intention
> is. So if we find a more useful set of features it's a good idea to
> discuss it.
> 
> By the way: Although I haven't written a Linux driver yet it's not my
> first open source project. So I know that there can be a lot of
> different opinions and sometimes a lot of revisions. So no big risk of
> scaring me away.
> 

Hopefully the feedback from all is helping you with developing kernel drivers :).

One request though is can you slow down a bit in the versions?
I had 3 versions in my inbox before I had a chance to review v1.

I have been asked the same thing by someone and now I try to give at least 24-48 hours so others over seas
can get a chance to review prior to posting a new version.

Dan

> Best regards
> 
> Christian
> 

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

[Christian Mauderer]

Hello Dan,

On 06/05/2019 19:40, Dan Murphy wrote:
> Christian
> 
> On 5/6/19 10:29 AM, Christian Mauderer wrote:
>> On 06/05/2019 17:15, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> Of course it would have been possible to make it a lot more universal by
>>>>> for example adding a prefix, a bit mask or other word lengths. But that
>>>>> would have added a lot of complexity without any actual application.
>>>>>
>>>>
>>>> I have to disagree here.  If this is supposed to be a universal SPI byte driver that
>>>> needs special handling then it is either needs to be created in a universal way or needs to be made
>>>> target specific.
>>>>
>>>
>>> Let him be. The driver is good.
>>>
>>> If some hardware needs more flexibility, we add it.
>>>
>>> No need to have 1000 releases of everything.
>>>
>>> 									Pavel
>>>
>>
>> Hello Pavel,
>>
>> thanks for the support.
>>
>> It's a pure hobby project so I have the time to add useful features or
>> to improve the description to make it clear what the drivers intention
>> is. So if we find a more useful set of features it's a good idea to
>> discuss it.
>>
>> By the way: Although I haven't written a Linux driver yet it's not my
>> first open source project. So I know that there can be a lot of
>> different opinions and sometimes a lot of revisions. So no big risk of
>> scaring me away.
>>
> 
> Hopefully the feedback from all is helping you with developing kernel drivers :).

I hadn't expected a such big discussion for such a small driver. But
it's no problem. Part of my work is to develop drivers for a open source
real time operating system. So I'm used to discussions. The community is
only a lot smaller and the drivers are far less standardized than for Linux.

> 
> One request though is can you slow down a bit in the versions?
> I had 3 versions in my inbox before I had a chance to review v1.
> 
> I have been asked the same thing by someone and now I try to give at least 24-48 hours so others over seas
> can get a chance to review prior to posting a new version.

I planned to wait till the discussion cools down before creating a v4. I
only have been so fast because yesterday I had the impression that some
big changes are requested and therefore created a v2. After that the
feedback seemed to only need detail changes therefore v3. But I forgot
that it maybe would be a good idea to wait till Monday.

Best regards

Christian

>
> Dan
> 
>> Best regards
>>
>> Christian
>>

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

[Christian Mauderer]

On 06/05/2019 16:58, Dan Murphy wrote:
> 
> 
> On 5/6/19 7:59 AM, Christian Mauderer wrote:
>> Hello Dan,
>>
>> thanks for reviewing the patch.
>>
>> On 06/05/2019 14:05, Dan Murphy wrote:
>>> Christian
>>>
>>> On 5/5/19 3:00 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 v2:
>>>> - use "if (ret)" instead of "if (ret != 0)"
>>>> - don't initialize ldev-fields with zero
>>>> - use devm_led_classdev_register instead of led_classdev_register
>>>> - check for error instead of good case with the last if in spi_byte_probe
>>>>
>>>> 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
>>>>
>>>>
>>>>  drivers/leds/Kconfig         |  12 ++++
>>>>  drivers/leds/Makefile        |   1 +
>>>>  drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 147 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..8170b2da497a
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-spi-byte.c
>>>> @@ -0,0 +1,134 @@
>>>> +// 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;
>>>> +
>>>
>>> Sorry if this has been addressed but the versions moved fast.
>>>
>>> What is the purpose of adding the off_value?
>>>
>>> If max is 63 and say off value is 1 then this will set brightness to 64 but that is not what the LED framework will send.
>>> if you read the brightness.
>>>
>>> Is it safe to assume that off_value would always be 0?
>>
>> No it's not always 0.
>>
>> In my target application (a microcontroller based LED controller from
>> Ubiquiti) I have the values from 0 to 63. But after some discussion I
>> wrote the driver to be more generic so it can cover some similar
>> controllers too.
>>
>> So if there is a hypothetical SPI-based controller that uses a single
>> byte with values from 0x80 (off) to 0x8f (maximum brightness) to control
>> a LED, you could set the off and max values to these two and the driver
>> sends 0x80 if you set brightness to 0 and 0x8f if you set brightness to 15.
>>
> 
> In this case would the max brightness just be 0x7f.
> Maybe an optional DT property for brightness mask would help here.
> 
> Not sure what the msb is in the example but if this bit defines the  LED on/off control then 
> maybe an optional dt property for this would be more clear.
> 
>> A more concrete application could be to let the LED slightly on to show
>> power by setting the off value to for example 10 but letting the max
>> value at 63. In that case there would be only 53 brightness levels left.
>>
> 
> But then the LED is not off by user space request.  When the LED is asked to be off
> it should be off.  The brightness values requested by the user space should be honored and
> not modified by the driver.

That's correct. It was an example what would be possible. You are
correct that this would be an ugly hack.

> 
>> Of course it would have been possible to make it a lot more universal by
>> for example adding a prefix, a bit mask or other word lengths. But that
>> would have added a lot of complexity without any actual application.
>>
> 
> I have to disagree here.  If this is supposed to be a universal SPI byte driver that
> needs special handling then it is either needs to be created in a universal way or needs to be made
> target specific.

If it should be a truly universal driver for SPI based controllers I
would at least need the following additional features:

- Variable direction (led brighter with lower or higher values).
- Counter at any location of the byte. Maybe even some odd combinations
like bit 7, 3 and 5 in this order.
- Sending some bytes before the LED brightness value.
- Sending multiple bytes for multiple LEDs.
- Configurable other bits.

And that would only include controllers without a SPI MISO channel. So
where does "universal" stop? I stopped when my application and maybe
some others (like using a digital potentiometer with an similar
interface) could be covered.

So it's not a universal but a multi-purpose driver that can be used for
every controller with the following interface:

- Only has a MOSI line. MISO can be ignored.
- Expect one byte via SPI.
- Expect a range of values from x to y to set brightness from off (x) to
bright (y).

It can be extended if an application appears that needs more than that.
Maybe it's a good idea to add that list of requirements to the device
tree description?

> 
>>>
>>>
>>>> +	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)
>>>> +		name = default_name;
>>>> +
>>>> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>>>> +	if (ret) {
>>>> +		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) {
>>>> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>
>>> You could probably allocate the led struct memory first and then pass in the address of those
>>> variables as opposed to creating the stack variables.
>>>
>>> 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>> 	if (!led)
>>> 		return -ENOMEM;
>>>
>>> 	ret = of_property_read_string(child, "label", &led->ldev.name);
>>> 	if (ret)
>>> 		led->ldev.name = default_name;
>>>
>>> 	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
>>> 	if (ret) {
>>> 		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>>> 		return -EINVAL;
>>> 	}
>>> 	.
>>> 	.
>>> 	.
>>
>> I had that in the first revision. Also no one objected, I noted that I
>> had to search whether I have initialized all fields when I added another
>> one. Therefore I thought it would be more readable if I initialize the
>> complete structure at one location. I put readability over efficiency in
>> that case because it's only called once during initialization. Of course
>> I can change that back.
>>
> 
> Well for readability you could also create a function that parses the node after allocating
> the memory.  That way all the DT parsing and value checking is done in a single function.
> 

OK for me too. I'm quite indifferent here. It's a matter of preference
how to style something like that.

>>>
>>>
>>>> +	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.max_brightness = led->max_value - led->off_value;
>>>
>>> Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
>>> And this is not documented in the DT doc.
>>>
>>> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
>>> But that is not what is described in the DT.
>>
>> Then my description in the DT wasn't clear enough. I wanted to express
>> that with the sentence: "The value of the byte can be any value between
>> the off-value and max-value defined in the properties."
>>
>> Should I add another example (beneath the Ubiquiti controller) like the
>> following in the description to make it more clear?
>>
>> "Another example could be a controller with the following control byte
>> structure:
>> - Bit 8 to 5: always 0x8
>> - Bit 4 to 0: brightness value
>> In that case the off-value would be 0x80 and the max-value would be 0x8f."
>>
> 
> In this case max-brightness would be 0xf.  No math needed.  With the aid of a brightness mask
> then the code would need to do a read before write.
> This makes this driver more extensible if it truly needs to be universal and generic.

That would mean that the controller allows a read of the register. Not
every controller does that. For example the one I want to support just
returns the previously sent byte.

> 
> read_value = 0
> 
> if (led->brightness_mask)
> 	spi_read()
> 
> value = (u8) brightness & led->brightness_mask | read_value; 
> // or it can also skip brightness_mask and use max_brightness
> // value = (u8) brightness & led->max_brightness | read_value; 
> 
> spi_write(value)
> 
> This aligns what is declared in the DT to what is expected from the user space.
> >>>
>>>> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>>>> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	spi_set_drvdata(spi, led);
>>>
>>> If you move this above the registration this can just become
>>>
>>> return = devm_led_classdev_register(&spi->dev, &led->ldev);
>>
>> Good point. I'll change that.
>>
>>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int spi_byte_remove(struct spi_device *spi)
>>>> +{
>>>> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
>>>> +
>>>> +	led_classdev_unregister(&led->ldev);
>>>
>>> Don't need this with devm call
>>
>> Thanks for the hint. Jacek told me that already. I wanted to wait for
>> some further feedback before spamming the list with another version.
>>
> 
> One other thing if the LED driver is removed and the LED is on and unmanaged that is ok?
> 

Is that any different for any of the other drivers? As soon as I remove
a driver, the device is unmanaged, isn't it?

Best regards

Christian

> Dan
> 
>>>
>>> Dan
>>>
>>> <snip>
>>>
>>
>> Best regards
>>
>> Christian
>>

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

[Dan Murphy]

Christian

On 5/6/19 10:19 AM, Christian Mauderer wrote:
> On 06/05/2019 16:58, Dan Murphy wrote:
<snip>

> 
> If it should be a truly universal driver for SPI based controllers I
> would at least need the following additional features:
> 
> - Variable direction (led brighter with lower or higher values).
> - Counter at any location of the byte. Maybe even some odd combinations
> like bit 7, 3 and 5 in this order.
> - Sending some bytes before the LED brightness value.
> - Sending multiple bytes for multiple LEDs.
> - Configurable other bits.
> 
> And that would only include controllers without a SPI MISO channel. So
> where does "universal" stop? I stopped when my application and maybe
> some others (like using a digital potentiometer with an similar
> interface) could be covered.
> 
> So it's not a universal but a multi-purpose driver that can be used for
> every controller with the following interface:
> 
> - Only has a MOSI line. MISO can be ignored.
> - Expect one byte via SPI.
> - Expect a range of values from x to y to set brightness from off (x) to
> bright (y).
> 
> It can be extended if an application appears that needs more than that.
> Maybe it's a good idea to add that list of requirements to the device
> tree description?
> 

Yes that would be a good idea since it is a multi-purpose driver with very specific requirements.

>>
>>>>
>>>>
>>>>> +	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)
>>>>> +		name = default_name;
>>>>> +
>>>>> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>>>>> +	if (ret) {
>>>>> +		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) {
>>>>> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>
>>>> You could probably allocate the led struct memory first and then pass in the address of those
>>>> variables as opposed to creating the stack variables.
>>>>
>>>> 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>>> 	if (!led)
>>>> 		return -ENOMEM;
>>>>
>>>> 	ret = of_property_read_string(child, "label", &led->ldev.name);
>>>> 	if (ret)
>>>> 		led->ldev.name = default_name;
>>>>
>>>> 	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
>>>> 	if (ret) {
>>>> 		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>>>> 		return -EINVAL;
>>>> 	}
>>>> 	.
>>>> 	.
>>>> 	.
>>>
>>> I had that in the first revision. Also no one objected, I noted that I
>>> had to search whether I have initialized all fields when I added another
>>> one. Therefore I thought it would be more readable if I initialize the
>>> complete structure at one location. I put readability over efficiency in
>>> that case because it's only called once during initialization. Of course
>>> I can change that back.
>>>
>>
>> Well for readability you could also create a function that parses the node after allocating
>> the memory.  That way all the DT parsing and value checking is done in a single function.
>>
> 
> OK for me too. I'm quite indifferent here. It's a matter of preference
> how to style something like that.
> 

True not saying to do create the function it was just a suggestion.

>>>>
>>>>
>>>>> +	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.max_brightness = led->max_value - led->off_value;
>>>>
>>>> Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
>>>> And this is not documented in the DT doc.
>>>>
>>>> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
>>>> But that is not what is described in the DT.
>>>
>>> Then my description in the DT wasn't clear enough. I wanted to express
>>> that with the sentence: "The value of the byte can be any value between
>>> the off-value and max-value defined in the properties."
>>>
>>> Should I add another example (beneath the Ubiquiti controller) like the
>>> following in the description to make it more clear?
>>>
>>> "Another example could be a controller with the following control byte
>>> structure:
>>> - Bit 8 to 5: always 0x8
>>> - Bit 4 to 0: brightness value
>>> In that case the off-value would be 0x80 and the max-value would be 0x8f."
>>>
>>
>> In this case max-brightness would be 0xf.  No math needed.  With the aid of a brightness mask
>> then the code would need to do a read before write.
>> This makes this driver more extensible if it truly needs to be universal and generic.
> 
> That would mean that the controller allows a read of the register. Not
> every controller does that. For example the one I want to support just
> returns the previously sent byte.
> 

True a spi_read would be ineffective here.

>>
>> read_value = 0
>>
>> if (led->brightness_mask)
>> 	spi_read()
>>
>> value = (u8) brightness & led->brightness_mask | read_value; 
>> // or it can also skip brightness_mask and use max_brightness
>> // value = (u8) brightness & led->max_brightness | read_value; 
>>
>> spi_write(value)
>>
>> This aligns what is declared in the DT to what is expected from the user space.
>>>>>
>>>>> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>>>>> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	spi_set_drvdata(spi, led);
>>>>
>>>> If you move this above the registration this can just become
>>>>
>>>> return = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>
>>> Good point. I'll change that.
>>>
>>>>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int spi_byte_remove(struct spi_device *spi)
>>>>> +{
>>>>> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
>>>>> +
>>>>> +	led_classdev_unregister(&led->ldev);
>>>>
>>>> Don't need this with devm call
>>>
>>> Thanks for the hint. Jacek told me that already. I wanted to wait for
>>> some further feedback before spamming the list with another version.
>>>
>>
>> One other thing if the LED driver is removed and the LED is on and unmanaged that is ok?
>>
> 
> Is that any different for any of the other drivers? As soon as I remove
> a driver, the device is unmanaged, isn't it?
> 

True.

Dan

> Best regards
> 
> Christian
> 
>> Dan
>>
>>>>
>>>> Dan
>>>>
>>>> <snip>
>>>>
>>>
>>> Best regards
>>>
>>> Christian
>>>

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

[Christian Mauderer]

Hello Dan,

On 06/05/2019 17:37, Dan Murphy wrote:
> Christian
> 
> On 5/6/19 10:19 AM, Christian Mauderer wrote:
>> On 06/05/2019 16:58, Dan Murphy wrote:
> <snip>
> 
>>
>> If it should be a truly universal driver for SPI based controllers I
>> would at least need the following additional features:
>>
>> - Variable direction (led brighter with lower or higher values).
>> - Counter at any location of the byte. Maybe even some odd combinations
>> like bit 7, 3 and 5 in this order.
>> - Sending some bytes before the LED brightness value.
>> - Sending multiple bytes for multiple LEDs.
>> - Configurable other bits.
>>
>> And that would only include controllers without a SPI MISO channel. So
>> where does "universal" stop? I stopped when my application and maybe
>> some others (like using a digital potentiometer with an similar
>> interface) could be covered.
>>
>> So it's not a universal but a multi-purpose driver that can be used for
>> every controller with the following interface:
>>
>> - Only has a MOSI line. MISO can be ignored.
>> - Expect one byte via SPI.
>> - Expect a range of values from x to y to set brightness from off (x) to
>> bright (y).
>>
>> It can be extended if an application appears that needs more than that.
>> Maybe it's a good idea to add that list of requirements to the device
>> tree description?
>>
> 
> Yes that would be a good idea since it is a multi-purpose driver with very specific requirements.

OK. I'll improve the description with this list (maybe with a slightly
better formulation).

> 
>>>
>>>>>
>>>>>
>>>>>> +	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)
>>>>>> +		name = default_name;
>>>>>> +
>>>>>> +	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>>>>>> +	if (ret) {
>>>>>> +		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) {
>>>>>> +		dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> You could probably allocate the led struct memory first and then pass in the address of those
>>>>> variables as opposed to creating the stack variables.
>>>>>
>>>>> 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>>>> 	if (!led)
>>>>> 		return -ENOMEM;
>>>>>
>>>>> 	ret = of_property_read_string(child, "label", &led->ldev.name);
>>>>> 	if (ret)
>>>>> 		led->ldev.name = default_name;
>>>>>
>>>>> 	ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
>>>>> 	if (ret) {
>>>>> 		dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>>>>> 		return -EINVAL;
>>>>> 	}
>>>>> 	.
>>>>> 	.
>>>>> 	.
>>>>
>>>> I had that in the first revision. Also no one objected, I noted that I
>>>> had to search whether I have initialized all fields when I added another
>>>> one. Therefore I thought it would be more readable if I initialize the
>>>> complete structure at one location. I put readability over efficiency in
>>>> that case because it's only called once during initialization. Of course
>>>> I can change that back.
>>>>
>>>
>>> Well for readability you could also create a function that parses the node after allocating
>>> the memory.  That way all the DT parsing and value checking is done in a single function.
>>>
>>
>> OK for me too. I'm quite indifferent here. It's a matter of preference
>> how to style something like that.
>>
> 
> True not saying to do create the function it was just a suggestion.
> 

I'll try it and have a look at what is better. If everything open
firmware related is in a extra function, that can simplify a potential
future change to get the information from somewhere else.

>>>>>
>>>>>
>>>>>> +	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.max_brightness = led->max_value - led->off_value;
>>>>>
>>>>> Again not sure why this is needed.  This is changing the behavior of what max brightness value is defined in the DT.
>>>>> And this is not documented in the DT doc.
>>>>>
>>>>> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
>>>>> But that is not what is described in the DT.
>>>>
>>>> Then my description in the DT wasn't clear enough. I wanted to express
>>>> that with the sentence: "The value of the byte can be any value between
>>>> the off-value and max-value defined in the properties."
>>>>
>>>> Should I add another example (beneath the Ubiquiti controller) like the
>>>> following in the description to make it more clear?
>>>>
>>>> "Another example could be a controller with the following control byte
>>>> structure:
>>>> - Bit 8 to 5: always 0x8
>>>> - Bit 4 to 0: brightness value
>>>> In that case the off-value would be 0x80 and the max-value would be 0x8f."
>>>>
>>>
>>> In this case max-brightness would be 0xf.  No math needed.  With the aid of a brightness mask
>>> then the code would need to do a read before write.
>>> This makes this driver more extensible if it truly needs to be universal and generic.
>>
>> That would mean that the controller allows a read of the register. Not
>> every controller does that. For example the one I want to support just
>> returns the previously sent byte.
>>
> 
> True a spi_read would be ineffective here.
> 
>>>
>>> read_value = 0
>>>
>>> if (led->brightness_mask)
>>> 	spi_read()
>>>
>>> value = (u8) brightness & led->brightness_mask | read_value; 
>>> // or it can also skip brightness_mask and use max_brightness
>>> // value = (u8) brightness & led->max_brightness | read_value; 
>>>
>>> spi_write(value)
>>>
>>> This aligns what is declared in the DT to what is expected from the user space.
>>>>>>
>>>>>> +	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>>>>>> +	ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	spi_set_drvdata(spi, led);
>>>>>
>>>>> If you move this above the registration this can just become
>>>>>
>>>>> return = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>>
>>>> Good point. I'll change that.
>>>>
>>>>>
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int spi_byte_remove(struct spi_device *spi)
>>>>>> +{
>>>>>> +	struct spi_byte_led	*led = spi_get_drvdata(spi);
>>>>>> +
>>>>>> +	led_classdev_unregister(&led->ldev);
>>>>>
>>>>> Don't need this with devm call
>>>>
>>>> Thanks for the hint. Jacek told me that already. I wanted to wait for
>>>> some further feedback before spamming the list with another version.
>>>>
>>>
>>> One other thing if the LED driver is removed and the LED is on and unmanaged that is ok?
>>>
>>
>> Is that any different for any of the other drivers? As soon as I remove
>> a driver, the device is unmanaged, isn't it?
>>
> 
> True.
> 
> Dan
> 
>> Best regards
>>
>> Christian
>>
>>> Dan
>>>
>>>>>
>>>>> Dan
>>>>>
>>>>> <snip>
>>>>>
>>>>
>>>> Best regards
>>>>
>>>> Christian
>>>>

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

[Rob Herring]

On Sun, May 5, 2019 at 3:00 PM <oss@c-mauderer.de> wrote:
>
> 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 v2:
> - None
>
> 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.

What about power control of the uC?

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

This part seems in no way generic.

How does one support the 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".

Generally, we don't do "generic" bindings like this. The exceptions
are either we have confidence they really can be generic or they where
created before we knew better. A sample size of 1 doesn't convince me
the former is true.

This comment *only* applies to the binding, not the driver. Specific
bindings can easily be bound to generic drivers.

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

What's the SPI mode configuration?

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

Can't we already express this with brightness-levels and
num-interpolated-steps properties? Some reason those ended up in
pwm-backlight.txt, but really could apply to any LED with level
controls.

Rob

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

[Pavel Machek]

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

Hi!

> > +* 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.
> 
> What about power control of the uC?
> 
> > +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.
> 
> This part seems in no way generic.
> 
> How does one support the 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".
> 
> Generally, we don't do "generic" bindings like this. The exceptions
> are either we have confidence they really can be generic or they where
> created before we knew better. A sample size of 1 doesn't convince me
> the former is true.
> 
> This comment *only* applies to the binding, not the driver. Specific
> bindings can easily be bound to generic drivers.

Ok, I'm afraid I caused this. What should the compatible be, then?

									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 v3 1/2] dt-bindings: leds: Add binding for spi-byte LED.

[Rob Herring]

On Mon, May 6, 2019 at 11:28 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > +* 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.
> >
> > What about power control of the uC?
> >
> > > +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.
> >
> > This part seems in no way generic.
> >
> > How does one support the 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".
> >
> > Generally, we don't do "generic" bindings like this. The exceptions
> > are either we have confidence they really can be generic or they where
> > created before we knew better. A sample size of 1 doesn't convince me
> > the former is true.
> >
> > This comment *only* applies to the binding, not the driver. Specific
> > bindings can easily be bound to generic drivers.
>
> Ok, I'm afraid I caused this. What should the compatible be, then?

Knowing nothing about the h/w other than the above description:
ubiquiti,aircube-leds

Not sure if that's a registered or correct vendor prefix though.

Rob

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

[Christian Mauderer]

On 06/05/2019 19:44, Rob Herring wrote:
> On Mon, May 6, 2019 at 11:28 AM Pavel Machek <pavel@ucw.cz> wrote:
>>
>> Hi!
>>
>>>> +* 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.
>>>
>>> What about power control of the uC?
>>>
>>>> +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.
>>>
>>> This part seems in no way generic.
>>>
>>> How does one support the 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".
>>>
>>> Generally, we don't do "generic" bindings like this. The exceptions
>>> are either we have confidence they really can be generic or they where
>>> created before we knew better. A sample size of 1 doesn't convince me
>>> the former is true.
>>>
>>> This comment *only* applies to the binding, not the driver. Specific
>>> bindings can easily be bound to generic drivers.
>>
>> Ok, I'm afraid I caused this. What should the compatible be, then?
> 
> Knowing nothing about the h/w other than the above description:
> ubiquiti,aircube-leds
> 
> Not sure if that's a registered or correct vendor prefix though.
> 
> Rob
> 

Where would such a vendor prefix be registered? Does that mean that only
the vendor is allowed to use it? In that case: How would a reverse
engineered prefix look like?

Regarding the vendor: I wanted to have support for the airCube in
OpenWRT. When I started working with the device I asked Ubiquiti for the
sources (they use a OpenWRT based system). I received more or less the
answer that they don't have an archive but of course would create one.
But they have no ETA for it. The hardware is quite similar to other
boards so after some weeks I just analysed all necessary and did the
port myself instead of fighting with the support. So something like
three months after my question to the support, I finished the patches
and they had been accepted to OpenWRT. Ubiquiti released a GPL archive
(still with some missing parts like U-Boot) about two weeks later. I had
a look at it and they are not using a device tree. So there is no
"official" string that I could deduce from that archive.

Best regards

Christian

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

[Pavel Machek]

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

Hi!

> >> Ok, I'm afraid I caused this. What should the compatible be, then?
> > 
> > Knowing nothing about the h/w other than the above description:
> > ubiquiti,aircube-leds
> > 
> > Not sure if that's a registered or correct vendor prefix though.
> > 
> > Rob
> > 
> 
> Where would such a vendor prefix be registered? Does that mean that only
> the vendor is allowed to use it? In that case: How would a reverse
> engineered prefix look like?

You can use it, too. It is in
Documentation/devicetree/bindings/vendor-prefixes.txt :

ubnt    Ubiquiti Networks

So you can probably use ubnt, prefix.

> (still with some missing parts like U-Boot) about two weeks later. I had
> a look at it and they are not using a device tree. So there is no
> "official" string that I could deduce from that archive.

Mainline is the master. You are more "official" than them ;-).
									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 v3 1/2] dt-bindings: leds: Add binding for spi-byte LED.

[Christian Mauderer]

On 06/05/2019 22:25, Pavel Machek wrote:
> Hi!
> 
>>>> Ok, I'm afraid I caused this. What should the compatible be, then?
>>>
>>> Knowing nothing about the h/w other than the above description:
>>> ubiquiti,aircube-leds
>>>
>>> Not sure if that's a registered or correct vendor prefix though.
>>>
>>> Rob
>>>
>>
>> Where would such a vendor prefix be registered? Does that mean that only
>> the vendor is allowed to use it? In that case: How would a reverse
>> engineered prefix look like?
> 
> You can use it, too. It is in
> Documentation/devicetree/bindings/vendor-prefixes.txt :
> 
> ubnt    Ubiquiti Networks
> 
> So you can probably use ubnt, prefix.
> 
>> (still with some missing parts like U-Boot) about two weeks later. I had
>> a look at it and they are not using a device tree. So there is no
>> "official" string that I could deduce from that archive.
> 
> Mainline is the master. You are more "official" than them ;-).
> 									Pavel
> 

Hello

let me summarize the direction before I create a v4:

Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his
Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that.

With the more specific name I'll remove the off-value and max-value from
the device tree. Instead I'll create some look up table in the driver.
based on the name or go back to the defines like in the v1 patch. What
kind of solution would be preferable depends on the next question:

How should I name the driver? Should I use a device specific name like
in v1 again (most likely now acb-spi-led)? That would allow to
potentially add a hardware supported blinking in that driver. The
alternative would be the more generic name that it has now
(leds-spi-byte) without any plans to add the blinking but it could be
potentially used for example for a digital potentiometer based
brightness setting.

Note that I didn't really had planned to implement the blinking support
because I don't have a use case for it. So it would be either a feature
that I would add because someone insists. Or it could be added in the
future by a user who wants that feature (maybe Ubiquiti when they
upgrade their kernel?).

If it is a required feature for that driver: Please note that although
of course I would do some basic tests during development it would be a
mostly unused and therefore untested feature.

Best regards

Christian

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

[Christian Mauderer]

On 07/05/2019 11:52, Christian Mauderer wrote:
> On 06/05/2019 22:25, Pavel Machek wrote:
>> Hi!
>>
>>>>> Ok, I'm afraid I caused this. What should the compatible be, then?
>>>>
>>>> Knowing nothing about the h/w other than the above description:
>>>> ubiquiti,aircube-leds
>>>>
>>>> Not sure if that's a registered or correct vendor prefix though.
>>>>
>>>> Rob
>>>>
>>>
>>> Where would such a vendor prefix be registered? Does that mean that only
>>> the vendor is allowed to use it? In that case: How would a reverse
>>> engineered prefix look like?
>>
>> You can use it, too. It is in
>> Documentation/devicetree/bindings/vendor-prefixes.txt :
>>
>> ubnt    Ubiquiti Networks
>>
>> So you can probably use ubnt, prefix.
>>
>>> (still with some missing parts like U-Boot) about two weeks later. I had
>>> a look at it and they are not using a device tree. So there is no
>>> "official" string that I could deduce from that archive.
>>
>> Mainline is the master. You are more "official" than them ;-).
>> 									Pavel
>>
> 
> Hello
> 
> let me summarize the direction before I create a v4:
> 
> Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his
> Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that.
> 
> With the more specific name I'll remove the off-value and max-value from
> the device tree. Instead I'll create some look up table in the driver.
> based on the name or go back to the defines like in the v1 patch. What
> kind of solution would be preferable depends on the next question:
> 
> How should I name the driver? Should I use a device specific name like
> in v1 again (most likely now acb-spi-led)? That would allow to
> potentially add a hardware supported blinking in that driver. The
> alternative would be the more generic name that it has now
> (leds-spi-byte) without any plans to add the blinking but it could be
> potentially used for example for a digital potentiometer based
> brightness setting.
> 
> Note that I didn't really had planned to implement the blinking support
> because I don't have a use case for it. So it would be either a feature
> that I would add because someone insists. Or it could be added in the
> future by a user who wants that feature (maybe Ubiquiti when they
> upgrade their kernel?).
> 
> If it is a required feature for that driver: Please note that although
> of course I would do some basic tests during development it would be a
> mostly unused and therefore untested feature.
> 
> Best regards
> 
> Christian
> 

Hello,

sorry for repeating my question. I assume I wrote to much text hiding
it: How should I name the driver?

The name for the binding is clear (ubnt,acb-spi-led). Only the driver is
left (keep leds-spi-byte or rename to leds-ubnt-acb-spi or something else).

Best regards

Christian

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

[Jacek Anaszewski]

Hi Christian,

On 5/10/19 9:50 PM, Christian Mauderer wrote:
> On 07/05/2019 11:52, Christian Mauderer wrote:
>> On 06/05/2019 22:25, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> Ok, I'm afraid I caused this. What should the compatible be, then?
>>>>>
>>>>> Knowing nothing about the h/w other than the above description:
>>>>> ubiquiti,aircube-leds
>>>>>
>>>>> Not sure if that's a registered or correct vendor prefix though.
>>>>>
>>>>> Rob
>>>>>
>>>>
>>>> Where would such a vendor prefix be registered? Does that mean that only
>>>> the vendor is allowed to use it? In that case: How would a reverse
>>>> engineered prefix look like?
>>>
>>> You can use it, too. It is in
>>> Documentation/devicetree/bindings/vendor-prefixes.txt :
>>>
>>> ubnt    Ubiquiti Networks
>>>
>>> So you can probably use ubnt, prefix.
>>>
>>>> (still with some missing parts like U-Boot) about two weeks later. I had
>>>> a look at it and they are not using a device tree. So there is no
>>>> "official" string that I could deduce from that archive.
>>>
>>> Mainline is the master. You are more "official" than them ;-).
>>> 									Pavel
>>>
>>
>> Hello
>>
>> let me summarize the direction before I create a v4:
>>
>> Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his
>> Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that.
>>
>> With the more specific name I'll remove the off-value and max-value from
>> the device tree. Instead I'll create some look up table in the driver.
>> based on the name or go back to the defines like in the v1 patch. What
>> kind of solution would be preferable depends on the next question:
>>
>> How should I name the driver? Should I use a device specific name like
>> in v1 again (most likely now acb-spi-led)? That would allow to
>> potentially add a hardware supported blinking in that driver. The
>> alternative would be the more generic name that it has now
>> (leds-spi-byte) without any plans to add the blinking but it could be
>> potentially used for example for a digital potentiometer based
>> brightness setting.
>>
>> Note that I didn't really had planned to implement the blinking support
>> because I don't have a use case for it. So it would be either a feature
>> that I would add because someone insists. Or it could be added in the
>> future by a user who wants that feature (maybe Ubiquiti when they
>> upgrade their kernel?).
>>
>> If it is a required feature for that driver: Please note that although
>> of course I would do some basic tests during development it would be a
>> mostly unused and therefore untested feature.
>>
>> Best regards
>>
>> Christian
>>
> 
> Hello,
> 
> sorry for repeating my question. I assume I wrote to much text hiding
> it: How should I name the driver?
> 
> The name for the binding is clear (ubnt,acb-spi-led). Only the driver is
> left (keep leds-spi-byte or rename to leds-ubnt-acb-spi or something else).

Why leds-spi-byte name would prevent addition of blink support? It can
be always added basing on OF compatible. If it is to be generic SPI
byte driver, then I'd use leds-spi-byte. Actually also the things
like allowed brightness levels could be determined basing on that,
and not in device tree, but in the driver.

Please compare how e.g. drivers/leds/leds-is31fl32xx.c accomplishes
that.

-- 
Best regards,
Jacek Anaszewski

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

[Christian Mauderer]

Hello Jacek,

On 10/05/2019 22:42, Jacek Anaszewski wrote:
> Hi Christian,
> 
> On 5/10/19 9:50 PM, Christian Mauderer wrote:
>> On 07/05/2019 11:52, Christian Mauderer wrote:
>>> On 06/05/2019 22:25, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>>> Ok, I'm afraid I caused this. What should the compatible be, then?
>>>>>>
>>>>>> Knowing nothing about the h/w other than the above description:
>>>>>> ubiquiti,aircube-leds
>>>>>>
>>>>>> Not sure if that's a registered or correct vendor prefix though.
>>>>>>
>>>>>> Rob
>>>>>>
>>>>>
>>>>> Where would such a vendor prefix be registered? Does that mean that
>>>>> only
>>>>> the vendor is allowed to use it? In that case: How would a reverse
>>>>> engineered prefix look like?
>>>>
>>>> You can use it, too. It is in
>>>> Documentation/devicetree/bindings/vendor-prefixes.txt :
>>>>
>>>> ubnt��� Ubiquiti Networks
>>>>
>>>> So you can probably use ubnt, prefix.
>>>>
>>>>> (still with some missing parts like U-Boot) about two weeks later.
>>>>> I had
>>>>> a look at it and they are not using a device tree. So there is no
>>>>> "official" string that I could deduce from that archive.
>>>>
>>>> Mainline is the master. You are more "official" than them ;-).
>>>> ����������������������������������� Pavel
>>>>
>>>
>>> Hello
>>>
>>> let me summarize the direction before I create a v4:
>>>
>>> Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his
>>> Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that.
>>>
>>> With the more specific name I'll remove the off-value and max-value from
>>> the device tree. Instead I'll create some look up table in the driver.
>>> based on the name or go back to the defines like in the v1 patch. What
>>> kind of solution would be preferable depends on the next question:
>>>
>>> How should I name the driver? Should I use a device specific name like
>>> in v1 again (most likely now acb-spi-led)? That would allow to
>>> potentially add a hardware supported blinking in that driver. The
>>> alternative would be the more generic name that it has now
>>> (leds-spi-byte) without any plans to add the blinking but it could be
>>> potentially used for example for a digital potentiometer based
>>> brightness setting.
>>>
>>> Note that I didn't really had planned to implement the blinking support
>>> because I don't have a use case for it. So it would be either a feature
>>> that I would add because someone insists. Or it could be added in the
>>> future by a user who wants that feature (maybe Ubiquiti when they
>>> upgrade their kernel?).
>>>
>>> If it is a required feature for that driver: Please note that although
>>> of course I would do some basic tests during development it would be a
>>> mostly unused and therefore untested feature.
>>>
>>> Best regards
>>>
>>> Christian
>>>
>>
>> Hello,
>>
>> sorry for repeating my question. I assume I wrote to much text hiding
>> it: How should I name the driver?
>>
>> The name for the binding is clear (ubnt,acb-spi-led). Only the driver is
>> left (keep leds-spi-byte or rename to leds-ubnt-acb-spi or something
>> else).
> 
> Why leds-spi-byte name would prevent addition of blink support? It can
> be always added basing on OF compatible. If it is to be generic SPI
> byte driver, then I'd use leds-spi-byte. Actually also the things
> like allowed brightness levels could be determined basing on that,
> and not in device tree, but in the driver.
> 
> Please compare how e.g. drivers/leds/leds-is31fl32xx.c accomplishes
> that.
> 

I would have expected that adding a lot of device specific code (in that
case blinking) to a multi-purpose driver would be bad style. But I'll go
for the generic name if that is the accepted way. I already mentioned
multiple times that my target is currently only the brightness. So the
device specific code maybe is added quite a bit in the future anyway in
which case it would still be possible to rename a part (if it isn't used
otherwise) or at least split it into it's own c-file.

I'll prepare a v4 in the near future and send it to the list. I only
learned that it would be a good idea to wait for at least a day for some
other opinions before doing that ;-)

Best regards

Christian

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

[Jacek Anaszewski]

On 5/11/19 8:56 AM, Christian Mauderer wrote:
> Hello Jacek,
> 
> On 10/05/2019 22:42, Jacek Anaszewski wrote:
>> Hi Christian,
>>
>> On 5/10/19 9:50 PM, Christian Mauderer wrote:
>>> On 07/05/2019 11:52, Christian Mauderer wrote:
>>>> On 06/05/2019 22:25, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>>> Ok, I'm afraid I caused this. What should the compatible be, then?
>>>>>>>
>>>>>>> Knowing nothing about the h/w other than the above description:
>>>>>>> ubiquiti,aircube-leds
>>>>>>>
>>>>>>> Not sure if that's a registered or correct vendor prefix though.
>>>>>>>
>>>>>>> Rob
>>>>>>>
>>>>>>
>>>>>> Where would such a vendor prefix be registered? Does that mean that
>>>>>> only
>>>>>> the vendor is allowed to use it? In that case: How would a reverse
>>>>>> engineered prefix look like?
>>>>>
>>>>> You can use it, too. It is in
>>>>> Documentation/devicetree/bindings/vendor-prefixes.txt :
>>>>>
>>>>> ubnt��� Ubiquiti Networks
>>>>>
>>>>> So you can probably use ubnt, prefix.
>>>>>
>>>>>> (still with some missing parts like U-Boot) about two weeks later.
>>>>>> I had
>>>>>> a look at it and they are not using a device tree. So there is no
>>>>>> "official" string that I could deduce from that archive.
>>>>>
>>>>> Mainline is the master. You are more "official" than them ;-).
>>>>>  ����������������������������������� Pavel
>>>>>
>>>>
>>>> Hello
>>>>
>>>> let me summarize the direction before I create a v4:
>>>>
>>>> Rob Herring suggested "ubnt,acb-spi-led" for the binding name in his
>>>> Mail from 06.05.2019 17:59 UTC. If no one objects, I'll use that.
>>>>
>>>> With the more specific name I'll remove the off-value and max-value from
>>>> the device tree. Instead I'll create some look up table in the driver.
>>>> based on the name or go back to the defines like in the v1 patch. What
>>>> kind of solution would be preferable depends on the next question:
>>>>
>>>> How should I name the driver? Should I use a device specific name like
>>>> in v1 again (most likely now acb-spi-led)? That would allow to
>>>> potentially add a hardware supported blinking in that driver. The
>>>> alternative would be the more generic name that it has now
>>>> (leds-spi-byte) without any plans to add the blinking but it could be
>>>> potentially used for example for a digital potentiometer based
>>>> brightness setting.
>>>>
>>>> Note that I didn't really had planned to implement the blinking support
>>>> because I don't have a use case for it. So it would be either a feature
>>>> that I would add because someone insists. Or it could be added in the
>>>> future by a user who wants that feature (maybe Ubiquiti when they
>>>> upgrade their kernel?).
>>>>
>>>> If it is a required feature for that driver: Please note that although
>>>> of course I would do some basic tests during development it would be a
>>>> mostly unused and therefore untested feature.
>>>>
>>>> Best regards
>>>>
>>>> Christian
>>>>
>>>
>>> Hello,
>>>
>>> sorry for repeating my question. I assume I wrote to much text hiding
>>> it: How should I name the driver?
>>>
>>> The name for the binding is clear (ubnt,acb-spi-led). Only the driver is
>>> left (keep leds-spi-byte or rename to leds-ubnt-acb-spi or something
>>> else).
>>
>> Why leds-spi-byte name would prevent addition of blink support? It can
>> be always added basing on OF compatible. If it is to be generic SPI
>> byte driver, then I'd use leds-spi-byte. Actually also the things
>> like allowed brightness levels could be determined basing on that,
>> and not in device tree, but in the driver.
>>
>> Please compare how e.g. drivers/leds/leds-is31fl32xx.c accomplishes
>> that.
>>
> 
> I would have expected that adding a lot of device specific code (in that
> case blinking) to a multi-purpose driver would be bad style.

This is a matter of how much customizable the driver is going to be.
It can serve as a nice code base for other devices with simple
SPI-based protocol.

> But I'll go
> for the generic name if that is the accepted way. I already mentioned
> multiple times that my target is currently only the brightness. So the
> device specific code maybe is added quite a bit in the future anyway in
> which case it would still be possible to rename a part (if it isn't used
> otherwise) or at least split it into it's own c-file.
> 
> I'll prepare a v4 in the near future and send it to the list. I only
> learned that it would be a good idea to wait for at least a day for some
> other opinions before doing that ;-)

Sure thing. The fact that I'm replying to your message doesn't mean
I'm expecting your immediate reaction :-)

-- 
Best regards,
Jacek Anaszewski

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

[Christian Mauderer]

On 06/05/2019 18:21, Rob Herring wrote:
> On Sun, May 5, 2019 at 3:00 PM <oss@c-mauderer.de> wrote:
>>
>> 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 v2:
>> - None
>>
>> 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.
> 
> What about power control of the uC?

You mean if the uC receives a reset or power cycle independent of the
main controller? I don't think that this can happen on that board. But I
don't have any schematics to prove that.

> 
>> +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.
> 
> This part seems in no way generic.
> 
> How does one support the blink patterns?

You are correct that this part is not generic. But a multi-purpose
driver like the one I proposed could deliver a basic support for the
device by controlling the brightness.

It's only a basic support so the blink patterns are not supported.

I had a look at the functions in "struct led_classdev". There is a
blink_set(..) function that expects that delay_on and delay_off can be
set independent. That's not possible for hardware supported blinking on
this device. The other function pattern_set(..) would allow an even more
universal interface. All possible patterns of the LED could be covered
in that but I don't think that this is true the other way round.

So in my opinion the only thing that can be implemented in a useful way
for that controller is the brightness.

> 
>> +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".
> 
> Generally, we don't do "generic" bindings like this. The exceptions
> are either we have confidence they really can be generic or they where
> created before we knew better. A sample size of 1 doesn't convince me
> the former is true.

I could construct another sample (some SPI-based digital potentiometer
where you set values between 17 and 213) but I doubt that it would be a
good idea to fight for the name.

My original target device is a quite special one: I don't have a chip
number. The controller Ubiquiti built here is based on a microcontroller
that could be anything. The general device is named "Ubiquiti airCube
ISP" or (a short form that I found at some locations) ubnt-acb-isp. I
assume that they used the same controller in the non-ISP-version but I
haven't checked that. So how about one of these:

- ubnt,spi-byte-led
- ubnt,spi-acb-led
- ubnt,acb-isp-led

Most likely I'll get the off-value and max-value based on the binding
name then (0 and 63 for that device). So I'll just remove the two
parameters then.

> 
> This comment *only* applies to the binding, not the driver. Specific
> bindings can easily be bound to generic drivers.
> 

So the driver should still be called spi-byte (or something similar)?

>> +
>> +Property rules described in Documentation/devicetree/bindings/spi/spi-bus.txt
>> +apply. In particular, "reg" and "spi-max-frequency" properties must be given.
> 
> What's the SPI mode configuration?

For this controller the mode seems to be CPOL = 0, CPHA = 0 (clock low
when idle, data valid on rising edge). CS is active low. The values are
MSB first (otherwise it would be an odd counter). Note that this is
based on reverse engineering (just like the protocol). So it's just a
justified guess.

If you are really interested in a lot of details, you can have a look at
the analysis that I did on that controller here:

https://github.com/c-mauderer/openwrt-Ubiquity-Aircube-notes/blob/5eb66d274db32238fc3249748be3a0eb26d1c91b/notes/Notes.asciidoc#led-controller

> 
>> +
>> +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.
> 
> Can't we already express this with brightness-levels and
> num-interpolated-steps properties? Some reason those ended up in
> pwm-backlight.txt, but really could apply to any LED with level
> controls.

The parameters gave the minimum and maximum SPI byte that should be
sent. But like said above: If the binding is based on the device name,
these parameters can just be a look up table or something similar based
on the binding name in the driver. So I can remove them.

> 
> Rob
> 

Best regards

Christian

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

[Rob Herring]

On Mon, May 6, 2019 at 12:03 PM Christian Mauderer <oss@c-mauderer.de> wrote:
>
> On 06/05/2019 18:21, Rob Herring wrote:
> > On Sun, May 5, 2019 at 3:00 PM <oss@c-mauderer.de> wrote:
> >>
> >> 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 v2:
> >> - None
> >>
> >> 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.
> >
> > What about power control of the uC?
>
> You mean if the uC receives a reset or power cycle independent of the
> main controller? I don't think that this can happen on that board. But I
> don't have any schematics to prove that.

I was really only pointing out potential issues around "generic"
bindings. The protocol interface is not the only thing.

> >> +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.
> >
> > This part seems in no way generic.
> >
> > How does one support the blink patterns?
>
> You are correct that this part is not generic. But a multi-purpose
> driver like the one I proposed could deliver a basic support for the
> device by controlling the brightness.
>
> It's only a basic support so the blink patterns are not supported.

Then you would have to change/extend the binding when you want to
support that. With a specific compatible, you only need a driver
change.

> I had a look at the functions in "struct led_classdev". There is a
> blink_set(..) function that expects that delay_on and delay_off can be
> set independent. That's not possible for hardware supported blinking on
> this device. The other function pattern_set(..) would allow an even more
> universal interface. All possible patterns of the LED could be covered
> in that but I don't think that this is true the other way round.
>
> So in my opinion the only thing that can be implemented in a useful way
> for that controller is the brightness.

What the OS can support evolves and should be independent of the binding.

> >
> >> +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".
> >
> > Generally, we don't do "generic" bindings like this. The exceptions
> > are either we have confidence they really can be generic or they where
> > created before we knew better. A sample size of 1 doesn't convince me
> > the former is true.
>
> I could construct another sample (some SPI-based digital potentiometer
> where you set values between 17 and 213) but I doubt that it would be a
> good idea to fight for the name.
>
> My original target device is a quite special one: I don't have a chip
> number. The controller Ubiquiti built here is based on a microcontroller
> that could be anything. The general device is named "Ubiquiti airCube
> ISP" or (a short form that I found at some locations) ubnt-acb-isp. I
> assume that they used the same controller in the non-ISP-version but I
> haven't checked that. So how about one of these:
>
> - ubnt,spi-byte-led
> - ubnt,spi-acb-led
> - ubnt,acb-isp-led

ubnt,acb-spi-led perhaps as the order is usually <product|soc>-<sub device>.

Or the last one if you wanted to keep 'isp'.

>
> Most likely I'll get the off-value and max-value based on the binding
> name then (0 and 63 for that device). So I'll just remove the two
> parameters then.
>
> >
> > This comment *only* applies to the binding, not the driver. Specific
> > bindings can easily be bound to generic drivers.
> >
>
> So the driver should still be called spi-byte (or something similar)?

Yes, whatever the LED maintainers want there.

Rob

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

[Christian Mauderer]

On 06/05/2019 19:59, Rob Herring wrote:
> On Mon, May 6, 2019 at 12:03 PM Christian Mauderer <oss@c-mauderer.de> wrote:
>>
>> On 06/05/2019 18:21, Rob Herring wrote:
>>> On Sun, May 5, 2019 at 3:00 PM <oss@c-mauderer.de> wrote:
>>>>
>>>> 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 v2:
>>>> - None
>>>>
>>>> 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.
>>>
>>> What about power control of the uC?
>>
>> You mean if the uC receives a reset or power cycle independent of the
>> main controller? I don't think that this can happen on that board. But I
>> don't have any schematics to prove that.
> 
> I was really only pointing out potential issues around "generic"
> bindings. The protocol interface is not the only thing.

OK.

> 
>>>> +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.
>>>
>>> This part seems in no way generic.
>>>
>>> How does one support the blink patterns?
>>
>> You are correct that this part is not generic. But a multi-purpose
>> driver like the one I proposed could deliver a basic support for the
>> device by controlling the brightness.
>>
>> It's only a basic support so the blink patterns are not supported.
> 
> Then you would have to change/extend the binding when you want to
> support that. With a specific compatible, you only need a driver
> change.

Accepted.

> 
>> I had a look at the functions in "struct led_classdev". There is a
>> blink_set(..) function that expects that delay_on and delay_off can be
>> set independent. That's not possible for hardware supported blinking on
>> this device. The other function pattern_set(..) would allow an even more
>> universal interface. All possible patterns of the LED could be covered
>> in that but I don't think that this is true the other way round.
>>
>> So in my opinion the only thing that can be implemented in a useful way
>> for that controller is the brightness.
> 
> What the OS can support evolves and should be independent of the binding.
> 

Yes OK. I maybe have seen the binding too tightly coupled.

>>>
>>>> +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".
>>>
>>> Generally, we don't do "generic" bindings like this. The exceptions
>>> are either we have confidence they really can be generic or they where
>>> created before we knew better. A sample size of 1 doesn't convince me
>>> the former is true.
>>
>> I could construct another sample (some SPI-based digital potentiometer
>> where you set values between 17 and 213) but I doubt that it would be a
>> good idea to fight for the name.
>>
>> My original target device is a quite special one: I don't have a chip
>> number. The controller Ubiquiti built here is based on a microcontroller
>> that could be anything. The general device is named "Ubiquiti airCube
>> ISP" or (a short form that I found at some locations) ubnt-acb-isp. I
>> assume that they used the same controller in the non-ISP-version but I
>> haven't checked that. So how about one of these:
>>
>> - ubnt,spi-byte-led
>> - ubnt,spi-acb-led
>> - ubnt,acb-isp-led
> 
> ubnt,acb-spi-led perhaps as the order is usually <product|soc>-<sub device>.
> 
> Or the last one if you wanted to keep 'isp'.
> 

In your other mail you said that maybe a vendor prefix is registered.
See my questions regarding that there.

>>
>> Most likely I'll get the off-value and max-value based on the binding
>> name then (0 and 63 for that device). So I'll just remove the two
>> parameters then.
>>
>>>
>>> This comment *only* applies to the binding, not the driver. Specific
>>> bindings can easily be bound to generic drivers.
>>>
>>
>> So the driver should still be called spi-byte (or something similar)?
> 
> Yes, whatever the LED maintainers want there.

OK. I'll discuss that with them as soon as the binding is clear.

> 
> Rob
> 

Best regards

Christian

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

[Jacek Anaszewski]

Hi Christian,

On 5/6/19 7:03 PM, Christian Mauderer wrote:
> On 06/05/2019 18:21, Rob Herring wrote:
>> On Sun, May 5, 2019 at 3:00 PM <oss@c-mauderer.de> wrote:
[...]
> 
> I had a look at the functions in "struct led_classdev". There is a
> blink_set(..) function that expects that delay_on and delay_off can be
> set independent. That's not possible for hardware supported blinking on
> this device. 

If delay_on and delay_off values are not supported by the hardware,
then blink_set should return -EINVAL, which will result in activating
software blink fallback for given LED class device.

-- 
Best regards,
Jacek Anaszewski