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

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

[list]

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

This patch adds the binding documentation for the LED controller found
in Ubiquity airCube ISP devices.

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

I tested the patches with a 4.14 and a 4.19 kernel on the current OpenWRT.
Although I didn't get the kernel running due to file system problems they build
fine with a 5.1-rc7.

I shortly described the protocol of the controller in a comment in the driver
file in the second patch.

Checkpatch gives the following warning for both patches:

  WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?

To be honest: I don't know what to do with it. Please excuse my ignorance here.
It's the first driver that I want to add to the Linux kernel.

Please point me to some documentation if I did miss some big points for
submitting patches.


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

diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
new file mode 100644
index 000000000000..ab1478cdc139
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
@@ -0,0 +1,49 @@
+Binding for the controller based LED found in Ubiquity airCube ISP and most
+likely some other Ubiquity devices.
+
+The protocol of the controller is quite simple. Only one byte will be sent. The
+value of the byte can be between the ubnt-spi,off_bright value and the
+ubnt-spi,max_bright value.
+
+The driver maybe can be used for other devices with a similar protocol too.
+
+Required properties:
+- compatible:		Should be "ubnt,spi-led".
+- spi-max-frequency:	Should be <100000> for this device.
+
+Optional sub-node properties:
+- ubnt-spi,off_bright:	The value that will be sent if the LED should be
+			switched off. Default value is 0.
+- ubnt-spi,max_bright:	Value for the maximum brightness. Default value for that
+			is 63.
+- label:		A label for the LED. If one is given, the LED will be
+			named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
+
+Being a SPI device this driver should be a sub-node of a SPI controller. The
+controller only supports one LED. For consistence with other controllers, the
+LED is defined as a sub-node.
+
+Example for the airCube ISP (with SPI controller matching that device):
+
+led_spi {
+	compatible = "spi-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
+	gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
+	cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+	num-chipselects = <1>;
+
+	led_ubnt@0 {
+		compatible = "ubnt,spi-led";
+		reg = <0>;
+		spi-max-frequency = <100000>;
+
+		led {
+			label = "system";
+			/* keep the LED slightly on to show powered device */
+			ubnt-spi,off_bright = /bits/ 8 <4>;
+		};
+	};
+};
-- 
2.21.0

[PATCH 2/2] leds: ubnt-spi: Add Ubnt AirCube ISP LED driver

[list]

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

This driver adds support for the custom LED controller used in Ubiquity
airCube ISP devices and most likely some other simmilar Ubiquity
products.

Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
---
 drivers/leds/Kconfig         |  10 +++
 drivers/leds/Makefile        |   1 +
 drivers/leds/leds-ubnt-spi.c | 140 +++++++++++++++++++++++++++++++++++
 3 files changed, 151 insertions(+)
 create mode 100644 drivers/leds/leds-ubnt-spi.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index a72f97fca57b..e96ab93860ec 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -766,6 +766,16 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_UBNT_SPI
+	tristate "LED support for Ubnt aircube ISP custom SPI LED controller"
+	depends on LEDS_CLASS
+	depends on SPI
+	depends on OF
+	help
+	  This option enables basic support for the LED controller used in
+	  Ubiquity airCube ISP devices. The controller is microcontroller based
+	  and uses a single byte on the SPI to set brightness or blink patterns.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 4c1b0054f379..f272bfac109c 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_UBNT_SPI)		+= leds-ubnt-spi.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-ubnt-spi.c b/drivers/leds/leds-ubnt-spi.c
new file mode 100644
index 000000000000..a3cb0dd5b0da
--- /dev/null
+++ b/drivers/leds/leds-ubnt-spi.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2019 Christian Mauderer <oss@c-mauderer.de>
+
+/*
+ *  Custom controller based LED controller used in Ubiquity airCube ISP.
+ *
+ *  Reverse engineered protocol:
+ *  - The device uses only a single byte sent via SPI.
+ *  - The SPI input doesn't contain any relevant information.
+ *  - Higher two bits set a mode. Lower six bits are a parameter.
+ *  - Mode: 00 -> set brightness between 0x00 (min) and 0x3F (max)
+ *  - Mode: 01 -> pulsing pattern (min -> max -> min) with an interval. From
+ *    some tests, the period is about (50ms + 102ms * parameter). There is a
+ *    slightly different pattern starting from 0x100 (longer gap between the
+ *    pulses) but the time still follows that calculation.
+ *  - Mode: 10 -> same as 01 but with only a ramp from min to max. Again a
+ *    slight jump in the pattern at 0x100.
+ *  - Mode: 11 -> blinking (off -> 25% -> off -> 25% -> ...) with a period of
+ *    (105ms * parameter)
+ *
+ *  NOTE: This driver currently only implements mode 00 (brightness).
+ */
+
+#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>
+
+#define UBNT_SPI_OFF_BRIGHTNESS	0x0
+#define UBNT_SPI_MAX_BRIGHTNESS	0x3F
+
+struct ubnt_spi_led {
+	struct led_classdev	ldev;
+	struct spi_device	*spi;
+	char			name[LED_MAX_NAME_SIZE];
+	struct mutex		mutex;
+	u8			off_bright;
+	u8			max_bright;
+};
+
+static int ubnt_spi_brightness_set_blocking(struct led_classdev *dev,
+					    enum led_brightness brightness)
+{
+	struct ubnt_spi_led *led = container_of(dev, struct ubnt_spi_led, ldev);
+	u8 value;
+	int ret;
+
+	value = (u8) brightness + led->off_bright;
+
+	mutex_lock(&led->mutex);
+	ret = spi_write(led->spi, &value, sizeof(value));
+	mutex_unlock(&led->mutex);
+
+	return ret;
+}
+
+static int ubnt_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct device_node *child;
+	struct ubnt_spi_led	*led;
+	int ret;
+	const char *tmp;
+	const char *default_name = ":";
+
+	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);
+
+	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->spi	= spi;
+
+	ret = of_property_read_string(child, "label", &tmp);
+	if (ret < 0)
+		tmp = default_name;
+	snprintf(led->name, sizeof(led->name), "ubnt-spi-led:%s", tmp);
+
+	ret = of_property_read_u8(child, "ubnt-spi,off_bright",
+				  &led->off_bright);
+	if (ret != 0)
+		led->off_bright = UBNT_SPI_OFF_BRIGHTNESS;
+
+	ret = of_property_read_u8(child, "ubnt-spi,max_bright",
+				  &led->max_bright);
+	if (ret != 0)
+		led->max_bright = UBNT_SPI_MAX_BRIGHTNESS;
+
+	mutex_init(&led->mutex);
+	led->ldev.name = led->name;
+	led->ldev.brightness = LED_OFF;
+	led->ldev.max_brightness = led->max_bright - led->off_bright;
+	led->ldev.brightness_set_blocking = ubnt_spi_brightness_set_blocking;
+	ret = led_classdev_register(&spi->dev, &led->ldev);
+	if (ret >= 0)
+		spi_set_drvdata(spi, led);
+
+	return ret;
+}
+
+static int ubnt_spi_remove(struct spi_device *spi)
+{
+	struct ubnt_spi_led	*led = spi_get_drvdata(spi);
+
+	led_classdev_unregister(&led->ldev);
+
+	return 0;
+}
+
+static const struct of_device_id ubnt_spi_dt_ids[] = {
+	{ .compatible = "ubnt,spi-led", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, ubnt_spi_dt_ids);
+
+static struct spi_driver ubnt_spi_driver = {
+	.probe		= ubnt_spi_probe,
+	.remove		= ubnt_spi_remove,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= ubnt_spi_dt_ids,
+	},
+};
+
+module_spi_driver(ubnt_spi_driver);
+
+MODULE_AUTHOR("Christian Mauderer <oss@c-mauderer.de>");
+MODULE_DESCRIPTION("Ubnt AirCube ISP LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:ubnt-spi-led");
-- 
2.21.0

