Skip to contentSkip to conversation

AS3645A fixes

PATCH RESEND Superseded Sakari Ailus 7 participants 16 messages Sep 18, 2017 – Nov 18, 2017 1 message not in the archive

Subsystems (3)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover AS3645A fixes

Hi folks,

(Resending, fixed linux-media list address.)

Here are a few fixes for the as3645a DTS as well as changes in bindings. The driver is not in a release yet. I'd like to get these in as through the media tree fixes branch.

since v1:

- Add LED colour to the name of the LED, this adds two patches to the set.

- Add a patch to unregister the indicator LED in driver remove function.

- No changes to v1 patches.

Sakari Ailus (6):
as3645a: Use ams,input-max-microamp as documented in DT bindings
dt: bindings: as3645a: Use LED number to refer to LEDs
as3645a: Use integer numbers for parsing LEDs
dt: bindings: as3645a: Improve label documentation, DT example
as3645a: Add colour to LED name
as3645a: Unregister indicator LED on device unbind

 .../devicetree/bindings/leds/ams,as3645a.txt       | 40 ++++++++++++++--------
 arch/arm/boot/dts/omap3-n950-n9.dtsi               | 10 ++++--
 drivers/leds/leds-as3645a.c                        | 33 +++++++++++++++---
 3 files changed, 61 insertions(+), 22 deletions(-)
Signature · 1 line
2.11.0
#1 lore raw

PATCH RESEND v2 1/6 as3645a: Use ams,input-max-microamp as documented in DT bindings

DT bindings document the property "ams,input-max-microamp" that limits the chip's maximum input current. The driver and the DTS however used "peak-current-limit" property. Fix this by using the property documented in DT binding documentation.

---
 arch/arm/boot/dts/omap3-n950-n9.dtsi | 2 +-
 drivers/leds/leds-as3645a.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/boot/dts/omap3-n950-n9.dtsi +1 -1
