[RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Matti Vaittinen]

RFC series improving common LED binding parsing support

Qucik grep for 'for_each' or 'linux,default-trigger' says it all.

Multiple LED controller drivers implement the very similar looping
through the child DT nodes in order to locate the LED nodes and read
and support the common LED dt bindings. Implementing this same
stuff for all LED controllers gets old pretty fast.

This RFC contains 3 suggestions:

Simplest is adding support for parsing the linux,default-trigger,
and default-state DT properties in led-core.

More interesting part is adding correct LED DT node lookup in
LED core. This RFC uses LED DT node names as a 'key' in a same
way regulator framework does for regulators. The thing is that
this approach requires the LED controller binding to dictate allowed
LED node names - which may or may not be doable. I need your help to
evaluate this and suggest better options :) If we still look at the
regulators, the regulator core did originally use "regulator-core"
property to do driver data/DT node pairing - but has since then
changed the approach to using the DT node names.

Last and least clear point is isolating the led_classdev to be owned
by the LED core. Controller drivers should pretty much never touch
it after the initialization. So one approach would be that drivers
only provided initialization data and operations to the core.

The patch series contains the led-core and led-class changes which
introduce (yet another) APIs for registering led class device to
core. Adding new interface is probably not the best option - one
might consider changing the (devm_)led_classdev_register_ext to do
what this new RFC API is doing.

In addition to core changes this series converted two (randomly
selected) existing drivers to use the new API. This can give an
overview how offloading the DT parsing to core could simplify many
of the LED controlled drivers.

Patches HAVE NOT BEEN TESTED other than for compiling. They are
only intended to be a starting point for discussion - and if the
ideas are seen worthy - then the patches should be further worked
and properly tested before being applied.

Matti Vaittinen (5):
  leds: Add common LED binding parsing support to LED class/core
  dt-bindings: an30259a: example for using fixed LED node names.
  leds: an30259a: Offload DT node locating and parsing to core
  dt-bindings: lm3692x: example for using fixed LED node names.
  leds: lm3692x: Offload DT node locating and parsing to core

 .../bindings/leds/leds-an30259a.txt           |   9 +-
 .../devicetree/bindings/leds/leds-lm3692x.txt |   4 +-
 drivers/leds/led-class.c                      | 247 +++++++++++++++++-
 drivers/leds/led-core.c                       | 111 +++++---
 drivers/leds/leds-an30259a.c                  | 181 ++++++-------
 drivers/leds/leds-lm3692x.c                   |  75 +++---
 include/linux/leds.h                          | 144 +++++++++-
 7 files changed, 586 insertions(+), 185 deletions(-)

-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[RFC PATCH 1/5] leds: Add common LED binding parsing support to LED class/core

[Matti Vaittinen]

Qucik grep for 'for_each' or 'linux,default-trigger' or
'default-state' under drivers/leds can tell quite a lot. It seems
multiple LED controller drivers implement the very similar looping
through the child nodes in order to locate the LED nodes and read
and support the common LED dt bindings. Implementing this same
stuff for all LED controllers gets old pretty fast.

This commit adds support for locating the LED node (based on known
node names - this is one RFC item as it may not be optimal) and
parsing of a few common LED DT properties.

This is achieved via new API and old APIs are untouched. Ideally
the new API won't be introduced though but some of the existing
APIs could be converted to do the DT node lookup and parsing.

If no starting point for node lookup is given, (parent) device's
own DT node is used. If no node name match is given, then device's
own node or passed stating point are used as such.

linux,default-trigger,
default-state (with the exception of keep),

(in addition to already handled
function-enumerator,
function,
color
and label).

Additionally this commit gives a nudge towards transferring the
led_classdev ownership from led controller drivers to LED class.
New API no longer allows controller driver to provide the
led_classdev but only the required init_data and operations. In
general most of the drivers should not directly touch the
leds_classdev after it is initialized. LEDs class belongs to,
well, LEDs class.