Re: [PATCH 2/2] leds: ubnt-spi: Add Ubnt AirCube ISP LED driver

[Pavel Machek]

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

On Sat 2019-05-04 14:28:25, list@c-mauderer.de wrote:
> From: Christian Mauderer <oss@c-mauderer.de>
> 
> This driver adds support for the custom LED controller used in Ubiquity
> airCube ISP devices and most likely some other simmilar Ubiquity
> products.

similar.

> +config LEDS_UBNT_SPI
> +	tristate "LED support for Ubnt aircube ISP custom SPI LED controller"
> +	depends on LEDS_CLASS
> +	depends on SPI
> +	depends on OF
> +	help
> +	  This option enables basic support for the LED controller used in
> +	  Ubiquity airCube ISP devices. The controller is microcontroller based
> +	  and uses a single byte on the SPI to set brightness or blink patterns.

> +/*
> + *  Custom controller based LED controller used in Ubiquity airCube ISP.
> + *
> + *  Reverse engineered protocol:
> + *  - The device uses only a single byte sent via SPI.
> + *  - The SPI input doesn't contain any relevant information.
> + *  - Higher two bits set a mode. Lower six bits are a parameter.
> + *  - Mode: 00 -> set brightness between 0x00 (min) and 0x3F (max)
> + *  - Mode: 01 -> pulsing pattern (min -> max -> min) with an interval. From
> + *    some tests, the period is about (50ms + 102ms * parameter). There is a
> + *    slightly different pattern starting from 0x100 (longer gap between the
> + *    pulses) but the time still follows that calculation.
> + *  - Mode: 10 -> same as 01 but with only a ramp from min to max. Again a
> + *    slight jump in the pattern at 0x100.
> + *  - Mode: 11 -> blinking (off -> 25% -> off -> 25% -> ...) with a period of
> + *    (105ms * parameter)
> + *
> + *  NOTE: This driver currently only implements mode 00 (brightness).
> + */

Aha, so this is not as simple as I thought.

"Slightly different pattern starting from 0x100"? What does 0x100 mean
here.

> +	mutex_init(&led->mutex);
> +	led->ldev.name = led->name;
> +	led->ldev.brightness = LED_OFF;
> +	led->ldev.max_brightness = led->max_bright - led->off_bright;

What happens when DTS has off_bright > max_bright? :-).

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

Do you need to do mutex_destroy?
									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 2/2] leds: ubnt-spi: Add Ubnt AirCube ISP LED driver

[Christian Mauderer]

First: Thanks for the quick review. I haven't expected an answer for
some days.

On 04/05/2019 18:20, Pavel Machek wrote:
> On Sat 2019-05-04 14:28:25, list@c-mauderer.de wrote:
>> From: Christian Mauderer <oss@c-mauderer.de>
>>
>> This driver adds support for the custom LED controller used in Ubiquity
>> airCube ISP devices and most likely some other simmilar Ubiquity
>> products.
> 
> similar.

I'll change that. And I noted another typo too: The company is called
Ubiquiti ...

> 
>> +config LEDS_UBNT_SPI
>> +	tristate "LED support for Ubnt aircube ISP custom SPI LED controller"
>> +	depends on LEDS_CLASS
>> +	depends on SPI
>> +	depends on OF
>> +	help
>> +	  This option enables basic support for the LED controller used in
>> +	  Ubiquity airCube ISP devices. The controller is microcontroller based
>> +	  and uses a single byte on the SPI to set brightness or blink patterns.
> 
>> +/*
>> + *  Custom controller based LED controller used in Ubiquity airCube ISP.
>> + *
>> + *  Reverse engineered protocol:
>> + *  - The device uses only a single byte sent via SPI.
>> + *  - The SPI input doesn't contain any relevant information.
>> + *  - Higher two bits set a mode. Lower six bits are a parameter.
>> + *  - Mode: 00 -> set brightness between 0x00 (min) and 0x3F (max)
>> + *  - Mode: 01 -> pulsing pattern (min -> max -> min) with an interval. From
>> + *    some tests, the period is about (50ms + 102ms * parameter). There is a
>> + *    slightly different pattern starting from 0x100 (longer gap between the
>> + *    pulses) but the time still follows that calculation.
>> + *  - Mode: 10 -> same as 01 but with only a ramp from min to max. Again a
>> + *    slight jump in the pattern at 0x100.
>> + *  - Mode: 11 -> blinking (off -> 25% -> off -> 25% -> ...) with a period of
>> + *    (105ms * parameter)
>> + *
>> + *  NOTE: This driver currently only implements mode 00 (brightness).
>> + */
> 
> Aha, so this is not as simple as I thought.