index cb47ae79a5f9..b86fc83a5a65 100644
@@ -273,7 +273,7 @@
 			flash-timeout-us = <150000>;
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
-			peak-current-limit = <1750000>;
+			ams,input-max-microamp = <1750000>;
 		};
 		indicator {
 			led-max-microamp = <10000>;
drivers/leds/leds-as3645a.c +1 -1
index bbbbe0898233..e3f89c6130d2 100644
@@ -534,7 +534,7 @@ static int as3645a_parse_node(struct as3645a *flash,
 	of_property_read_u32(flash->flash_node, "voltage-reference",
 			     &cfg->voltage_reference);
 
-	of_property_read_u32(flash->flash_node, "peak-current-limit",
+	of_property_read_u32(flash->flash_node, "ams,input-max-microamp",
 			     &cfg->peak);
 	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
 
Signature · 1 line
2.11.0
#2 lore raw

PATCH RESEND v2 2/6 dt: bindings: as3645a: Use LED number to refer to LEDs

In reply to Sakari Ailus

Use integers (reg property) to tell the number of the LED to the driver instead of the node name. While both of these approaches are currently used by the LED bindings, using integers will require less driver changes for ACPI support. Additionally, it will make possible LED naming using chip and LED node names, effectively making the label property most useful for human-readable names only.

---
 .../devicetree/bindings/leds/ams,as3645a.txt       | 28 ++++++++++++++--------
 1 file changed, 18 insertions(+), 10 deletions(-)
Documentation/devicetree/bindings/leds/ams,as3645a.txt +18 -10
index 12c5ef26ec73..fdc40e354a64 100644
@@ -15,11 +15,14 @@ Required properties
 
 compatible	: Must be "ams,as3645a".
 reg		: The I2C address of the device. Typically 0x30.
+#address-cells	: 1
+#size-cells	: 0
 
 
-Required properties of the "flash" child node
-=============================================
+Required properties of the flash child node (0)
+===============================================
 
+reg: 0
 flash-timeout-us: Flash timeout in microseconds. The value must be in
 		  the range [100000, 850000] and divisible by 50000.
 flash-max-microamp: Maximum flash current in microamperes. Has to be
@@ -33,20 +36,21 @@ ams,input-max-microamp: Maximum flash controller input current. The
 			and divisible by 50000.
 
 
-Optional properties of the "flash" child node
-=============================================
+Optional properties of the flash child node
+===========================================
 
 label		: The label of the flash LED.
 
 
-Required properties of the "indicator" child node
-=================================================
+Required properties of the indicator child node (1)
+===================================================
 
+reg: 1
 led-max-microamp: Maximum indicator current. The allowed values are
 		  2500, 5000, 7500 and 10000.
 
-Optional properties of the "indicator" child node
-=================================================
+Optional properties of the indicator child node
+===============================================
 
 label		: The label of the indicator LED.
 
@@ -55,16 +59,20 @@ Example
 =======
 
 	as3645a@30 {
+		#address-cells: 1
+		#size-cells: 0
 		reg = <0x30>;
 		compatible = "ams,as3645a";
-		flash {
+		flash@0 {
+			reg = <0x0>;
 			flash-timeout-us = <150000>;
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;
 			label = "as3645a:flash";
 		};
-		indicator {
+		indicator@1 {
+			reg = <0x1>;
 			led-max-microamp = <10000>;
 			label = "as3645a:indicator";
 		};
Signature · 1 line
2.11.0
#3 lore raw

PATCH RESEND v2 3/6 as3645a: Use integer numbers for parsing LEDs

In reply to Sakari Ailus

Use integer numbers for LEDs, 0 is the flash and 1 is the indicator.

---
 arch/arm/boot/dts/omap3-n950-n9.dtsi |  8 ++++++--
 drivers/leds/leds-as3645a.c          | 26 ++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)
arch/arm/boot/dts/omap3-n950-n9.dtsi +6 -2
index b86fc83a5a65..1b0bd72945f2 100644
@@ -267,15 +267,19 @@
 	clock-frequency = <400000>;
 
 	as3645a@30 {
+		#address-cells = <1>;
+		#size-cells = <0>;
 		reg = <0x30>;
 		compatible = "ams,as3645a";
-		flash {
+		flash@0 {
+			reg = <0x0>;
 			flash-timeout-us = <150000>;
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;
 		};
-		indicator {
+		indicator@1 {
+			reg = <0x1>;
 			led-max-microamp = <10000>;
 		};
 	};
drivers/leds/leds-as3645a.c +24 -2
index e3f89c6130d2..605e0c64e974 100644
@@ -112,6 +112,10 @@
 #define AS_PEAK_mA_TO_REG(a) \
 	((min_t(u32, AS_PEAK_mA_MAX, a) - 1250) / 250)
 
+/* LED numbers for Devicetree */
+#define AS_LED_FLASH				0
+#define AS_LED_INDICATOR			1
+
 enum as_mode {
 	AS_MODE_EXT_TORCH = 0 << AS_CONTROL_MODE_SETTING_SHIFT,
 	AS_MODE_INDICATOR = 1 << AS_CONTROL_MODE_SETTING_SHIFT,
@@ -491,10 +495,29 @@ static int as3645a_parse_node(struct as3645a *flash,
 			      struct device_node *node)
 {
 	struct as3645a_config *cfg = &flash->cfg;
+	struct device_node *child;
 	const char *name;
 	int rval;
 
-	flash->flash_node = of_get_child_by_name(node, "flash");
+	for_each_child_of_node(node, child) {
+		u32 id = 0;
+
+		of_property_read_u32(child, "reg", &id);
+
+		switch (id) {
+		case AS_LED_FLASH:
+			flash->flash_node = of_node_get(child);
+			break;
+		case AS_LED_INDICATOR:
+			flash->indicator_node = of_node_get(child);
+			break;
+		default:
+			dev_warn(&flash->client->dev,
+				 "unknown LED %u encountered, ignoring\n", id);
+			break;
+		}
+	}
+
 	if (!flash->flash_node) {
 		dev_err(&flash->client->dev, "can't find flash node\n");
 		return -ENODEV;
@@ -538,7 +561,6 @@ static int as3645a_parse_node(struct as3645a *flash,
 			     &cfg->peak);
 	cfg->peak = AS_PEAK_mA_TO_REG(cfg->peak);
 
-	flash->indicator_node = of_get_child_by_name(node, "indicator");
 	if (!flash->indicator_node) {
 		dev_warn(&flash->client->dev,
 			 "can't find indicator node\n");
Signature · 1 line
2.11.0

A message that is not in the archive (sent off-list or not yet received): 20170918102349.8935-1-sakari.ailus-VuQAYsv1563Yd54FQh9/[email protected]

#5 lore raw

PATCH RESEND v2 4/6 dt: bindings: as3645a: Improve label documentation, DT example

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

---
 Documentation/devicetree/bindings/leds/ams,as3645a.txt | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/leds/ams,as3645a.txt +8 -4
index fdc40e354a64..9adba41e74b3 100644
@@ -39,7 +39,9 @@ ams,input-max-microamp: Maximum flash controller input current. The
 Optional properties of the flash child node
 ===========================================
 
-label		: The label of the flash LED.
+label		: The label of the flash LED. The label is otherwise assumed to
+		  be the device node name concatenated with ":white:" and the
+		  name of the flash LED node is assumed if omitted.
 
 
 Required properties of the indicator child node (1)
@@ -52,7 +54,9 @@ led-max-microamp: Maximum indicator current. The allowed values are
 Optional properties of the indicator child node
 ===============================================
 
-label		: The label of the indicator LED.
+label		: The label of the indicator LED. The label is otherwise assumed
+		  to be the device node name concatenated with ":red:" and the
+		  name of the indicator LED node is assumed if omitted.
 
 
 Example
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};
Signature · 6 lines
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#6 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

Thanks,
Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#7 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

For association with a particular camera --- in the long run I'd propose to use Media controller / property API for that in the long run. We don't have that yet though.

Cc Jacek, too.

Signature · 6 lines
Sakari Ailus
e-mail: [email protected]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#8 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

Quoted 3 lines

For association with a particular camera --- in the long run I'd propose to use Media controller / property API for that in the long run. We don't have that yet though.

We don't have that yet. Plus simple applications may not want to talk v4l2 ioctls....

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#9 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Hi Pavel,

Quoted 42 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

Quoted 7 lines

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

Signature · 6 lines
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#10 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Quoted 50 lines · Jacek Anaszewski

Hi Pavel,

Quoted 47 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Quoted 7 lines
Quoted 2 lines

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

I really dislike how this naming convention is used for label. label is supposed to be the phyically identifiable name. Having the devicename defeats that. Perhaps color, too. We'd be better off with a color property. It seems we're overloading the naming with too many things. Now we're adding device association.

I do want to see standard names though. On 96boards for example, there are defined LEDs and locations. The function on some are defined (e.g. WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see the same label across all boards.

Rob

#11 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Quoted 59 lines · Rob Herring
Quoted 50 lines · Jacek Anaszewski

Hi Pavel,

Quoted 47 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

Documentation/devicetree/bindings/leds/ams,as3645a.txt +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Documentation/devicetree/bindings/leds/ams,as3645a.txt Example · line 83

Quoted 10 lines
Documentation/devicetree/bindings/leds/ams,as3645a.txt +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Quoted 7 lines
Quoted 2 lines

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

Quoted 5 lines

I really dislike how this naming convention is used for label. label is supposed to be the phyically identifiable name. Having the devicename defeats that. Perhaps color, too. We'd be better off with a color property. It seems we're overloading the naming with too many things. Now we're adding device association.

Regarding devicename - there is indeed inconsistency in the way how LED DT bindings use label, as some of them use it for defining full LED class device name, and the rest fill only colour and function, leaving addition of a devicename to the driver.

The problem is also in current definition of label in LED common bindings documentation, which says:

"It has to uniquely identify a device, i.e. no other LED class device can be assigned the same label."

In view of your above words this is not true, and we probably should remove this sentence (it doesn't have DT maintainer ack btw).

Quoted 4 lines

I do want to see standard names though. On 96boards for example, there are defined LEDs and locations. The function on some are defined (e.g. WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see the same label across all boards.

Currently we have following LED functions (obtained with grep label Documentation/devicetree/bindings/leds/* | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed '/^$/d' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sort -u)

0
1
2
2g
3
4
5
6
7
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
green
indicator
inet
keypad
phone
power
red
sata
sata0
sata1
tel
tv
upgrading
usb
usr0
usr1
usr35
wan
white
wireless
wps
yellow

By extracting numerical pattern names and replacing numbers with N we're getting something like this:

N
Ng
colour
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
indicator
inet
keypad
phone
power
sataN
tel
tv
upgrading
usb
usrN
wan
wireless
wps

Is this list something you'd like to see as a base of standard LED functions? It seems that this list would have to be continuously supplemented with new positions.

Signature · 6 lines
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#12 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Quoted 159 lines · Jacek Anaszewski
Quoted 59 lines · Rob Herring
Quoted 50 lines · Jacek Anaszewski

Hi Pavel,

Quoted 47 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

(quoted diff) +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Review Example · line 83

Quoted 10 lines
(quoted diff) +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Quoted 7 lines
Quoted 2 lines

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

Quoted 5 lines

I really dislike how this naming convention is used for label. label is supposed to be the phyically identifiable name. Having the devicename defeats that. Perhaps color, too. We'd be better off with a color property. It seems we're overloading the naming with too many things. Now we're adding device association.

Regarding devicename - there is indeed inconsistency in the way how LED DT bindings use label, as some of them use it for defining full LED class device name, and the rest fill only colour and function, leaving addition of a devicename to the driver.

The problem is also in current definition of label in LED common bindings documentation, which says:

"It has to uniquely identify a device, i.e. no other LED class device can be assigned the same label."

In view of your above words this is not true, and we probably should remove this sentence (it doesn't have DT maintainer ack btw).

Quoted 4 lines

I do want to see standard names though. On 96boards for example, there are defined LEDs and locations. The function on some are defined (e.g. WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see the same label across all boards.

Currently we have following LED functions (obtained with grep label Documentation/devicetree/bindings/leds/* | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed '/^$/d' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sort -u)

0
1
2
2g
3
4
5
6
7
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
green
indicator
inet
keypad
phone
power
red
sata
sata0
sata1
tel
tv
upgrading
usb
usr0
usr1
usr35
wan
white
wireless
wps
yellow

By extracting numerical pattern names and replacing numbers with N we're getting something like this:

N
Ng
colour
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
indicator
inet
keypad
phone
power
sataN
tel
tv
upgrading
usb
usrN
wan
wireless
wps

Quoted 4 lines

Is this list something you'd like to see as a base of standard LED functions? It seems that this list would have to be continuously supplemented with new positions.

Even better option is to grep through all *.dts* files in the arch directory. A slightly modified command chain. which removes numerical postfixes (there are some not covered corner cases though)

find arch -name "*.dts*" | xargs grep label | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sed s'/\([^0-9]*\)\([0-9]*\)/\1/' | sed '/^$/d' | sort -u

gives following 135 positions (~5 colours percolated due to some non-covered corner cases), and some of them don't belong to LED DT nodes unfortunately, as e.g. gpio-keys use also ":" as a delimiter in their labels, but the whole set gives reasonable overview I think:

active
activity_led
adsl
alarm
alive
all
amber
app
aux
backup
backup_led
bl
blue
bluetooth
boot
bottom
brick-status
bt
CEL
chrg
COM
copy
core_module
cpu
D
debug
DIA
disk
disk_led
down
dsl
enocean
enter
err
error
esata
ethernet-status
fail
fault
front
func
function
g
ghz
ghz-1
ghz-2
gpio
green
gsm
HD
hdd
hdderr
health
health_led
heart
heartbeat
home
inet
info
internet
keypad
L
lan
led
ledb
left
l_hdd
live
logo
microSD
misc
mmc
nand
network
on
orange
os
panel
pmu_stat
power
power_led
programming
proximitysensor
pulse
pwr
qss
rebuild_led
red
r_hdd
right
router
rs
rx
sata
sata-l
sata-r
sd
SD
sleep
standby
stat
state
status
Status
sw
sys
system
system-status
tel
top
tv
tx
up
usb
USB
usb_1
usb_2
usb_copy
usb-port1
usb-port2
user
USER
usr
wan
white
wifi
wifi_ap
wifi-status
wireless
wlan
wlan_g
wmode
wps
WPS
yellow

Signature · 2 lines
Best regards,
Jacek Anaszewski
#13 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

Hi Rob,

Any thoughts on my analysis? Do you think that LED function naming standardization would really make sense having the abundance of LED functions categories present in the mainline dts files?

The list of standard LED function names would have to be continuously extended as people would be adding new boards. Limiting users to only existing set of LED functions wouldn't be practical IMHO.

I'd propose only to modify 'label' property description in LED common bindings, so that it would explicitly state that it should contain only "colour:functon" segments. It would be driver's responsibility to add "devicename:" prefix to the label and use the whole string for a LED class device name. Some drivers do that already. All DT bindings, dts files and LED class drivers that don't adhere to this rule would have to be updated accordingly.

Regarding colour - the "devicename:colour:function" LED device naming convention defined in Documentation/leds/leds-class.txt is around for a long time. Since LED colour can vary from board two board and is independent of a driver, we have to have a means for defining it in DT. Would you see providing separate 'colour' DT property as a solution?

Best regards,
Jacek Anaszewski

Quoted 315 lines · Jacek Anaszewski
Quoted 159 lines · Jacek Anaszewski
Quoted 59 lines · Rob Herring
Quoted 50 lines · Jacek Anaszewski

Hi Pavel,

Quoted 47 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

(quoted diff) +0 -0
@@ -69,11 +73,11 @@ Example
 			flash-max-microamp = <320000>;
 			led-max-microamp = <60000>;
 			ams,input-max-microamp = <1750000>;

Review Example · line 83

Quoted 10 lines
(quoted diff) +2 -2
-			label = "as3645a:flash";
+			label = "as3645a:white:flash";
 		};
 		indicator@1 {
 			reg = <0x1>;
 			led-max-microamp = <10000>;
-			label = "as3645a:indicator";
+			label = "as3645a:red:indicator";
 		};
 	};

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Quoted 7 lines
Quoted 2 lines

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

Quoted 5 lines

I really dislike how this naming convention is used for label. label is supposed to be the phyically identifiable name. Having the devicename defeats that. Perhaps color, too. We'd be better off with a color property. It seems we're overloading the naming with too many things. Now we're adding device association.

Regarding devicename - there is indeed inconsistency in the way how LED DT bindings use label, as some of them use it for defining full LED class device name, and the rest fill only colour and function, leaving addition of a devicename to the driver.

The problem is also in current definition of label in LED common bindings documentation, which says:

"It has to uniquely identify a device, i.e. no other LED class device can be assigned the same label."

In view of your above words this is not true, and we probably should remove this sentence (it doesn't have DT maintainer ack btw).

Quoted 4 lines

I do want to see standard names though. On 96boards for example, there are defined LEDs and locations. The function on some are defined (e.g. WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see the same label across all boards.

Currently we have following LED functions (obtained with grep label Documentation/devicetree/bindings/leds/* | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed '/^$/d' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sort -u)

0
1
2
2g
3
4
5
6
7
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
green
indicator
inet
keypad
phone
power
red
sata
sata0
sata1
tel
tv
upgrading
usb
usr0
usr1
usr35
wan
white
wireless
wps
yellow

By extracting numerical pattern names and replacing numbers with N we're getting something like this:

N
Ng
colour
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
indicator
inet
keypad
phone
power
sataN
tel
tv
upgrading
usb
usrN
wan
wireless
wps

Quoted 4 lines

Is this list something you'd like to see as a base of standard LED functions? It seems that this list would have to be continuously supplemented with new positions.

Even better option is to grep through all *.dts* files in the arch directory. A slightly modified command chain. which removes numerical postfixes (there are some not covered corner cases though)

find arch -name "*.dts*" | xargs grep label | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sed s'/\([^0-9]*\)\([0-9]*\)/\1/' | sed '/^$/d' | sort -u

gives following 135 positions (~5 colours percolated due to some non-covered corner cases), and some of them don't belong to LED DT nodes unfortunately, as e.g. gpio-keys use also ":" as a delimiter in their labels, but the whole set gives reasonable overview I think:

active
activity_led
adsl
alarm
alive
all
amber
app
aux
backup
backup_led
bl
blue
bluetooth
boot
bottom
brick-status
bt
CEL
chrg
COM
copy
core_module
cpu
D
debug
DIA
disk
disk_led
down
dsl
enocean
enter
err
error
esata
ethernet-status
fail
fault
front
func
function
g
ghz
ghz-1
ghz-2
gpio
green
gsm
HD
hdd
hdderr
health
health_led
heart
heartbeat
home
inet
info
internet
keypad
L
lan
led
ledb
left
l_hdd
live
logo
microSD
misc
mmc
nand
network
on
orange
os
panel
pmu_stat
power
power_led
programming
proximitysensor
pulse
pwr
qss
rebuild_led
red
r_hdd
right
router
rs
rx
sata
sata-l
sata-r
sd
SD
sleep
standby
stat
state
status
Status
sw
sys
system
system-status
tel
top
tv
tx
up
usb
USB
usb_1
usb_2
usb_copy
usb-port1
usb-port2
user
USER
usr
wan
white
wifi
wifi_ap
wifi-status
wireless
wlan
wlan_g
wmode
wps
WPS
yellow

maintainer
#14 lore raw

Re: [RESEND PATCH v2 4/6] dt: bindings: as3645a: Improve label documentation, DT example

In reply to Jacek Anaszewski
Quoted 71 lines · Jacek Anaszewski
Quoted 59 lines · Rob Herring
Quoted 50 lines · Jacek Anaszewski

Hi Pavel,

Quoted 47 lines · Pavel Machek
Quoted 27 lines · Sakari Ailus

Hi Pavel,

Quoted 24 lines · Pavel Machek

Hi!

Quoted 7 lines

Specify the exact label used if the label property is omitted in DT, as well as use label in the example that conforms to LED device naming.

(quoted diff) +0 -0
@@ -69,11 +73,11 @@ Example
                         flash-max-microamp = <320000>;
                         led-max-microamp = <60000>;
                         ams,input-max-microamp = <1750000>;

Review Example · line 83

Quoted 10 lines
(quoted diff) +2 -2
-                        label = "as3645a:flash";
+                        label = "as3645a:white:flash";
                 };
                 indicator@1 {
                         reg = <0x1>;
                         led-max-microamp = <10000>;
-                        label = "as3645a:indicator";
+                        label = "as3645a:red:indicator";
                 };
         };

Ok, but userspace still has no chance to determine if this is flash from main camera or flash for front camera; todays smartphones have flashes on both cameras.

Quoted 7 lines
Quoted 2 lines

So.. Can I suggset as3645a:white:main_camera_flash or main_flash or ....?

If there's just a single one in the device, could you use that?

Even if we name this so for N9 (and N900), the application still would only work with the two devices.

Well, I'd plan to name it on other devices, too.

Quoted 3 lines

My suggestion would be to look for a flash LED, and perhaps the maximum current as well. That should generally work better than assumptions on the label.

If you just look for flash LED, you don't know if it is front one or back one. Its true that if you have just one flash it is usually on the back camera, but you can't know if maybe driver is not available for the main flash.

Quoted 7 lines
Quoted 2 lines

Lets get this right, please "main_camera_flash" is 12 bytes more than "flash", and it saves application logic.. more than 12 bytes, I'm sure.

What you are trying to introduce is yet another level of LED class device naming standard, one level below devicename:colour:function. It seems you want also to come up with the set of standarized LED function names. This would certainly have to be covered for consistency.

Quoted 5 lines

I really dislike how this naming convention is used for label. label is supposed to be the phyically identifiable name. Having the devicename defeats that. Perhaps color, too. We'd be better off with a color property. It seems we're overloading the naming with too many things. Now we're adding device association.

Regarding devicename - there is indeed inconsistency in the way how LED DT bindings use label, as some of them use it for defining full LED class device name, and the rest fill only colour and function, leaving addition of a devicename to the driver.

Quoted 8 lines

The problem is also in current definition of label in LED common bindings documentation, which says:

"It has to uniquely identify a device, i.e. no other LED class device can be assigned the same label."

In view of your above words this is not true, and we probably should remove this sentence (it doesn't have DT maintainer ack btw).

Well if label is not unique, then what is the point? It's really just that using the controller is not really meaningful to a user except maybe a devboard that just hooks up LEDs with no defined function.

I can think of a case where you'd have duplicated labels if you had a back plane with multiple identical cards

Quoted 79 lines
Quoted 4 lines

I do want to see standard names though. On 96boards for example, there are defined LEDs and locations. The function on some are defined (e.g. WiFi/BT) and somewhat undefined on others (user{1-4}). I'd like to see the same label across all boards.

Currently we have following LED functions (obtained with grep label Documentation/devicetree/bindings/leds/* | sed s'/^.*label/label/g' | awk -F"=" '{print $2}' | sed '/^$/d' | sed s'/.*:\(.*\)";/\1/' | sed '/^\s\{1,\}/d' | sort -u)

0
1
2
2g
3
4
5
6
7
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
green
indicator
inet
keypad
phone
power
red
sata
sata0
sata1
tel
tv
upgrading
usb
usr0
usr1
usr35
wan
white
wireless
wps
yellow

By extracting numerical pattern names and replacing numbers with N we're getting something like this:

N
Ng
colour
adsl
alarm
alive
aux
broadband
chrg
dsl
flash
indicator
inet
keypad
phone
power
sataN
tel
tv
upgrading
usb
usrN
wan
wireless
wps

Quoted 3 lines

Is this list something you'd like to see as a base of standard LED functions? It seems that this list would have to be continuously supplemented with new positions.

Yes, I think this would be good.

Quoted 4 lines

--
Best regards,
Jacek Anaszewski

#15 lore raw

PATCH RESEND v2 5/6 as3645a: Add colour to LED name

In reply to Sakari Ailus

Add the colour of the LED to the LED name, as specified in Documentation/leds/leds-class.txt.

---
 drivers/leds/leds-as3645a.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/leds/leds-as3645a.c +2 -2
index 605e0c64e974..edeb0a499f6c 100644
@@ -528,7 +528,7 @@ static int as3645a_parse_node(struct as3645a *flash,
 		strlcpy(names->flash, name, sizeof(names->flash));
 	else
 		snprintf(names->flash, sizeof(names->flash),
-			 "%s:flash", node->name);
+			 "%s:white:flash", node->name);
 
 	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
 				    &cfg->flash_timeout_us);
@@ -572,7 +572,7 @@ static int as3645a_parse_node(struct as3645a *flash,
 		strlcpy(names->indicator, name, sizeof(names->indicator));
 	else
 		snprintf(names->indicator, sizeof(names->indicator),
-			 "%s:indicator", node->name);
+			 "%s:red:indicator", node->name);
 
 	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
 				    &cfg->indicator_max_ua);
Signature · 1 line
2.11.0
#16 lore raw

PATCH RESEND v2 6/6 as3645a: Unregister indicator LED on device unbind

In reply to Sakari Ailus

The indicator LED was registered in probe but was not removed in driver remove callback. Fix this.

---
 drivers/leds/leds-as3645a.c | 1 +
 1 file changed, 1 insertion(+)
drivers/leds/leds-as3645a.c +1 -0
index edeb0a499f6c..9494ba26c64b 100644
@@ -743,6 +743,7 @@ static int as3645a_remove(struct i2c_client *client)
 	as3645a_set_control(flash, AS_MODE_EXT_TORCH, false);
 
 	v4l2_flash_release(flash->vf);
+	v4l2_flash_release(flash->vfind);
 
 	led_classdev_flash_unregister(&flash->fled);
 	led_classdev_unregister(&flash->iled_cdev);
Signature · 1 line
2.11.0