This commit HAS NOT BEEN TESTED at all. Only compile tested.
This is only RFC - Eg, request for comments. If people see some
of the ideas as useful then properly tested patch can be provided.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/leds/led-class.c | 247 ++++++++++++++++++++++++++++++++++++++-
 drivers/leds/led-core.c  | 111 +++++++++++-------
 include/linux/leds.h     | 144 +++++++++++++++++++++--
 3 files changed, 451 insertions(+), 51 deletions(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 647b1263c579..ba5da3b49010 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -235,6 +235,206 @@ static int led_classdev_next_name(const char *init_name, char *name,
 	return i;
 }
 
+int led_of_required_props_found(struct led_properties *props)
+{
+	/* RFC_NOTE: Do we _require_ function or label if DT is used? */
+	return (props->function || props->label);
+}
+
+static void led_add_props(struct led_classdev *ld, struct led_properties *props)
+{
+	ld->default_trigger = props->default_trigger;
+	/*
+	 * According to binding docs the LED is by-default turned OFF unless
+	 * default_state is used to indicate it should be ON or that state
+	 * should be kept as is
+	 */
+	ld->brightness = LED_OFF;
+	if (props->default_state) {
+		if (!strcmp(props->default_state, "on"))
+			ld->brightness = LED_FULL;
+	/*
+	 * RFC_NOTE:
+	 * Do we need handling for 'keep'? We probably should not call the
+	 * brightness_get prior calling the of_parse_cb if one is provided.
+	 *
+	 * If handling 'keep' is needed then we can add a flag to
+	 * advertice that state should be queried and kept as-is. Else
+	 * the drivers should implement own of_parse_cb just for handling
+	 * the keep - which feels wrong... LED core could/should do it
+	 * automagically if brightness_get is provided.
+	 */
+	}
+}
+/*
+ * RFC_NOTE: Ideally we could have the pointer to a struct led_ops in struct
+ * led_classdev and we could just directly populate it. But I don't want to go
+ * through all the existing drivers for this change now.
+ */
+static void led_add_ops(struct led_classdev *ld, const struct led_ops *ops)
+{
+	ld->brightness_set = ops->brightness_set;
+	ld->brightness_set_blocking = ops->brightness_set_blocking;
+	ld->brightness_get = ops->brightness_get;
+	ld->blink_set = ops->blink_set;
+	ld->pattern_set = ops->pattern_set;
+	ld->pattern_clear = ops->pattern_clear;
+	ld->flash_resume = ops->flash_resume;
+}
+
+/*
+ * RFC_NOTE: Maybe the led drivers should initialize all fields in
+ * led_classdev struct by default. I guess big part of led_classdev should
+ * belong to LED-class and ideally drivers only provide const init data and ops
+ * for the class.
+ *
+ * We should not provide both led_classdev_register_dt and
+ * led_classdev_register_ext. These functions should be merged to do what is
+ * the best approach when we have fwnode.
+ */
+
+/**
+ * led_classdev_register_dt - register LED device with FW node 'match'
+ *
+ * @parent: LED controller device this LED is driven by
+ * @ops: LED control operations for this LED
+ * @init_data: the LED class device initialization data
+ *
+ * Returns a handle to use for unregistering or error ptr.
+ */
+void *led_classdev_register_dt(struct device *parent,
+				const struct led_ops *ops,
+				struct led_init_data *init_data)
+{
+	char composed_name[LED_MAX_NAME_SIZE];
+	char final_name[LED_MAX_NAME_SIZE];
+	const char *proposed_name = composed_name;
+	int ret;
+	struct led_properties props = {};
+	struct led_classdev *ld;
+	struct fwnode_handle *fw;
+
+	if (!init_data || !ops)
+		return ERR_PTR(-EINVAL);
+
+	if (init_data->devname_mandatory && !init_data->devicename) {
+		dev_err(parent, "Mandatory device name is missing");
+		return ERR_PTR(-EINVAL);
+	}
+
+	fw = led_find_fwnode(parent, init_data);
+	if (!fw) {
+		dev_err(parent, "No fwnode found\n");
+		return ERR_PTR(-ENOENT);
+	}
+
+	ret = led_parse_fwnode_props(parent, fw, &props);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	ret = led_of_required_props_found(&props);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ld = devm_kzalloc(parent, sizeof(*ld), GFP_KERNEL);
+	if (!ld)
+		return ERR_PTR(-ENOMEM);
+
+	ret = led_compose_name(parent, init_data, &props, composed_name);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	ret = led_classdev_next_name(proposed_name, final_name,
+				     sizeof(final_name));
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	/*
+	 * We don't want each driver to need to travel through the DT nodes in
+	 * order to locate correct. Call driver's fw node parsing callback and
+	 * pass it the node we already located if callback has been registered.
+	 * That way driver can easily skim through anydriver specific properties
+	 * it may have.
+	 *
+	 * Also add pointer to init_data so that driver can embed
+	 * it to it's own private data and obtain private data using offset_of.
+	 * This is needed if the driver no longer posses the led_classdev.
+	 */
+	ld->init_data = init_data;
+	if (init_data->of_parse_cb)
+		ret = init_data->of_parse_cb(ld, fw, &props);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	led_add_props(ld, &props);
+	led_add_ops(ld, ops);
+
+	mutex_init(&ld->led_access);
+	mutex_lock(&ld->led_access);
+	ld->dev = device_create_with_groups(leds_class, parent, 0,
+				ld, ld->groups, "%s", final_name);
+	if (IS_ERR(ld->dev)) {
+		mutex_unlock(&ld->led_access);
+		return ld->dev;
+	}
+	if (init_data && init_data->fwnode)
+		ld->dev->fwnode = fw;
+
+	if (ret)
+		dev_warn(parent, "Led %s renamed to %s due to name collision",
+				ld->name, dev_name(ld->dev));
+
+	if (ld->flags & LED_BRIGHT_HW_CHANGED) {
+		ret = led_add_brightness_hw_changed(ld);
+		if (ret) {
+			device_unregister(ld->dev);
+			ld->dev = NULL;
+			mutex_unlock(&ld->led_access);
+			return ERR_PTR(ret);
+		}
+	}
+
+	ld->work_flags = 0;
+#ifdef CONFIG_LEDS_TRIGGERS
+	init_rwsem(&ld->trigger_lock);
+#endif
+#ifdef CONFIG_LEDS_BRIGHTNESS_HW_CHANGED
+	ld->brightness_hw_changed = -1;
+#endif
+	/* add to the list of leds */
+	down_write(&leds_list_lock);
+	list_add_tail(&ld->node, &leds_list);
+	up_write(&leds_list_lock);
+
+	if (!ld->max_brightness)
+		ld->max_brightness = LED_FULL;
+
+	led_update_brightness(ld);
+
+	led_init_core(ld);
+
+#ifdef CONFIG_LEDS_TRIGGERS
+	led_trigger_set_default(ld);
+#endif
+
+	mutex_unlock(&ld->led_access);
+
+	dev_dbg(parent, "Registered led device: %s\n",
+			ld->name);
+
+	return ld;
+}
+EXPORT_SYMBOL_GPL(led_classdev_register_dt);
+
+void led_classdecv_unregister_dt(void *handle)
+{
+	return led_classdev_unregister((struct led_classdev *)handle);
+}
+EXPORT_SYMBOL_GPL(led_classdecv_unregister_dt);
+/*
+ * RFC_NOTE: We could perhaps merge this with 'led_classdev_register_dt'
+ * I don't think we should have two interfaces for same thing in the long run.
+ */
 /**
  * led_classdev_register_ext - register a new object of led_classdev class
  *			       with init data.
@@ -251,13 +451,17 @@ int led_classdev_register_ext(struct device *parent,
 	char final_name[LED_MAX_NAME_SIZE];
 	const char *proposed_name = composed_name;
 	int ret;
+	struct led_properties props = {};
 
 	if (init_data) {
 		if (init_data->devname_mandatory && !init_data->devicename) {
 			dev_err(parent, "Mandatory device name is missing");
 			return -EINVAL;
 		}
-		ret = led_compose_name(parent, init_data, composed_name);
+		led_parse_fwnode_props(parent, init_data->fwnode, &props);
+
+		ret = led_compose_name(parent, init_data, &props,
+				       composed_name);
 		if (ret < 0)
 			return ret;
 	} else {
@@ -370,6 +574,47 @@ static void devm_led_classdev_release(struct device *dev, void *res)
 	led_classdev_unregister(*(struct led_classdev **)res);
 }
 
+/**
+ * devm_led_classdev_register_dt - resource managed led_classdev_register_dt()
+ *
+ * @parent: parent of LED device
+ * @ops: the led_device operations
+ * @init_data: LED class device initialization data
+ */
+void *devm_led_classdev_register_dt(struct device *parent,
+				     const struct led_ops *ops,
+				     struct led_init_data *init_data)
+{
+	void **ptr, *handle;
+
+	ptr = devres_alloc(devm_led_classdev_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	handle = led_classdev_register_dt(parent, ops, init_data);
+	if (!IS_ERR(handle)) {
+		*ptr = handle;
+		devres_add(parent, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return handle;
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_register_dt);
+
+/**
+ * devm_led_classdev_unregister_dt - resource managed led_classdev_unregister_dt
+ * @dev: The device to unregister.
+ * @handle: The handle returned by devm_led_classdev_register_dt.
+ */
+void devm_led_classdev_unregister_dt(struct device *dev,
+				     void *handle)
+{
+	devm_led_classdev_unregister(dev, handle);
+}
+EXPORT_SYMBOL_GPL(devm_led_classdev_unregister_dt);
+
 /**
  * devm_led_classdev_register_ext - resource managed led_classdev_register_ext()
  *
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..d6217e807e70 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -365,70 +365,99 @@ void led_sysfs_enable(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_sysfs_enable);
 
-static void led_parse_fwnode_props(struct device *dev,
-				   struct fwnode_handle *fwnode,
-				   struct led_properties *props)
+int led_parse_fwnode_props(struct device *dev, struct fwnode_handle *fwnode,
+			   struct led_properties *props)
 {
-	int ret;
+	int ret = 0;
 
 	if (!fwnode)
-		return;
+		return -EINVAL;
 
 	if (fwnode_property_present(fwnode, "label")) {
 		ret = fwnode_property_read_string(fwnode, "label", &props->label);
 		if (ret)
 			dev_err(dev, "Error parsing 'label' property (%d)\n", ret);
-		return;
+		return ret;
 	}
 
+	/**
+	 * Please note, logic changed - if invalid property is found we bail
+	 * early out without parsing the rest of the properties. Originally
+	 * this was the case only for 'label' property. I don't know the
+	 * rationale behind original logic allowing invalid properties to be
+	 * given. If there is a reason then we should reconsider this.
+	 * Intuitively it feels correct to just yell and quit if we hit value we
+	 * don't understand - but intuition may be wrong at times :)
+	 */
 	if (fwnode_property_present(fwnode, "color")) {
 		ret = fwnode_property_read_u32(fwnode, "color", &props->color);
-		if (ret)
+		if (ret) {
 			dev_err(dev, "Error parsing 'color' property (%d)\n", ret);
-		else if (props->color >= LED_COLOR_ID_MAX)
+			return ret;
+		} else if (props->color >= LED_COLOR_ID_MAX) {
 			dev_err(dev, "LED color identifier out of range\n");
-		else
-			props->color_present = true;
+			return ret;
+		}
+		props->color_present = true;
 	}
 
+	if (fwnode_property_present(fwnode, "function")) {
+		ret = fwnode_property_read_string(fwnode, "function",
+						  &props->function);
+		if (ret) {
+			dev_err(dev,
+				"Error parsing 'function' property (%d)\n",
+				ret);
+			return ret;
+		}
+	}
 
-	if (!fwnode_property_present(fwnode, "function"))
-		return;
-
-	ret = fwnode_property_read_string(fwnode, "function", &props->function);
-	if (ret) {
-		dev_err(dev,
-			"Error parsing 'function' property (%d)\n",
-			ret);
+	if (fwnode_property_present(fwnode, "function-enumerator")) {
+		ret = fwnode_property_read_u32(fwnode, "function-enumerator",
+					       &props->func_enum);
+		if (ret) {
+			dev_err(dev,
+				"Error parsing 'function-enumerator' property (%d)\n",
+				ret);
+			return ret;
+		}
+		props->func_enum_present = true;
 	}
 
-	if (!fwnode_property_present(fwnode, "function-enumerator"))
-		return;
+	if (fwnode_property_present(fwnode, "default-state")) {
+		ret = fwnode_property_read_string(fwnode, "default-state",
+						  &props->default_state);
+		if (ret) {
+			dev_err(dev,
+				"Error parsing 'default-state' property (%d)\n",
+				ret);
+			return ret;
+		}
+	}
 
-	ret = fwnode_property_read_u32(fwnode, "function-enumerator",
-				       &props->func_enum);
-	if (ret) {
-		dev_err(dev,
-			"Error parsing 'function-enumerator' property (%d)\n",
-			ret);
-	} else {
-		props->func_enum_present = true;
+	if (fwnode_property_present(fwnode, "linux,default-trigger")) {
+		ret = fwnode_property_read_string(fwnode,
+						  "linux,default-trigger",
+						  &props->default_trigger);
+		if (ret)
+			dev_err(dev,
+				"Error parsing 'linux,default-trigger' property (%d)\n",
+				ret);
 	}
+	return ret;
 }
+EXPORT_SYMBOL_GPL(led_parse_fwnode_props);
 
 int led_compose_name(struct device *dev, struct led_init_data *init_data,
-		     char *led_classdev_name)
+		     struct led_properties *props, char *led_classdev_name)
 {
-	struct led_properties props = {};
 	struct fwnode_handle *fwnode = init_data->fwnode;
 	const char *devicename = init_data->devicename;
 
 	if (!led_classdev_name)
 		return -EINVAL;
 
-	led_parse_fwnode_props(dev, fwnode, &props);
-
-	if (props.label) {
+	if (props->label) {
 		/*
 		 * If init_data.devicename is NULL, then it indicates that
 		 * DT label should be used as-is for LED class device name.
@@ -436,23 +465,23 @@ int led_compose_name(struct device *dev, struct led_init_data *init_data,
 		 * the final LED class device name.
 		 */
 		if (!devicename) {
-			strscpy(led_classdev_name, props.label,
+			strscpy(led_classdev_name, props->label,
 				LED_MAX_NAME_SIZE);
 		} else {
 			snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
-				 devicename, props.label);
+				 devicename, props->label);
 		}
-	} else if (props.function || props.color_present) {
+	} else if (props->function || props->color_present) {
 		char tmp_buf[LED_MAX_NAME_SIZE];
 
-		if (props.func_enum_present) {
+		if (props->func_enum_present) {
 			snprintf(tmp_buf, LED_MAX_NAME_SIZE, "%s:%s-%d",
-				 props.color_present ? led_colors[props.color] : "",
-				 props.function ?: "", props.func_enum);
+				 props->color_present ? led_colors[props->color] : "",
+				 props->function ?: "", props->func_enum);
 		} else {
 			snprintf(tmp_buf, LED_MAX_NAME_SIZE, "%s:%s",
-				 props.color_present ? led_colors[props.color] : "",
-				 props.function ?: "");
+				 props->color_present ? led_colors[props->color] : "",
+				 props->function ?: "");
 		}
 		if (init_data->devname_mandatory) {
 			snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
diff --git a/include/linux/leds.h b/include/linux/leds.h
index efb309dba914..bf254f271e31 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -13,6 +13,7 @@
 #include <linux/kernfs.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/rwsem.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -20,6 +21,7 @@
 
 struct device;
 struct led_pattern;
+struct led_classdev;
 /*
  * LED Core
  */
@@ -31,7 +33,34 @@ enum led_brightness {
 	LED_FULL	= 255,
 };
 
+struct led_properties {
+	u32		color;
+	bool		color_present;
+	const char	*function;
+	u32		func_enum;
+	bool		func_enum_present;
+	const char	*label;
+	const char	*default_trigger;
+	const char	*default_state;
+};
+
+struct led_ops {
+	void (*brightness_set)(struct led_classdev *led_cdev,
+			       enum led_brightness brightness);
+	int (*brightness_set_blocking)(struct led_classdev *led_cdev,
+				       enum led_brightness brightness);
+	enum led_brightness (*brightness_get)(struct led_classdev *led_cdev);
+	int (*blink_set)(struct led_classdev *led_cdev,
+			 unsigned long *delay_on,
+			 unsigned long *delay_off);
+	int (*pattern_set)(struct led_classdev *led_cdev,
+			   struct led_pattern *pattern, u32 len, int repeat);
+	int (*pattern_clear)(struct led_classdev *led_cdev);
+	void (*flash_resume)(struct led_classdev *led_cdev);
+};
+
 struct led_init_data {
+	const char *of_match;
 	/* device fwnode handle */
 	struct fwnode_handle *fwnode;
 	/*
@@ -53,9 +82,31 @@ struct led_init_data {
 	 * set it to true
 	 */
 	bool devname_mandatory;
+	/*
+	 * RFC_NOTE: if led_classdev_register_ext is merged with
+	 * led_classdev_register_dt - then led_classdev_register_ext should
+	 * invoke this too.
+	 */
+	/*
+	 * Callback which a LED driver can register if it has own non-standard
+	 * DT properties. Core calls this with the located DT node during
+	 * class_device registration when led_classdev_register_dt API is used
+	 */
+	int (*of_parse_cb)(struct led_classdev *ld, struct fwnode_handle *fw,
+			    struct led_properties *props);
+	/*
+	 * RFC_NOTE: init data should probably also contain the flags entry
+	 * for some of the flags - but I don't think we should add all of them
+	 * right away - rather add new things to init data 'as needed' basis?
+	 *
+	 * And if not all of the flags are meant for drivers (but for core) -
+	 * then we might want to add independent bool entities rather than
+	 * 'flags' member here?
+	 */
 };
 
 struct led_classdev {
+	struct led_init_data	*init_data;
 	const char		*name;
 	enum led_brightness	 brightness;
 	enum led_brightness	 max_brightness;
@@ -150,6 +201,32 @@ struct led_classdev {
 	struct mutex		led_access;
 };
 
+/**
+ * led_classdev_register_dt - register LED device with FW node 'match'
+ *
+ * @parent: LED controller device this LED is driven by
+ * @ops: LED control operations for this LED
+ * @init_data: the LED class device initialization data
+ *
+ * Returns a handle to use for unregistering or error ptr.
+ */
+void *led_classdev_register_dt(struct device *parent,
+			       const struct led_ops *ops,
+			       struct led_init_data *init_data);
+
+/**
+ * devm_led_classdev_register_dt - resource managed led_classdev_register_dt()
+ *
+ * @parent: parent of LED device
+ * @ops: the led_device operations
+ * @init_data: LED class device initialization data
+ */
+void *devm_led_classdev_register_dt(struct device *parent,
+				    const struct led_ops *ops,
+				    struct led_init_data *init_data);
+
+void led_classdecv_unregister_dt(void *handle);
+void devm_led_classdev_unregister_dt(struct device *dev, void *handle);
 /**
  * led_classdev_register_ext - register a new object of LED class with
  *			       init data
@@ -302,6 +379,7 @@ extern void led_sysfs_enable(struct led_classdev *led_cdev);
  * led_compose_name - compose LED class device name
  * @dev: LED controller device object
  * @init_data: the LED class device initialization data
+ * @props: LED properties as parsed from fwnode.
  * @led_classdev_name: composed LED class device name
  *
  * Create LED class device name basing on the provided init_data argument.
@@ -311,6 +389,7 @@ extern void led_sysfs_enable(struct led_classdev *led_cdev);
  * Returns: 0 on success or negative error value on failure
  */
 extern int led_compose_name(struct device *dev, struct led_init_data *init_data,
+			    struct led_properties *props,
 			    char *led_classdev_name);
 
 /**
@@ -324,6 +403,62 @@ static inline bool led_sysfs_is_disabled(struct led_classdev *led_cdev)
 	return led_cdev->flags & LED_SYSFS_DISABLE;
 }
 
+/**
+ * led_find_fwnode - find fwnode matching given LED
+ * @parent: LED controller device this LED is driven by
+ * @init_data: the LED class device initialization data
+ *
+ * Find the fw node matching given LED init data.
+ *
+ * Returns: node handle or NULL if matching fw node was not found
+ */
+static inline struct fwnode_handle *led_find_fwnode(struct device *parent,
+						struct led_init_data *init_data)
+{
+	struct fwnode_handle *fw;
+
+	if (!init_data)
+		return dev_fwnode(parent);
+
+	if (!init_data->fwnode)
+		fw = dev_fwnode(parent);
+	else
+		fw = init_data->fwnode;
+
+	/*
+	 * Simple things are pretty. I think simplest is to use DT node-name
+	 * for matching the node with LED - same way regulators use the node
+	 * name to match with desc.
+	 *
+	 * This may not work with existing LED DT entries if the node name has
+	 * been freely selectible. In order to this to work the binding doc
+	 * for LED driver should define usable node names. This may also be
+	 * a problem if LED node name should reflect the actual LED HW (not
+	 * LED controller HW) because the LED controller binding should should
+	 * define the acceptable names. So maybe we should consider some other
+	 * matching mechanism? This function can do the matching anyways as
+	 * init data should contain some 'match' for DT - whether it is node
+	 * name or specific property needs to be evaluated.
+	 */
+	if (init_data->of_match)
+		fw = fwnode_get_named_child_node(fw, init_data->of_match);
+
+	return fw;
+}
+
+/**
+ * led_parse_fwnode_props - parse LED common properties from fwnode
+ * @dev: pointer to LED device.
+ * @fwnode: LED node containing the properties
+ * @props: structure where found property data is stored.
+ *
+ * Parse the common LED properties from fwnode.
+ *
+ * Returns: 0 on success or negative error value on failure
+ */
+int led_parse_fwnode_props(struct device *dev, struct fwnode_handle *fwnode,
+			   struct led_properties *props);
+
 /*
  * LED Triggers
  */
@@ -490,15 +625,6 @@ struct led_platform_data {
 	struct led_info	*leds;
 };
 
-struct led_properties {
-	u32		color;
-	bool		color_present;
-	const char	*function;
-	u32		func_enum;
-	bool		func_enum_present;
-	const char	*label;
-};
-
 struct gpio_desc;
 typedef int (*gpio_blink_set_t)(struct gpio_desc *desc, int state,
 				unsigned long *delay_on,
-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[RFC PATCH 2/5] dt-bindings: an30259a: example for using fixed LED node names.

[Matti Vaittinen]

Use predefined LED node name to match the LED node in driver.

It would be nice to offload common LED property parsing to
LED core driver. One of the key things to allow this is somehow
'pair' the LED DT node with LED driver initialization data.

This patch uses LED node name as a 'key' in a same fashion
as regulators do. The an30259a was selected as demonstration
example and this change may not be really feasible for an30259a
as I have no idea whether the existing DTs for devices out there
have specific node names (or can be changed). This servers just
as an example to initiate discussion as to how we could pair the
driver data and DT node.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
index cbd833906b2b..bd1a2d11a0ad 100644
--- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
+++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
@@ -9,7 +9,8 @@ Required properties:
 	- #address-cells: Must be 1.
 	- #size-cells: Must be 0.
 
-Each LED is represented as a sub-node of the panasonic,an30259a node.
+Each LED is represented as a sub-node of the panasonic,an30259a node. LED nodes
+must be named as led1 led2 and led3.
 
 Required sub-node properties:
 	- reg: Pin that the LED is connected to. Must be 1, 2, or 3.
@@ -34,20 +35,20 @@ led-controller@30 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 
-	led@1 {
+	led1 {
 		reg = <1>;
 		linux,default-trigger = "heartbeat";
 		function = LED_FUNCTION_INDICATOR;
 		color = <LED_COLOR_ID_RED>;
 	};
 
-	led@2 {
+	led2 {
 		reg = <2>;
 		function = LED_FUNCTION_INDICATOR;
 		color = <LED_COLOR_ID_GREEN>;
 	};
 
-	led@3 {
+	led3 {
 		reg = <3>;
 		function = LED_FUNCTION_INDICATOR;
 		color = <LED_COLOR_ID_BLUE>;
-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Re: [RFC PATCH 2/5] dt-bindings: an30259a: example for using fixed LED node names.

[Rob Herring]

On Tue, Oct 29, 2019 at 02:47:26PM +0200, Matti Vaittinen wrote:
> Use predefined LED node name to match the LED node in driver.
> 
> It would be nice to offload common LED property parsing to
> LED core driver. One of the key things to allow this is somehow
> 'pair' the LED DT node with LED driver initialization data.
> 
> This patch uses LED node name as a 'key' in a same fashion
> as regulators do. The an30259a was selected as demonstration
> example and this change may not be really feasible for an30259a
> as I have no idea whether the existing DTs for devices out there
> have specific node names (or can be changed). This servers just
> as an example to initiate discussion as to how we could pair the
> driver data and DT node.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-an30259a.txt b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> index cbd833906b2b..bd1a2d11a0ad 100644
> --- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> @@ -9,7 +9,8 @@ Required properties:
>  	- #address-cells: Must be 1.
>  	- #size-cells: Must be 0.
>  
> -Each LED is represented as a sub-node of the panasonic,an30259a node.
> +Each LED is represented as a sub-node of the panasonic,an30259a node. LED nodes
> +must be named as led1 led2 and led3.
>  
>  Required sub-node properties:
>  	- reg: Pin that the LED is connected to. Must be 1, 2, or 3.
> @@ -34,20 +35,20 @@ led-controller@30 {
>  	#address-cells = <1>;
>  	#size-cells = <0>;
>  
> -	led@1 {
> +	led1 {
>  		reg = <1>;

This is wrong. reg requires a unit-address and vice-versa.

>  		linux,default-trigger = "heartbeat";
>  		function = LED_FUNCTION_INDICATOR;
>  		color = <LED_COLOR_ID_RED>;
>  	};
>  
> -	led@2 {
> +	led2 {
>  		reg = <2>;
>  		function = LED_FUNCTION_INDICATOR;
>  		color = <LED_COLOR_ID_GREEN>;
>  	};
>  
> -	led@3 {
> +	led3 {
>  		reg = <3>;
>  		function = LED_FUNCTION_INDICATOR;
>  		color = <LED_COLOR_ID_BLUE>;
> -- 
> 2.21.0
> 
> 
> -- 
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
> 
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
> Simon says - in Latin please.
> ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> Thanks to Simon Glass for the translation =] 

Re: SPAM (R/EU IT) // Re: [RFC PATCH 2/5] dt-bindings: an30259a: example for using fixed LED node names.

[Vaittinen, Matti]

Hello Rob & All,

On Tue, 2019-11-05 at 21:42 -0600, Rob Herring wrote:
> On Tue, Oct 29, 2019 at 02:47:26PM +0200, Matti Vaittinen wrote:
> > Use predefined LED node name to match the LED node in driver.
> > 
> > It would be nice to offload common LED property parsing to
> > LED core driver. One of the key things to allow this is somehow
> > 'pair' the LED DT node with LED driver initialization data.
> > 
> > This patch uses LED node name as a 'key' in a same fashion
> > as regulators do. The an30259a was selected as demonstration
> > example and this change may not be really feasible for an30259a
> > as I have no idea whether the existing DTs for devices out there
> > have specific node names (or can be changed). This servers just
> > as an example to initiate discussion as to how we could pair the
> > driver data and DT node.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> >  Documentation/devicetree/bindings/leds/leds-an30259a.txt | 9
> > +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-
> > an30259a.txt b/Documentation/devicetree/bindings/leds/leds-
> > an30259a.txt
> > index cbd833906b2b..bd1a2d11a0ad 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-an30259a.txt
> > @@ -9,7 +9,8 @@ Required properties:
> >  	- #address-cells: Must be 1.
> >  	- #size-cells: Must be 0.
> >  
> > -Each LED is represented as a sub-node of the panasonic,an30259a
> > node.
> > +Each LED is represented as a sub-node of the panasonic,an30259a
> > node. LED nodes
> > +must be named as led1 led2 and led3.
> >  
> >  Required sub-node properties:
> >  	- reg: Pin that the LED is connected to. Must be 1, 2, or 3.
> > @@ -34,20 +35,20 @@ led-controller@30 {
> >  	#address-cells = <1>;
> >  	#size-cells = <0>;
> >  
> > -	led@1 {
> > +	led1 {
> >  		reg = <1>;
> 
> This is wrong. reg requires a unit-address and vice-versa.

Right.

I'd be interested to know how using node name(s) to match the driver
data (like regulators do) is seen in general? Is this a bad idea for
LEDs? Would it be better to have a leds-compatible (like we had
regulator-compatible before)?

If node names can be used - would

	led1@1 {

	};

	led2@2 {

	};
	... do?

Using node names as key requires them to be unique. Other option could
be doing the search based on node name and unit-address combination - 
but if unit-address depends on board the led is placed - then driver
might not know it. Should I convert the RFC to introduce and use led-
compatible?

> >  		linux,default-trigger = "heartbeat";
> >  		function = LED_FUNCTION_INDICATOR;
> >  		color = <LED_COLOR_ID_RED>;
> >  	};
> >  
> > -	led@2 {
> > +	led2 {
> >  		reg = <2>;
> >  		function = LED_FUNCTION_INDICATOR;
> >  		color = <LED_COLOR_ID_GREEN>;
> >  	};
> >  
> > -	led@3 {
> > +	led3 {
> >  		reg = <3>;
> >  		function = LED_FUNCTION_INDICATOR;
> >  		color = <LED_COLOR_ID_BLUE>;
> > -- 
> > 2.21.0
> > 
> > 
> > -- 
> > Matti Vaittinen, Linux device drivers
> > ROHM Semiconductors, Finland SWDC
> > Kiviharjunlenkki 1E
> > 90220 OULU
> > FINLAND
> > 
> > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > ~~~
> > Simon says - in Latin please.
> > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> > Thanks to Simon Glass for the translation =] 

[RFC PATCH 3/5] leds: an30259a: Offload DT node locating and parsing to core

[Matti Vaittinen]

This comment serves as an example how led controller drivers
could be simplified if led-class/led-core would handle DT node
locating and parsing. leds-an30259a was randomly selected from
drivers using 'devm_led_classdev_register_ext' API. No further
study was done :)

This commit HAS NOT BEEN TESTED at all. Only compile tested.
This is only RFC - Eg, request for comments. If people see some
of the ideas as useful then properly tested patch should be
provided.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/leds/leds-an30259a.c | 181 +++++++++++++++++------------------
 1 file changed, 87 insertions(+), 94 deletions(-)

diff --git a/drivers/leds/leds-an30259a.c b/drivers/leds/leds-an30259a.c
index 250dc9d6f635..3df91866d6a2 100644
--- a/drivers/leds/leds-an30259a.c
+++ b/drivers/leds/leds-an30259a.c
@@ -61,10 +61,33 @@
 
 struct an30259a;
 
+/*
+ * RFC_NOTE: What would be the correct way to match the DT node with driver?
+ * This may not work for leds-an30259a.c as the DT entries may not be exactly
+ * what is used in dt-binding example. For _new_ drivers we could either fix the
+ * node names (?) can we or introduce some "led-compatible" property (like
+ * regulators had regulator-compatible prior switching to node-names). But if we
+ * wish to convert old drivers to use this new API (I suggested merging the
+ * led_classdev_register_ext and led_classdev_register_dt into one in order to
+ * not create yet another LED class registration API) we may need to add some
+ * of_match_cb call-back in led_init_data so that old drivers can implement own
+ * DT matching mechanism and LED core just calls that for each child node. I'd
+ * prefer not having such as ideally drivers should not need to care about DT
+ * nodes unless they have driver specific properties. Core should handle generic
+ * LED properties.
+ */
+static const char *of_led_names[AN30259A_MAX_LEDS] = {
+	"led1", "led2", "led3",
+};
+
 struct an30259a_led {
 	struct an30259a *chip;
 	struct fwnode_handle *fwnode;
-	struct led_classdev cdev;
+/*
+ * RFC_NOTE: We give ownership of led_classdev to LED core. LED driver should
+ * not really need it for anything?
+ */
+	struct led_init_data init_data;
 	u32 num;
 	u32 default_state;
 	bool sloping;
@@ -85,7 +108,7 @@ static int an30259a_brightness_set(struct led_classdev *cdev,
 	int ret;
 	unsigned int led_on;
 
-	led = container_of(cdev, struct an30259a_led, cdev);
+	led = container_of(cdev->init_data, struct an30259a_led, init_data);
 	mutex_lock(&led->chip->mutex);
 
 	ret = regmap_read(led->chip->regmap, AN30259A_REG_LED_ON, &led_on);
@@ -132,7 +155,7 @@ static int an30259a_blink_set(struct led_classdev *cdev,
 	unsigned int led_on;
 	unsigned long off = *delay_off, on = *delay_on;
 
-	led = container_of(cdev, struct an30259a_led, cdev);
+	led = container_of(cdev->init_data, struct an30259a_led, init_data);
 
 	mutex_lock(&led->chip->mutex);
 	num = led->num;
@@ -199,56 +222,76 @@ static int an30259a_blink_set(struct led_classdev *cdev,
 	return ret;
 }
 
-static int an30259a_dt_init(struct i2c_client *client,
-			    struct an30259a *chip)
+static int of_check_reg(struct led_classdev *ld, struct fwnode_handle *fw,
+			struct led_properties *props)
 {
-	struct device_node *np = client->dev.of_node, *child;
-	int count, ret;
-	int i = 0;
+	u32 source;
+	int ret;
+	struct an30259a *chip;
 	const char *str;
-	struct an30259a_led *led;
+	unsigned int led_on;
+	struct an30259a_led *led = container_of(ld->init_data,
+						struct an30259a_led, init_data);
 
-	count = of_get_child_count(np);
-	if (!count || count > AN30259A_MAX_LEDS)
+	chip = led->chip;
+
+	ret = fwnode_property_read_u32(fw, "reg", &source);
+	if (ret != 0 || !source || source > AN30259A_MAX_LEDS) {
+		dev_err(&chip->client->dev, "Couldn't read LED address: %d\n",
+			ret);
 		return -EINVAL;
+	}
+	led->num = source;
+	chip->num_leds++;
+
+	/*
+	 * RFC_NOTE: We need to add default-state = "keep" handling here
+	 * if we don't implement get_brightness and keep support in core
+	 */
+	if (!fwnode_property_read_string(fw, "default-state", &str)) {
+		if (!strcmp(str, "keep"))
+			ret = regmap_read(chip->regmap, AN30259A_REG_LED_ON,
+					  &led_on);
+		if (ret)
+			return ret;
 
-	for_each_available_child_of_node(np, child) {
-		u32 source;
+		if (!(led_on & AN30259A_LED_EN(led->num)))
+			ld->brightness = LED_OFF;
+		else
+			regmap_read(chip->regmap, AN30259A_REG_LEDCC(led->num),
+			    &ld->brightness);
+	}
 
-		ret = of_property_read_u32(child, "reg", &source);
-		if (ret != 0 || !source || source > AN30259A_MAX_LEDS) {
-			dev_err(&client->dev, "Couldn't read LED address: %d\n",
-				ret);
-			count--;
-			continue;
-		}
+	return 0;
+}
 
-		led = &chip->leds[i];
+const static struct led_ops an30259a_ops = {
+	.brightness_set_blocking = an30259a_brightness_set,
+	.blink_set = an30259a_blink_set,
+};
 
-		led->num = source;
-		led->chip = chip;
-		led->fwnode = of_fwnode_handle(child);
+static int an30259a_dt_init(struct i2c_client *client,
+			    struct an30259a *chip)
+{
+	void *ret;
+	int i = 0;
+	struct an30259a_led *led;
 
-		if (!of_property_read_string(child, "default-state", &str)) {
-			if (!strcmp(str, "on"))
-				led->default_state = STATE_ON;
-			else if (!strcmp(str, "keep"))
-				led->default_state = STATE_KEEP;
-			else
-				led->default_state = STATE_OFF;
-		}
+	for (i = 0; i < AN30259A_MAX_LEDS; i++) {
+		led = &chip->leds[i];
 
-		of_property_read_string(child, "linux,default-trigger",
-					&led->cdev.default_trigger);
+		led->init_data.of_match = of_match_ptr(of_led_names[i]);
+		led->init_data.devicename = AN30259A_NAME;
+		led->init_data.default_label = ":";
+		led->init_data.of_parse_cb = of_check_reg;
+		led->chip = chip;
 
-		i++;
+		ret = devm_led_classdev_register_dt(&client->dev, &an30259a_ops,
+						 &led->init_data);
+		if (IS_ERR(ret))
+			return PTR_ERR(ret);
 	}
 
-	if (!count)
-		return -EINVAL;
-
-	chip->num_leds = i;
-
 	return 0;
 }
 
@@ -258,75 +301,25 @@ static const struct regmap_config an30259a_regmap_config = {
 	.max_register = AN30259A_REG_MAX,
 };
 
-static void an30259a_init_default_state(struct an30259a_led *led)
-{
-	struct an30259a *chip = led->chip;
-	int led_on, err;
-
-	switch (led->default_state) {
-	case STATE_ON:
-		led->cdev.brightness = LED_FULL;
-		break;
-	case STATE_KEEP:
-		err = regmap_read(chip->regmap, AN30259A_REG_LED_ON, &led_on);
-		if (err)
-			break;
-
-		if (!(led_on & AN30259A_LED_EN(led->num))) {
-			led->cdev.brightness = LED_OFF;
-			break;
-		}
-		regmap_read(chip->regmap, AN30259A_REG_LEDCC(led->num),
-			    &led->cdev.brightness);
-		break;
-	default:
-		led->cdev.brightness = LED_OFF;
-	}
-
-	an30259a_brightness_set(&led->cdev, led->cdev.brightness);
-}
-
 static int an30259a_probe(struct i2c_client *client)
 {
 	struct an30259a *chip;
-	int i, err;
+	int err;
 
 	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
-	err = an30259a_dt_init(client, chip);
-	if (err < 0)
-		return err;
-
 	mutex_init(&chip->mutex);
 	chip->client = client;
 	i2c_set_clientdata(client, chip);
 
 	chip->regmap = devm_regmap_init_i2c(client, &an30259a_regmap_config);
 
-	for (i = 0; i < chip->num_leds; i++) {
-		struct led_init_data init_data = {};
-
-		an30259a_init_default_state(&chip->leds[i]);
-		chip->leds[i].cdev.brightness_set_blocking =
-			an30259a_brightness_set;
-		chip->leds[i].cdev.blink_set = an30259a_blink_set;
-
-		init_data.fwnode = chip->leds[i].fwnode;
-		init_data.devicename = AN30259A_NAME;
-		init_data.default_label = ":";
-
-		err = devm_led_classdev_register_ext(&client->dev,
-						 &chip->leds[i].cdev,
-						 &init_data);
-		if (err < 0)
-			goto exit;
-	}
-	return 0;
+	err = an30259a_dt_init(client, chip);
+	if (err)
+		mutex_destroy(&chip->mutex);
 
-exit:
-	mutex_destroy(&chip->mutex);
 	return err;
 }
 
-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[RFC PATCH 4/5] dt-bindings: lm3692x: example for using fixed LED node names.

[Matti Vaittinen]

Use predefined LED node name to match the LED node in driver.

It would be nice to offload common LED property parsing to
LED core driver. One of the key things to allow this is somehow
'pair' the LED DT node with LED driver initialization data.

This patch uses LED node name as a 'key' in a same fashion
as regulators do. The lm3692x was selected as demonstration
example and this change is not intended to be feasible as such
(surprize =]) This servers just as an example to initiate
discussion as to how (if) we could pair the driver data and DT
node.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 Documentation/devicetree/bindings/leds/leds-lm3692x.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
index 4c2d923f8758..03866d491d01 100644
--- a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
@@ -5,7 +5,7 @@ white-LED driver designed for LCD display backlighting.
 
 The main difference between the LM36922 and LM36923 is the number of
 LED strings it supports.  The LM36922 supports two strings while the LM36923
-supports three strings.
+supports three strings. LED sub-node must be named as "led_node_name_here".
 
 Required properties:
 	- compatible:
@@ -45,7 +45,7 @@ led-controller@36 {
 	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
 	vled-supply = <&vbatt>;
 
-	led@0 {
+	led_node_name_here {
 		reg = <0>;
 		function = LED_FUNCTION_BACKLIGHT;
 		color = <LED_COLOR_ID_WHITE>;
-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[RFC PATCH 5/5] leds: lm3692x: Offload DT node locating and parsing to core

[Matti Vaittinen]

This comment serves as an example how led controller drivers
could be simplified if led-class/led-core would handle DT node
locating and parsing. leds-lm3692x was randomly selected from
drivers using 'devm_led_classdev_register_ext' API. No further
study was done :)

This commit HAS NOT BEEN TESTED at all. Only compile tested.
This is only RFC - Eg, request for comments. If people see some
of the ideas as useful then properly tested patch should be
provided.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/leds/leds-lm3692x.c | 75 ++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 34 deletions(-)

diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
index 3d381f2f73d0..abe2b1113049 100644
--- a/drivers/leds/leds-lm3692x.c
+++ b/drivers/leds/leds-lm3692x.c
@@ -108,7 +108,7 @@
 struct lm3692x_led {
 	struct mutex lock;
 	struct i2c_client *client;
-	struct led_classdev led_dev;
+	struct led_init_data init_data;
 	struct regmap *regmap;
 	struct gpio_desc *enable_gpio;
 	struct regulator *regulator;
@@ -166,7 +166,8 @@ static int lm3692x_brightness_set(struct led_classdev *led_cdev,
 				enum led_brightness brt_val)
 {
 	struct lm3692x_led *led =
-			container_of(led_cdev, struct lm3692x_led, led_dev);
+			container_of(led_cdev->init_data, struct lm3692x_led,
+				     init_data);
 	int ret;
 	int led_brightness_lsb = (brt_val >> 5);
 
@@ -319,49 +320,56 @@ static int lm3692x_init(struct lm3692x_led *led)
 
 	return ret;
 }
-static int lm3692x_probe_dt(struct lm3692x_led *led)
+
+static int lm3692x_of_parse_cb(struct led_classdev *ld, struct fwnode_handle *fw,
+			       struct led_properties *props)
 {
-	struct fwnode_handle *child = NULL;
-	struct led_init_data init_data = {};
 	int ret;
+	struct lm3692x_led *led = container_of(ld->init_data,
+					       struct lm3692x_led, init_data);
+
+	ret = fwnode_property_read_u32(fw, "reg", &led->led_enable);
+	if (ret)
+		dev_err(&led->client->dev, "reg DT property missing\n");
+
+	return ret;
+}
+
+static const struct led_ops lm3692x_ops = {
+	.brightness_set_blocking = lm3692x_brightness_set,
+};
+
+static int lm3692x_probe_dt(struct lm3692x_led *led)
+{
+	void *ret;
 
 	led->enable_gpio = devm_gpiod_get_optional(&led->client->dev,
 						   "enable", GPIOD_OUT_LOW);
 	if (IS_ERR(led->enable_gpio)) {
-		ret = PTR_ERR(led->enable_gpio);
-		dev_err(&led->client->dev, "Failed to get enable gpio: %d\n",
-			ret);
-		return ret;
+		dev_err(&led->client->dev, "Failed to get enable gpio: %ld\n",
+			PTR_ERR(led->enable_gpio));
+		return PTR_ERR(led->enable_gpio);
 	}
 
 	led->regulator = devm_regulator_get(&led->client->dev, "vled");
 	if (IS_ERR(led->regulator))
 		led->regulator = NULL;
 
-	child = device_get_next_child_node(&led->client->dev, child);
-	if (!child) {
-		dev_err(&led->client->dev, "No LED Child node\n");
-		return -ENODEV;
-	}
-
-	fwnode_property_read_string(child, "linux,default-trigger",
-				    &led->led_dev.default_trigger);
-
-	ret = fwnode_property_read_u32(child, "reg", &led->led_enable);
-	if (ret) {
-		dev_err(&led->client->dev, "reg DT property missing\n");
-		return ret;
-	}
-
-	init_data.fwnode = child;
-	init_data.devicename = led->client->name;
-	init_data.default_label = ":";
-
-	ret = devm_led_classdev_register_ext(&led->client->dev, &led->led_dev,
-					     &init_data);
-	if (ret) {
-		dev_err(&led->client->dev, "led register err: %d\n", ret);
-		return ret;
+	/*
+	 * RFC_NOTE: Do we have fixed node name here or do we need another way to
+	 * do 'match'?
+	 */
+	led->init_data.of_match = of_match_ptr("led_node_name_here");
+	led->init_data.of_parse_cb = lm3692x_of_parse_cb;
+	led->init_data.devicename = led->client->name;
+	led->init_data.default_label = ":";
+
+	ret = devm_led_classdev_register_dt(&led->client->dev, &lm3692x_ops,
+					    &led->init_data);
+	if (IS_ERR(ret)) {
+		dev_err(&led->client->dev, "led register err: %ld\n",
+			PTR_ERR(ret));
+		return PTR_ERR(ret);
 	}
 
 	return 0;
@@ -379,7 +387,6 @@ static int lm3692x_probe(struct i2c_client *client,
 
 	mutex_init(&led->lock);
 	led->client = client;
-	led->led_dev.brightness_set_blocking = lm3692x_brightness_set;
 	led->model_id = id->driver_data;
 	i2c_set_clientdata(client, led);
 
-- 
2.21.0


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

Re: [RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Enrico Weigelt, metux IT consult]

Hi,


> The thing is that
> this approach requires the LED controller binding to dictate allowed
> LED node names - which may or may not be doable. I need your help to
> evaluate this and suggest better options :)

even though I like the idea of convention-over-code, but if that's
changing allowed LED names that would risk breaking things, eg:

a) existing DT's (in the field) become incompatible with newer
   kernel versions
b) existing userlands that rely on speicific LED names become
   incomatible with newer kernel versions.



--mtx

-- 
Dringender Hinweis: aufgrund existenzieller Bedrohung durch "Emotet"
sollten Sie *niemals* MS-Office-Dokumente via E-Mail annehmen/öffenen,
selbst wenn diese von vermeintlich vertrauenswürdigen Absendern zu
stammen scheinen. Andernfalls droht Totalschaden.
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Vaittinen, Matti]

Hello Enrico & All,

First of all - thanks for showing the interest :] I am happy knowing
someone actually was interested about the RFC :)

On Mon, 2019-11-18 at 10:21 +0100, Enrico Weigelt, metux IT consult
wrote:
> Hi,
> 
> 
> > The thing is that
> > this approach requires the LED controller binding to dictate
> > allowed
> > LED node names - which may or may not be doable. I need your help
> > to
> > evaluate this and suggest better options :)
> 
> even though I like the idea of convention-over-code, but if that's
> changing allowed LED names that would risk breaking things, eg:
> 
> a) existing DT's (in the field) become incompatible with newer
>    kernel versions

This was my main concern. This of course would not mean that we could
not take this approach for new LED controller drivers - but that would
(probably) lead to dual led registration interface - one for current
approach where LED drivers do all the dirty work themself - and parse
the DT - one for new drivers which could leave DT parsing to LED core.

> b) existing userlands that rely on speicific LED names become
>    incomatible with newer kernel versions.

I didn't even think this far, but yes, I see... LED node name might be
reflected in user-space LED name. I won't start arguing if this is sane
or not - this is what we seem to be living with today :)

Anyways, I did send out a LED core change patch which allows one to add
either the node-name, or a property-value pair in init_data. LED core
can then use either of these to do LED node lookup. If none of these is
given, then we can fall-back to existing logic. That way we won't
change existing stuff.
Here:
https://lore.kernel.org/lkml/258b5c9934e2b31a5f433a7dbb908dfe5da3d30c.1574059625.git.matti.vaittinen@fi.rohmeurope.com/T/#u


I didn't invest too much of time on this yet - but at first glimpse it
seemed that at least some of the drivers did use reg - property with
fixed value to do the matching. Those could set the property name to
'reg' and value to 'X' and leave the DT node lookup and common property
parsing to the LED core. If my patch won't get too big objection (and
if no fatal flaws are found from the idea) - then I might try and find
the time to do some follow-up patches simplifying existing LED
drivers...

> 
> 
> 
> --mtx
> 

Re: [RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Enrico Weigelt, metux IT consult]

On 18.11.19 11:38, Vaittinen, Matti wrote:

Hi,

>> a) existing DT's (in the field) become incompatible with newer
>>    kernel versions
> 
> This was my main concern. This of course would not mean that we could
> not take this approach for new LED controller drivers - but that would
> (probably) lead to dual led registration interface 

Maybe just a flag for that ? Perhaps the driver could also specify a
list of node names for the LEDs, so led-core can do the lookup for them.

>> b) existing userlands that rely on speicific LED names become
>>    incomatible with newer kernel versions.
> 
> I didn't even think this far, but yes, I see... LED node name might be
> reflected in user-space LED name. I won't start arguing if this is sane
> or not - this is what we seem to be living with today :)

Especially in embedded world, this can really make sense: applications
just use a defined LED name, no matter which board it's running on.
Convention over configuration.

Personally, I also like to use LED subsystem as frontend for things like
gpio-driven relais, etc, and assign semantically fitting names instead
of "technical" ones,

> I didn't invest too much of time on this yet - but at first glimpse it
> seemed that at least some of the drivers did use reg - property with
> fixed value to do the matching. Those could set the property name to
> 'reg' and value to 'X' and leave the DT node lookup and common property
> parsing to the LED core. If my patch won't get too big objection (and
> if no fatal flaws are found from the idea) - then I might try and find
> the time to do some follow-up patches simplifying existing LED
> drivers...

Sounds good :)


--mtx

-- 
Dringender Hinweis: aufgrund existenzieller Bedrohung durch "Emotet"
sollten Sie *niemals* MS-Office-Dokumente via E-Mail annehmen/öffenen,
selbst wenn diese von vermeintlich vertrauenswürdigen Absendern zu
stammen scheinen. Andernfalls droht Totalschaden.
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Vaittinen, Matti]

On Tue, 2019-11-19 at 20:52 +0100, Enrico Weigelt, metux IT consult
wrote:
> On 18.11.19 11:38, Vaittinen, Matti wrote:
> 
> Hi,
> 
> > > a) existing DT's (in the field) become incompatible with newer
> > >    kernel versions
> > 
> > This was my main concern. This of course would not mean that we
> > could
> > not take this approach for new LED controller drivers - but that
> > would
> > (probably) lead to dual led registration interface 
> 
> Maybe just a flag for that ? Perhaps the driver could also specify a
> list of node names for the LEDs, so led-core can do the lookup for
> them.

This is actually close to what I suggested in my other email to Jacek.

> > > b) existing userlands that rely on speicific LED names become
> > >    incomatible with newer kernel versions.
> > 
> > I didn't even think this far, but yes, I see... LED node name might
> > be
> > reflected in user-space LED name. I won't start arguing if this is
> > sane
> > or not - this is what we seem to be living with today :)
> 
> Especially in embedded world, this can really make sense:
> applications
> just use a defined LED name, no matter which board it's running on.
> Convention over configuration.

Definitely. I am all for generating the name based on LED _function_ -
no matter what the board is. I like the LED name generation base on
'function' DT property. But node names tend to be somewhat generic - or
board specific (to avoid collisions). So using node name directly is
not (as far as my understanding goes - which is limited on this topic)
optimal for guaranteeing coherent view (across the boards) for user-
space. Wow, what a nice sentence for non native English speaker like me
xD

> Personally, I also like to use LED subsystem as frontend for things
> like
> gpio-driven relais, etc, and assign semantically fitting names
> instead
> of "technical" ones,

This is outside of my experience so I just believe what you say :)

> 
> > I didn't invest too much of time on this yet - but at first glimpse
> > it
> > seemed that at least some of the drivers did use reg - property
> > with
> > fixed value to do the matching. Those could set the property name
> > to
> > 'reg' and value to 'X' and leave the DT node lookup and common
> > property
> > parsing to the LED core. If my patch won't get too big objection
> > (and
> > if no fatal flaws are found from the idea) - then I might try and
> > find
> > the time to do some follow-up patches simplifying existing LED
> > drivers...
> 
> Sounds good :)
> 
> 
> --mtx
> 

Re: [RFC PATCH 0/5] leds: Add DT node finding and parsing to core

[Pavel Machek]

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

Hi!

> Personally, I also like to use LED subsystem as frontend for things like
> gpio-driven relais, etc, and assign semantically fitting names instead
> of "technical" ones,

Don't do that. Maybe we need "named GPIOs", but lets not abuse LED
subsystem for that. (Even if I may have made that mistake before).

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 --]