Yes, right. But maybe implementing a generic driver would be a good idea
anyway. I don't think that it is useful to support more than the
brightness in the near future. Currently the only application I know of
would be to port OpenWRT to the device. OpenWRT only switches the LED to
max or min.

If you don't object, I'll use the generic approach (including the name
change to led-spi-byte).

> 
> "Slightly different pattern starting from 0x100"? What does 0x100 mean
> here.

That should be 0x10 instead of 0x100. So for example the jump from 0x4f
to 0x50. But if I use the generic approach, that text will change anyway.

> 
>> +	mutex_init(&led->mutex);
>> +	led->ldev.name = led->name;
>> +	led->ldev.brightness = LED_OFF;
>> +	led->ldev.max_brightness = led->max_bright - led->off_bright;
> 
> What happens when DTS has off_bright > max_bright? :-).

That slipped me. I'll create a check for that.

> 
>> +
>> +static int ubnt_spi_remove(struct spi_device *spi)
>> +{
>> +	struct ubnt_spi_led	*led = spi_get_drvdata(spi);
>> +
>> +	led_classdev_unregister(&led->ldev);
>> +
>> +	return 0;
>> +}
> 
> Do you need to do mutex_destroy?

Yes. I'll add it.

> 									Pavel
> 

Re: [PATCH 2/2] leds: ubnt-spi: Add Ubnt AirCube ISP LED driver

[Pavel Machek]

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

Hi!

> First: Thanks for the quick review. I haven't expected an answer for
> some days.

Thanks for simple driver :-).

You may want to wait day or two before submitting v2, you may get some
other review comment.

Best regards,

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

[Pavel Machek]

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

On Sat 2019-05-04 14:28:24, list@c-mauderer.de wrote:
> From: Christian Mauderer <oss@c-mauderer.de>
> 
> This patch adds the binding documentation for the LED controller found
> in Ubiquity airCube ISP devices.
> 
> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
> ---
> 
> I tested the patches with a 4.14 and a 4.19 kernel on the current OpenWRT.
> Although I didn't get the kernel running due to file system problems they build
> fine with a 5.1-rc7.
> 
> I shortly described the protocol of the controller in a comment in the driver
> file in the second patch.
> 
> Checkpatch gives the following warning for both patches:
> 
>   WARNING: added, moved or deleted file(s), does MAINTAINERS need
> updating?

Ignore that :-).

> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
> new file mode 100644
> index 000000000000..ab1478cdc139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
> @@ -0,0 +1,49 @@
> +Binding for the controller based LED found in Ubiquity airCube ISP and most
> +likely some other Ubiquity devices.

It would be good to know what chip it is.. and name the binding
accordingly.

Alternatively, call its led-spi-byte, or something, as it is really
trivial protocol. Maybe other chips will have same interface?

> +Example for the airCube ISP (with SPI controller matching that device):
> +
> +led_spi {
> +	compatible = "spi-gpio";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
> +	gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +	cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
> +	num-chipselects = <1>;
> +
> +	led_ubnt@0 {
> +		compatible = "ubnt,spi-led";
> +		reg = <0>;
> +		spi-max-frequency = <100000>;
> +
> +		led {
> +			label = "system";
> +			/* keep the LED slightly on to show powered device */
> +			ubnt-spi,off_bright = /bits/ 8 <4>;
> +		};
> +	};
> +};

Otherwise looks good to me,

Acked-by: Pavel Machek <pavel@ucw.cz>

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

[Christian Mauderer]

On 04/05/2019 18:12, Pavel Machek wrote:
> On Sat 2019-05-04 14:28:24, list@c-mauderer.de wrote:
>> From: Christian Mauderer <oss@c-mauderer.de>
>>
>> This patch adds the binding documentation for the LED controller found
>> in Ubiquity airCube ISP devices.
>>
>> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
>> ---
>>
>> I tested the patches with a 4.14 and a 4.19 kernel on the current OpenWRT.
>> Although I didn't get the kernel running due to file system problems they build
>> fine with a 5.1-rc7.
>>
>> I shortly described the protocol of the controller in a comment in the driver
>> file in the second patch.
>>
>> Checkpatch gives the following warning for both patches:
>>
>>   WARNING: added, moved or deleted file(s), does MAINTAINERS need
>> updating?
> 
> Ignore that :-).

OK. Thanks.

> 
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>> new file mode 100644
>> index 000000000000..ab1478cdc139
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>> @@ -0,0 +1,49 @@
>> +Binding for the controller based LED found in Ubiquity airCube ISP and most
>> +likely some other Ubiquity devices.
> 
> It would be good to know what chip it is.. and name the binding
> accordingly.

The chip they use is a SONiX 8F26E611LA which is a generic 8-Bit
microcontroller. Ubiquiti seems to have programmed it to be a LED
controller. But it could do something completely different in another
hardware too. Therefore I didn't use the name.

> 
> Alternatively, call its led-spi-byte, or something, as it is really
> trivial protocol. Maybe other chips will have same interface?

See my other mail: I'll use the generic approach with that name.

> 
>> +Example for the airCube ISP (with SPI controller matching that device):
>> +
>> +led_spi {
>> +	compatible = "spi-gpio";
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
>> +	gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
>> +	cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
>> +	num-chipselects = <1>;
>> +
>> +	led_ubnt@0 {
>> +		compatible = "ubnt,spi-led";
>> +		reg = <0>;
>> +		spi-max-frequency = <100000>;
>> +
>> +		led {
>> +			label = "system";
>> +			/* keep the LED slightly on to show powered device */
>> +			ubnt-spi,off_bright = /bits/ 8 <4>;
>> +		};
>> +	};
>> +};
> 
> Otherwise looks good to me,
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 

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

[Jacek Anaszewski]

Hi Christian,

Thank you for the patch.

On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
> From: Christian Mauderer <oss@c-mauderer.de>
> 
> This patch adds the binding documentation for the LED controller found
> in Ubiquity airCube ISP devices.
> 
> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
> ---
> 
> I tested the patches with a 4.14 and a 4.19 kernel on the current OpenWRT.
> Although I didn't get the kernel running due to file system problems they build
> fine with a 5.1-rc7.
> 
> I shortly described the protocol of the controller in a comment in the driver
> file in the second patch.
> 
> Checkpatch gives the following warning for both patches:
> 
>    WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> 
> To be honest: I don't know what to do with it. Please excuse my ignorance here.
> It's the first driver that I want to add to the Linux kernel.

You can add yourself as a maintainer of this driver, but it is customary
rather for more complex drivers.

> Please point me to some documentation if I did miss some big points for
> submitting patches.
> 
> 
>   .../bindings/leds/leds-ubnt-spi.txt           | 49 +++++++++++++++++++
>   1 file changed, 49 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
> new file mode 100644
> index 000000000000..ab1478cdc139
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
> @@ -0,0 +1,49 @@
> +Binding for the controller based LED found in Ubiquity airCube ISP and most
> +likely some other Ubiquity devices.
> +
> +The protocol of the controller is quite simple. Only one byte will be sent. The
> +value of the byte can be between the ubnt-spi,off_bright value and the
> +ubnt-spi,max_bright value.
> +
> +The driver maybe can be used for other devices with a similar protocol too.
> +
> +Required properties:
> +- compatible:		Should be "ubnt,spi-led".
> +- spi-max-frequency:	Should be <100000> for this device.
> +
> +Optional sub-node properties:
> +- ubnt-spi,off_bright:	The value that will be sent if the LED should be
> +			switched off. Default value is 0.
> +- ubnt-spi,max_bright:	Value for the maximum brightness. Default value for that
> +			is 63.
> +- label:		A label for the LED. If one is given, the LED will be
> +			named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
> +
> +Being a SPI device this driver should be a sub-node of a SPI controller. The
> +controller only supports one LED. For consistence with other controllers, the
> +LED is defined as a sub-node.
> +
> +Example for the airCube ISP (with SPI controller matching that device):
> +
> +led_spi {
> +	compatible = "spi-gpio";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
> +	gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +	cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
> +	num-chipselects = <1>;

SPI node implementation is out of LED bindings scope.
Here you're showing spi-gpio configuration, but people are free to use
hardware SPI module if available on the platform of their choice.

Effectively, please remove above led_spi node. You can compare other SPI
based LED bindings, e.g.:

Documentation/devicetree/bindings/leds/leds-cr0014114.txt

> +	led_ubnt@0 {

s/led_usbnt/led-controller/

> +		compatible = "ubnt,spi-led";
> +		reg = <0>;
> +		spi-max-frequency = <100000>;
> +
> +		led {
> +			label = "system";

In label we expect "color:function" pattern. If section is to be left
empty than just leave it blank, e.g.:

			label = ":system"

But, is this LED function name telling something useful?
What is the actual function of this LED?

I work for some time on LED unification patch set and there is
a patch with common LED function names [0], but there is nothing
suitable for access points. There is "wlan", but is is rather for
wifi dongle LEDs (side note: "wifi" seems to be more ubiquitous,
so I will probably go for it finally).

So, maybe we for access points "wifi-ap" would work?
i.e. I propose the label in the form:

			label  = ":wifi-ap"


> +			/* keep the LED slightly on to show powered device */
> +			ubnt-spi,off_bright = /bits/ 8 <4>;
> +		};
> +	};
> +};
> 

[0] https://www.spinics.net/lists/kernel/msg3103824.html

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

I missed two issues, please take a look below.

> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>> From: Christian Mauderer <oss@c-mauderer.de>
>>
[...]
>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default 

 From DT POV the only valid reason for which max brightness level would
need to be limited is a protection against hardware damage. Please use
led-max-microamp property for that if this is the case. Otherwise such
setting can be skipped.

There are two bindings that allow for configuring max-brightness level
but they quite old.

>> value for that
>> +            is 63.
>> +- label:        A label for the LED. If one is given, the LED will be
>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>> +

Please use just:

label : see Documentation/devicetree/bindings/leds/common.txt

-- 
Best regards,
Jacek Anaszewski

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

[Christian Mauderer]

On 04/05/2019 21:45, Jacek Anaszewski wrote:
> I missed two issues, please take a look below.
> 
>> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>>> From: Christian Mauderer <oss@c-mauderer.de>
>>>
> [...]
>>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default 
> 
> From DT POV the only valid reason for which max brightness level would
> need to be limited is a protection against hardware damage. Please use
> led-max-microamp property for that if this is the case. Otherwise such
> setting can be skipped.
> 
> There are two bindings that allow for configuring max-brightness level
> but they quite old.
> 

My intention with that has been slightly different than limiting the
current:

The driver uses a very simple protocol for setting the brightness: It
sends one byte via SPI. The value of that byte can be (for that
controller) between 0 and 63. My intention when adding the limits has
been to allow adaption to similar hardware by changing the values. If
for example some other controller wants brightness values between 42 and
173 the off_bright could be set to 42 and the max_bright to 173.

Note that Pavel Machek suggested: "Alternatively, call its led-spi-byte,
or something, as it is really trivial protocol. Maybe other chips will
have same interface?"

If I follow that suggestion (which I think would be a good idea), I
would even need the values as mandatory ones instead of optional ones.
Would "led-spi-byte,off-value" and "led-spi-byte,max-value" be better
names in that case?

>>> value for that
>>> +            is 63.
>>> +- label:        A label for the LED. If one is given, the LED will be
>>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>>> +
> 
> Please use just:
> 
> label : see Documentation/devicetree/bindings/leds/common.txt
> 
I'll use that.

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

[Jacek Anaszewski]

On 5/4/19 10:01 PM, Christian Mauderer wrote:
> On 04/05/2019 21:45, Jacek Anaszewski wrote:
>> I missed two issues, please take a look below.
>>
>>> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>>>> From: Christian Mauderer <oss@c-mauderer.de>
>>>>
>> [...]
>>>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default
>>
>>  From DT POV the only valid reason for which max brightness level would
>> need to be limited is a protection against hardware damage. Please use
>> led-max-microamp property for that if this is the case. Otherwise such
>> setting can be skipped.
>>
>> There are two bindings that allow for configuring max-brightness level
>> but they quite old.
>>
> 
> My intention with that has been slightly different than limiting the
> current:
> 
> The driver uses a very simple protocol for setting the brightness: It
> sends one byte via SPI. The value of that byte can be (for that
> controller) between 0 and 63. My intention when adding the limits has
> been to allow adaption to similar hardware by changing the values. If
> for example some other controller wants brightness values between 42 and
> 173 the off_bright could be set to 42 and the max_bright to 173.
> 
> Note that Pavel Machek suggested: "Alternatively, call its led-spi-byte,
> or something, as it is really trivial protocol. Maybe other chips will
> have same interface?"
> 
> If I follow that suggestion (which I think would be a good idea), I
> would even need the values as mandatory ones instead of optional ones.
> Would "led-spi-byte,off-value" and "led-spi-byte,max-value" be better
> names in that case?

Yes, that makes perfect sense.

>>>> value for that
>>>> +            is 63.
>>>> +- label:        A label for the LED. If one is given, the LED will be
>>>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>>>> +
>>
>> Please use just:
>>
>> label : see Documentation/devicetree/bindings/leds/common.txt
>>
> I'll use that.

Thanks!

-- 
Best regards,
Jacek Anaszewski

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

[Christian Mauderer]

On 04/05/2019 21:34, Jacek Anaszewski wrote:
> Hi Christian,
> 
> Thank you for the patch.

Hello Jacek,

thank you for your time to review it.

> 
> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>> From: Christian Mauderer <oss@c-mauderer.de>
>>
>> This patch adds the binding documentation for the LED controller found
>> in Ubiquity airCube ISP devices.
>>
>> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
>> ---
>>
>> I tested the patches with a 4.14 and a 4.19 kernel on the current
>> OpenWRT.
>> Although I didn't get the kernel running due to file system problems
>> they build
>> fine with a 5.1-rc7.
>>
>> I shortly described the protocol of the controller in a comment in the
>> driver
>> file in the second patch.
>>
>> Checkpatch gives the following warning for both patches:
>>
>>    WARNING: added, moved or deleted file(s), does MAINTAINERS need
>> updating?
>>
>> To be honest: I don't know what to do with it. Please excuse my
>> ignorance here.
>> It's the first driver that I want to add to the Linux kernel.
> 
> You can add yourself as a maintainer of this driver, but it is customary
> rather for more complex drivers.

I would like to follow the best practice here. So if you say that for
this simple driver it's not usual, I won't add me. Except if you think
it is useful to have someone to blame for it ;-)

> 
>> Please point me to some documentation if I did miss some big points for
>> submitting patches.
>>
>>
>>   .../bindings/leds/leds-ubnt-spi.txt           | 49 +++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>> b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>> new file mode 100644
>> index 000000000000..ab1478cdc139
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>> @@ -0,0 +1,49 @@
>> +Binding for the controller based LED found in Ubiquity airCube ISP
>> and most
>> +likely some other Ubiquity devices.
>> +
>> +The protocol of the controller is quite simple. Only one byte will be
>> sent. The
>> +value of the byte can be between the ubnt-spi,off_bright value and the
>> +ubnt-spi,max_bright value.
>> +
>> +The driver maybe can be used for other devices with a similar
>> protocol too.
>> +
>> +Required properties:
>> +- compatible:        Should be "ubnt,spi-led".
>> +- spi-max-frequency:    Should be <100000> for this device.
>> +
>> +Optional sub-node properties:
>> +- ubnt-spi,off_bright:    The value that will be sent if the LED
>> should be
>> +            switched off. Default value is 0.
>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default
>> value for that
>> +            is 63.
>> +- label:        A label for the LED. If one is given, the LED will be
>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>> +
>> +Being a SPI device this driver should be a sub-node of a SPI
>> controller. The
>> +controller only supports one LED. For consistence with other
>> controllers, the
>> +LED is defined as a sub-node.
>> +
>> +Example for the airCube ISP (with SPI controller matching that device):
>> +
>> +led_spi {
>> +    compatible = "spi-gpio";
>> +    #address-cells = <1>;
>> +    #size-cells = <0>;
>> +
>> +    gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
>> +    gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
>> +    cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
>> +    num-chipselects = <1>;
> 
> SPI node implementation is out of LED bindings scope.
> Here you're showing spi-gpio configuration, but people are free to use
> hardware SPI module if available on the platform of their choice.
> 
> Effectively, please remove above led_spi node. You can compare other SPI
> based LED bindings, e.g.:
> 
> Documentation/devicetree/bindings/leds/leds-cr0014114.txt

Reason behind adding it was that most likely the controller is only
useful for that device. Of course that's no longer true together with
the suggestion from Pavel Machek to make it more generic.

I'll follow your suggestion.

> 
>> +    led_ubnt@0 {
> 
> s/led_usbnt/led-controller/
> 
>> +        compatible = "ubnt,spi-led";
>> +        reg = <0>;
>> +        spi-max-frequency = <100000>;
>> +
>> +        led {
>> +            label = "system";
> 
> In label we expect "color:function" pattern. If section is to be left
> empty than just leave it blank, e.g.:
> 
>             label = ":system"
> 
> But, is this LED function name telling something useful?
> What is the actual function of this LED?

The LED is a white one. It's the only LED of a WLAN access point. There
are not even any network LEDs. Ubiquiti uses it to show some system
states with different pulsing patterns during boot. After that it's
always on.

In OpenWRT it maybe could also show some network traffic (blinking
between dim and bright). But that's user configurable. My plan was to
just let it dim when the system is booted.

> 
> I work for some time on LED unification patch set and there is
> a patch with common LED function names [0], but there is nothing
> suitable for access points. There is "wlan", but is is rather for
> wifi dongle LEDs (side note: "wifi" seems to be more ubiquitous,
> so I will probably go for it finally).
> 
> So, maybe we for access points "wifi-ap" would work?
> i.e. I propose the label in the form:
> 
>             label  = ":wifi-ap"
> 

I wasn't aware of that list. Maybe "power" or even better "status" would
match the function.

Should I add the color too? So "white:status"?

> 
>> +            /* keep the LED slightly on to show powered device */
>> +            ubnt-spi,off_bright = /bits/ 8 <4>;
>> +        };
>> +    };
>> +};
>>
> 
> [0] https://www.spinics.net/lists/kernel/msg3103824.html
> 

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

[Jacek Anaszewski]

On 5/4/19 9:48 PM, Christian Mauderer wrote:
> On 04/05/2019 21:34, Jacek Anaszewski wrote:
>> Hi Christian,
>>
>> Thank you for the patch.
> 
> Hello Jacek,
> 
> thank you for your time to review it.

You're welcome.

>>
>> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>>> From: Christian Mauderer <oss@c-mauderer.de>
>>>
>>> This patch adds the binding documentation for the LED controller found
>>> in Ubiquity airCube ISP devices.
>>>
>>> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
>>> ---
>>>
>>> I tested the patches with a 4.14 and a 4.19 kernel on the current
>>> OpenWRT.
>>> Although I didn't get the kernel running due to file system problems
>>> they build
>>> fine with a 5.1-rc7.
>>>
>>> I shortly described the protocol of the controller in a comment in the
>>> driver
>>> file in the second patch.
>>>
>>> Checkpatch gives the following warning for both patches:
>>>
>>>     WARNING: added, moved or deleted file(s), does MAINTAINERS need
>>> updating?
>>>
>>> To be honest: I don't know what to do with it. Please excuse my
>>> ignorance here.
>>> It's the first driver that I want to add to the Linux kernel.
>>
>> You can add yourself as a maintainer of this driver, but it is customary
>> rather for more complex drivers.
> 
> I would like to follow the best practice here. So if you say that for
> this simple driver it's not usual, I won't add me. Except if you think
> it is useful to have someone to blame for it ;-)

People will always have your email in the driver, so I'd ignore
that warning as Pavel already proposed.

>>> Please point me to some documentation if I did miss some big points for
>>> submitting patches.
>>>
>>>
>>>    .../bindings/leds/leds-ubnt-spi.txt           | 49 +++++++++++++++++++
>>>    1 file changed, 49 insertions(+)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> new file mode 100644
>>> index 000000000000..ab1478cdc139
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> @@ -0,0 +1,49 @@
>>> +Binding for the controller based LED found in Ubiquity airCube ISP
>>> and most
>>> +likely some other Ubiquity devices.
>>> +
>>> +The protocol of the controller is quite simple. Only one byte will be
>>> sent. The
>>> +value of the byte can be between the ubnt-spi,off_bright value and the
>>> +ubnt-spi,max_bright value.
>>> +
>>> +The driver maybe can be used for other devices with a similar
>>> protocol too.
>>> +
>>> +Required properties:
>>> +- compatible:        Should be "ubnt,spi-led".
>>> +- spi-max-frequency:    Should be <100000> for this device.
>>> +
>>> +Optional sub-node properties:
>>> +- ubnt-spi,off_bright:    The value that will be sent if the LED
>>> should be
>>> +            switched off. Default value is 0.
>>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default
>>> value for that
>>> +            is 63.
>>> +- label:        A label for the LED. If one is given, the LED will be
>>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>>> +
>>> +Being a SPI device this driver should be a sub-node of a SPI
>>> controller. The
>>> +controller only supports one LED. For consistence with other
>>> controllers, the
>>> +LED is defined as a sub-node.
>>> +
>>> +Example for the airCube ISP (with SPI controller matching that device):
>>> +
>>> +led_spi {
>>> +    compatible = "spi-gpio";
>>> +    #address-cells = <1>;
>>> +    #size-cells = <0>;
>>> +
>>> +    gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
>>> +    gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>> +    cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
>>> +    num-chipselects = <1>;
>>
>> SPI node implementation is out of LED bindings scope.
>> Here you're showing spi-gpio configuration, but people are free to use
>> hardware SPI module if available on the platform of their choice.
>>
>> Effectively, please remove above led_spi node. You can compare other SPI
>> based LED bindings, e.g.:
>>
>> Documentation/devicetree/bindings/leds/leds-cr0014114.txt
> 
> Reason behind adding it was that most likely the controller is only
> useful for that device. Of course that's no longer true together with
> the suggestion from Pavel Machek to make it more generic.
> 
> I'll follow your suggestion.

Thank you.
>>> +    led_ubnt@0 {
>>
>> s/led_usbnt/led-controller/
>>
>>> +        compatible = "ubnt,spi-led";
>>> +        reg = <0>;
>>> +        spi-max-frequency = <100000>;
>>> +
>>> +        led {
>>> +            label = "system";
>>
>> In label we expect "color:function" pattern. If section is to be left
>> empty than just leave it blank, e.g.:
>>
>>              label = ":system"
>>
>> But, is this LED function name telling something useful?
>> What is the actual function of this LED?
> 
> The LED is a white one. It's the only LED of a WLAN access point. There
> are not even any network LEDs. Ubiquiti uses it to show some system
> states with different pulsing patterns during boot. After that it's
> always on.
> 
> In OpenWRT it maybe could also show some network traffic (blinking
> between dim and bright). But that's user configurable. My plan was to
> just let it dim when the system is booted.

Thank you for this explanation.

>>
>> I work for some time on LED unification patch set and there is
>> a patch with common LED function names [0], but there is nothing
>> suitable for access points. There is "wlan", but is is rather for
>> wifi dongle LEDs (side note: "wifi" seems to be more ubiquitous,
>> so I will probably go for it finally).
>>
>> So, maybe we for access points "wifi-ap" would work?
>> i.e. I propose the label in the form:
>>
>>              label  = ":wifi-ap"
>>
> 
> I wasn't aware of that list. Maybe "power" or even better "status" would
> match the function.

Hmm, I've just found out that there are two "wlan-ap" occurrences in
the existing mainline bindings, so I propose to follow that.

> Should I add the color too? So "white:status"?

Yes, why not if it is known. So, having the above I propose:

		label = "white:wlan-ap";

>>
>>> +            /* keep the LED slightly on to show powered device */
>>> +            ubnt-spi,off_bright = /bits/ 8 <4>;
>>> +        };
>>> +    };
>>> +};
>>>
>>
>> [0] https://www.spinics.net/lists/kernel/msg3103824.html
>>
> 

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

Hi!

> >I wasn't aware of that list. Maybe "power" or even better "status" would
> >match the function.
> 
> Hmm, I've just found out that there are two "wlan-ap" occurrences in
> the existing mainline bindings, so I propose to follow that.
> 
> >Should I add the color too? So "white:status"?
> 
> Yes, why not if it is known. So, having the above I propose:
> 
> 		label = "white:wlan-ap";

Linux now runs on many different devices, and I believe userland wants
to know "this is main notification LED for this device" (and the only
one in this case).

Phones normally have at most one such LED. Now access point. Someone
will make ethernet switch with just one LED. Maybe thermostat will
have only one LED. Bluetooth GPS. ...

I'd prefer not to mention type of device, and simply state that this
is status LED for the device. Droid 4 uses
"status-led:{red,green,blue}", so "status-led:white" would seem
reasonable.

"white:status" sounds good, too. But I'd prefer not to mention type of
the device, it is simply status LED.

Best regards,
									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 1/2] dt-bindings: leds: Add binding for ubnt-spi LED.

[Pavel Machek]

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

Hi!

> > >I wasn't aware of that list. Maybe "power" or even better "status" would
> > >match the function.
> > 
> > Hmm, I've just found out that there are two "wlan-ap" occurrences in
> > the existing mainline bindings, so I propose to follow that.

Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".

> > >Should I add the color too? So "white:status"?
> > 
> > Yes, why not if it is known. So, having the above I propose:
> > 
> > 		label = "white:wlan-ap";
> 
> Linux now runs on many different devices, and I believe userland wants
> to know "this is main notification LED for this device" (and the only
> one in this case).

...and I guess if you have single LED it will be used for more than
"is AP active". IOW it will likely to be more similar to "status" than
"wlan-ap".

Best regards,
									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 1/2] dt-bindings: leds: Add binding for ubnt-spi LED.

[Christian Mauderer]

On 05/05/2019 00:17, Pavel Machek wrote:
> Hi!
> 
>>>> I wasn't aware of that list. Maybe "power" or even better "status" would
>>>> match the function.
>>>
>>> Hmm, I've just found out that there are two "wlan-ap" occurrences in
>>> the existing mainline bindings, so I propose to follow that.
> 
> Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".
> 
>>>> Should I add the color too? So "white:status"?
>>>
>>> Yes, why not if it is known. So, having the above I propose:
>>>
>>> 		label = "white:wlan-ap";
>>
>> Linux now runs on many different devices, and I believe userland wants
>> to know "this is main notification LED for this device" (and the only
>> one in this case).
> 
> ...and I guess if you have single LED it will be used for more than
> "is AP active". IOW it will likely to be more similar to "status" than
> "wlan-ap".
> 
> Best regards,
> 									Pavel
> 

As far as I understand it the dt-bindings is an example anyway, right?
So it's not really relevant what the LED does in my specific system.
Jacek said

> In label we expect "color:function" pattern.

Therefore I think that a "white:status" would be a good example label
and I'll use that.

I'll prepare a patch set v2 and (hopefully) send it in the next hours
after I've tested in on my hardware.

Best regards

Christian

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

[Jacek Anaszewski]

Hi,

On 5/5/19 12:17 AM, Pavel Machek wrote:
> Hi!
> 
>>>> I wasn't aware of that list. Maybe "power" or even better "status" would
>>>> match the function.
>>>
>>> Hmm, I've just found out that there are two "wlan-ap" occurrences in
>>> the existing mainline bindings, so I propose to follow that.
> 
> Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".
> 
>>>> Should I add the color too? So "white:status"?
>>>
>>> Yes, why not if it is known. So, having the above I propose:
>>>
>>> 		label = "white:wlan-ap";
>>
>> Linux now runs on many different devices, and I believe userland wants
>> to know "this is main notification LED for this device" (and the only
>> one in this case).

This LED is on the access point, so it should have this affiliation
somehow represented in the name.

> ...and I guess if you have single LED it will be used for more than
> "is AP active". IOW it will likely to be more similar to "status" than
> "wlan-ap".

IMO if a LED is on some specific device, then it should be reflected
in the "function" section of the LED name. It facilitates locating in
physically. If usersapce wants to change its purpose it is free to do
so. e.g. via triggers. But it will not affect the LED name.

-- 
Best regards,
Jacek Anaszewski

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

[Christian Mauderer]

On 05/05/2019 12:56, Jacek Anaszewski wrote:
> Hi,
> 
> On 5/5/19 12:17 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>> I wasn't aware of that list. Maybe "power" or even better "status"
>>>>> would
>>>>> match the function.
>>>>
>>>> Hmm, I've just found out that there are two "wlan-ap" occurrences in
>>>> the existing mainline bindings, so I propose to follow that.
>>
>> Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".
>>
>>>>> Should I add the color too? So "white:status"?
>>>>
>>>> Yes, why not if it is known. So, having the above I propose:
>>>>
>>>> ������� label = "white:wlan-ap";
>>>
>>> Linux now runs on many different devices, and I believe userland wants
>>> to know "this is main notification LED for this device" (and the only
>>> one in this case).
> 
> This LED is on the access point, so it should have this affiliation
> somehow represented in the name.
> 
>> ...and I guess if you have single LED it will be used for more than
>> "is AP active". IOW it will likely to be more similar to "status" than
>> "wlan-ap".
> 
> IMO if a LED is on some specific device, then it should be reflected
> in the "function" section of the LED name. It facilitates locating in
> physically. If usersapce wants to change its purpose it is free to do
> so. e.g. via triggers. But it will not affect the LED name.
> 

This looks like a strongly opinion based discussion. Currently it's only
relevant for the name in the examble in the dt-binding document. Isn't
it completely irrelevant what I use there? The controller could be used
on any device, couldn't it?

Regarding your argument toward naming (maybe relevant when I add it to
the device tree files in OpenWRT): Would that mean that if a
hypothetical server has one LED to show some arbitrary system states it
should be named "green:server" instead of "green:status" because it is
on a specific device (server in that case)?

I would expect a LED called "wlan-ap" to show the status of the WLAN.
Like many small WLAN access points, the airCube has multiple ports. So
it could be used as a generic router with WLAN switched off too. In that
case "wlan-ap" wouldn't really fit the function of the LED.

Like I said: Ubiquiti uses the LED in the original software to show some
system events like "I'm finished booting" (steady brightness) or "I'm
currently upgrading" (blinking pattern).

Best regards

Christian

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

[Jacek Anaszewski]

On 5/5/19 1:51 PM, Christian Mauderer wrote:
> On 05/05/2019 12:56, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 5/5/19 12:17 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> I wasn't aware of that list. Maybe "power" or even better "status"
>>>>>> would
>>>>>> match the function.
>>>>>
>>>>> Hmm, I've just found out that there are two "wlan-ap" occurrences in
>>>>> the existing mainline bindings, so I propose to follow that.
>>>
>>> Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".
>>>
>>>>>> Should I add the color too? So "white:status"?
>>>>>
>>>>> Yes, why not if it is known. So, having the above I propose:
>>>>>
>>>>>  ������� label = "white:wlan-ap";
>>>>
>>>> Linux now runs on many different devices, and I believe userland wants
>>>> to know "this is main notification LED for this device" (and the only
>>>> one in this case).
>>
>> This LED is on the access point, so it should have this affiliation
>> somehow represented in the name.
>>
>>> ...and I guess if you have single LED it will be used for more than
>>> "is AP active". IOW it will likely to be more similar to "status" than
>>> "wlan-ap".
>>
>> IMO if a LED is on some specific device, then it should be reflected
>> in the "function" section of the LED name. It facilitates locating in
>> physically. If usersapce wants to change its purpose it is free to do
>> so. e.g. via triggers. But it will not affect the LED name.
>>
> 
> This looks like a strongly opinion based discussion. Currently it's only
> relevant for the name in the examble in the dt-binding document. Isn't
> it completely irrelevant what I use there? The controller could be used
> on any device, couldn't it?
> 
> Regarding your argument toward naming (maybe relevant when I add it to
> the device tree files in OpenWRT): Would that mean that if a
> hypothetical server has one LED to show some arbitrary system states it
> should be named "green:server" instead of "green:status" because it is
> on a specific device (server in that case)?
> 
> I would expect a LED called "wlan-ap" to show the status of the WLAN.
> Like many small WLAN access points, the airCube has multiple ports. So
> it could be used as a generic router with WLAN switched off too. In that
> case "wlan-ap" wouldn't really fit the function of the LED.
> 
> Like I said: Ubiquiti uses the LED in the original software to show some
> system events like "I'm finished booting" (steady brightness) or "I'm
> currently upgrading" (blinking pattern).

OK, I've misinterpreted something here. Let's have "white:status"
in the example.

-- 
Best regards,
Jacek Anaszewski

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

[Pavel Machek]

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

On Sun 2019-05-05 12:56:35, Jacek Anaszewski wrote:
> Hi,
> 
> On 5/5/19 12:17 AM, Pavel Machek wrote:
> >Hi!
> >
> >>>>I wasn't aware of that list. Maybe "power" or even better "status" would
> >>>>match the function.
> >>>
> >>>Hmm, I've just found out that there are two "wlan-ap" occurrences in
> >>>the existing mainline bindings, so I propose to follow that.
> >
> >Let me see... dove-d3plug.dts has "status", "wlan-ap", "wlan-act".
> >
> >>>>Should I add the color too? So "white:status"?
> >>>
> >>>Yes, why not if it is known. So, having the above I propose:
> >>>
> >>>		label = "white:wlan-ap";
> >>
> >>Linux now runs on many different devices, and I believe userland wants
> >>to know "this is main notification LED for this device" (and the only
> >>one in this case).
> 
> This LED is on the access point, so it should have this affiliation
> somehow represented in the name.
> 
> >...and I guess if you have single LED it will be used for more than
> >"is AP active". IOW it will likely to be more similar to "status" than
> >"wlan-ap".
> 
> IMO if a LED is on some specific device, then it should be reflected
> in the "function" section of the LED name. It facilitates locating in
> physically. If usersapce wants to change its purpose it is free to do
> so. e.g. via triggers. But it will not affect the LED name.

I'd prefer not to do that. Existing ":wlan-ap" LED is a LED probably
indicating that access point is on; while this will normally indicate
system status. Its function is more similar to "status" on
dove-d3plug.dts.

I'd prefer LED names not to depend on function of system they are
in. Status LED on phone should be named same way status LED of
wireless AP is.

Best regards,
								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 --]