[v4,15/16] leds: Add common LED binding parsing support to LED class/core
diff mbox series

Message ID 773ed63e512f9086483089d67c492d092444bc8a.1573928775.git.matti.vaittinen@fi.rohmeurope.com
State New
Headers show
Series
  • Support ROHM BD71828 PMIC
Related show

Commit Message

Vaittinen, Matti Nov. 16, 2019, 7:02 p.m. UTC
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 - or linux,led-compatible property) and handling of
few common LED properties.

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

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

Regarding the node look-up: If no init_data is given, then no
node-lookup is done and cdev name is used as such.

If init_data is goven but no starting point for node lookup - then
(parent) device's own DT node is used. If no led-compatible is given,
then of_match is searched for. If neither led-compatible not of_match
is given then device's own node or passed starting point are used as
such.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/leds/led-class.c |  88 ++++++++++++--
 drivers/leds/led-core.c  | 246 +++++++++++++++++++++++++++++++--------
 include/linux/leds.h     |  90 ++++++++++++--
 3 files changed, 360 insertions(+), 64 deletions(-)

Comments

Dan Carpenter Nov. 18, 2019, 5:59 a.m. UTC | #1
Hi Matti,

[auto build test WARNING on 31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c]

url:    https://github.com/0day-ci/linux/commits/Matti-Vaittinen/Support-ROHM-BD71828-PMIC/20191117-030515
base:    31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/leds/led-core.c:400 fw_is_match() error: uninitialized symbol 'ret'.
drivers/leds/led-core.c:465 led_find_fwnode() error: double free of 'val'

# https://github.com/0day-ci/linux/commit/7b8033cfca34525c9e45fe2e74783fef74f4a49c
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 7b8033cfca34525c9e45fe2e74783fef74f4a49c
vim +/ret +400 drivers/leds/led-core.c

7b8033cfca3452 Matti Vaittinen  2019-11-16  368  static int fw_is_match(struct fwnode_handle *fw,
7b8033cfca3452 Matti Vaittinen  2019-11-16  369  		       struct led_fw_match_property *mp, void *val)
bb4e9af0348dfe Jacek Anaszewski 2019-06-09  370  {
7b8033cfca3452 Matti Vaittinen  2019-11-16  371  	void *cmp = mp->raw_val;
bb4e9af0348dfe Jacek Anaszewski 2019-06-09  372  	int ret;
bb4e9af0348dfe Jacek Anaszewski 2019-06-09  373  
7b8033cfca3452 Matti Vaittinen  2019-11-16  374  	if (mp->raw_val) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  375  		ret = fwnode_property_read_u8_array(fw, mp->name, val,
7b8033cfca3452 Matti Vaittinen  2019-11-16  376  						    mp->size);
7b8033cfca3452 Matti Vaittinen  2019-11-16  377  	} else if (mp->intval) {
                                                                   ^^^^^^^^^^
Smatch is complaining about if this is false then ret isn't set.

7b8033cfca3452 Matti Vaittinen  2019-11-16  378  		cmp = mp->intval;
7b8033cfca3452 Matti Vaittinen  2019-11-16  379  		switch (mp->size) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  380  		case 1:
7b8033cfca3452 Matti Vaittinen  2019-11-16  381  			ret = fwnode_property_read_u8_array(fw, mp->name, val,
7b8033cfca3452 Matti Vaittinen  2019-11-16  382  						    mp->size);
7b8033cfca3452 Matti Vaittinen  2019-11-16  383  			break;
7b8033cfca3452 Matti Vaittinen  2019-11-16  384  		case 2:
7b8033cfca3452 Matti Vaittinen  2019-11-16  385  			ret = fwnode_property_read_u16_array(fw, mp->name, val,
7b8033cfca3452 Matti Vaittinen  2019-11-16  386  						    mp->size);
7b8033cfca3452 Matti Vaittinen  2019-11-16  387  			break;
7b8033cfca3452 Matti Vaittinen  2019-11-16  388  		case 4:
7b8033cfca3452 Matti Vaittinen  2019-11-16  389  			ret = fwnode_property_read_u32_array(fw, mp->name, val,
7b8033cfca3452 Matti Vaittinen  2019-11-16  390  						    mp->size);
7b8033cfca3452 Matti Vaittinen  2019-11-16  391  			break;
7b8033cfca3452 Matti Vaittinen  2019-11-16  392  		case 8:
7b8033cfca3452 Matti Vaittinen  2019-11-16  393  			ret = fwnode_property_read_u64_array(fw, mp->name, val,
7b8033cfca3452 Matti Vaittinen  2019-11-16  394  						    mp->size);
7b8033cfca3452 Matti Vaittinen  2019-11-16  395  			break;
7b8033cfca3452 Matti Vaittinen  2019-11-16  396  		default:
7b8033cfca3452 Matti Vaittinen  2019-11-16  397  			return -EINVAL;
7b8033cfca3452 Matti Vaittinen  2019-11-16  398  		}
7b8033cfca3452 Matti Vaittinen  2019-11-16  399  	}
7b8033cfca3452 Matti Vaittinen  2019-11-16 @400  	if (!ret && cmp) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  401  		if (!memcmp(val, cmp, mp->size)) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  402  			kfree(val);
                                                                        ^^^^^^^^^^
This kfree leads to a double free below.  Freeing it here is a
layering violation, so delete this one and keep the kfree in the caller.

7b8033cfca3452 Matti Vaittinen  2019-11-16  403  			return 1;
7b8033cfca3452 Matti Vaittinen  2019-11-16  404  		}
7b8033cfca3452 Matti Vaittinen  2019-11-16  405  	}
7b8033cfca3452 Matti Vaittinen  2019-11-16  406  	return 0;
7b8033cfca3452 Matti Vaittinen  2019-11-16  407  }
7b8033cfca3452 Matti Vaittinen  2019-11-16  408  /**
7b8033cfca3452 Matti Vaittinen  2019-11-16  409   * led_find_fwnode - find fwnode for led
7b8033cfca3452 Matti Vaittinen  2019-11-16  410   * @parent	LED controller device
7b8033cfca3452 Matti Vaittinen  2019-11-16  411   * @init_data	led init data with match information
7b8033cfca3452 Matti Vaittinen  2019-11-16  412   *
7b8033cfca3452 Matti Vaittinen  2019-11-16  413   * Scans the firmware nodes and returns node matching the given init_data.
7b8033cfca3452 Matti Vaittinen  2019-11-16  414   * NOTE: Function increases refcount for found node. Caller must decrease
7b8033cfca3452 Matti Vaittinen  2019-11-16  415   * refcount using fwnode_handle_put when finished with node.
7b8033cfca3452 Matti Vaittinen  2019-11-16  416   */
7b8033cfca3452 Matti Vaittinen  2019-11-16  417  struct fwnode_handle *led_find_fwnode(struct device *parent,
7b8033cfca3452 Matti Vaittinen  2019-11-16  418  				      struct led_init_data *init_data)
7b8033cfca3452 Matti Vaittinen  2019-11-16  419  {
7b8033cfca3452 Matti Vaittinen  2019-11-16  420  	struct fwnode_handle *fw;
7b8033cfca3452 Matti Vaittinen  2019-11-16  421  
7b8033cfca3452 Matti Vaittinen  2019-11-16  422  	/*
7b8033cfca3452 Matti Vaittinen  2019-11-16  423  	 * This should never be called W/O init data. We could always return
7b8033cfca3452 Matti Vaittinen  2019-11-16  424  	 * dev_fwnode() - but then we should pump-up the refcount
7b8033cfca3452 Matti Vaittinen  2019-11-16  425  	 */
7b8033cfca3452 Matti Vaittinen  2019-11-16  426  	if (!init_data)
7b8033cfca3452 Matti Vaittinen  2019-11-16  427  		return NULL;
7b8033cfca3452 Matti Vaittinen  2019-11-16  428  
7b8033cfca3452 Matti Vaittinen  2019-11-16  429  	if (!init_data->fwnode)
7b8033cfca3452 Matti Vaittinen  2019-11-16  430  		fw = dev_fwnode(parent);
7b8033cfca3452 Matti Vaittinen  2019-11-16  431  	else
7b8033cfca3452 Matti Vaittinen  2019-11-16  432  		fw = init_data->fwnode;
7b8033cfca3452 Matti Vaittinen  2019-11-16  433  
7b8033cfca3452 Matti Vaittinen  2019-11-16  434  	if (!fw)
7b8033cfca3452 Matti Vaittinen  2019-11-16  435  		return NULL;
7b8033cfca3452 Matti Vaittinen  2019-11-16  436  
7b8033cfca3452 Matti Vaittinen  2019-11-16  437  	/*
7b8033cfca3452 Matti Vaittinen  2019-11-16  438  	 * Simple things are pretty. I think simplest is to use DT node-name
7b8033cfca3452 Matti Vaittinen  2019-11-16  439  	 * for matching the node with LED - same way regulators use the node
7b8033cfca3452 Matti Vaittinen  2019-11-16  440  	 * name to match with desc.
7b8033cfca3452 Matti Vaittinen  2019-11-16  441  	 *
7b8033cfca3452 Matti Vaittinen  2019-11-16  442  	 * This may not work with existing LED DT entries if the node name has
7b8033cfca3452 Matti Vaittinen  2019-11-16  443  	 * been freely selectible. In order to this to work the binding doc
7b8033cfca3452 Matti Vaittinen  2019-11-16  444  	 * for LED driver should define usable node names.
7b8033cfca3452 Matti Vaittinen  2019-11-16  445  	 *
7b8033cfca3452 Matti Vaittinen  2019-11-16  446  	 * If this is not working we can define specific match property which
7b8033cfca3452 Matti Vaittinen  2019-11-16  447  	 * value we scan and use for matching for LEDs connected to the
7b8033cfca3452 Matti Vaittinen  2019-11-16  448  	 * controller.
7b8033cfca3452 Matti Vaittinen  2019-11-16  449  	 */
7b8033cfca3452 Matti Vaittinen  2019-11-16  450  	if (init_data->match_property.name && init_data->match_property.size) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  451  		u8 *val;
7b8033cfca3452 Matti Vaittinen  2019-11-16  452  		int ret;
7b8033cfca3452 Matti Vaittinen  2019-11-16  453  		struct fwnode_handle *child;
7b8033cfca3452 Matti Vaittinen  2019-11-16  454  		struct led_fw_match_property *mp;
7b8033cfca3452 Matti Vaittinen  2019-11-16  455  
7b8033cfca3452 Matti Vaittinen  2019-11-16  456  		mp = &init_data->match_property;
7b8033cfca3452 Matti Vaittinen  2019-11-16  457  
7b8033cfca3452 Matti Vaittinen  2019-11-16  458  		val = kzalloc(mp->size, GFP_KERNEL);
7b8033cfca3452 Matti Vaittinen  2019-11-16  459  		if (!val)
7b8033cfca3452 Matti Vaittinen  2019-11-16  460  			return ERR_PTR(-ENOMEM);
7b8033cfca3452 Matti Vaittinen  2019-11-16  461  
7b8033cfca3452 Matti Vaittinen  2019-11-16  462  		fwnode_for_each_child_node(fw, child) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  463  			ret = fw_is_match(child, mp, val);
                                                                                                     ^^^

7b8033cfca3452 Matti Vaittinen  2019-11-16  464  			if (ret > 0) {
7b8033cfca3452 Matti Vaittinen  2019-11-16 @465  				kfree(val);
                                                                                      ^^^

Oops.

7b8033cfca3452 Matti Vaittinen  2019-11-16  466  				return child;
7b8033cfca3452 Matti Vaittinen  2019-11-16  467  			}
7b8033cfca3452 Matti Vaittinen  2019-11-16  468  			if (ret < 0) {
7b8033cfca3452 Matti Vaittinen  2019-11-16  469  				dev_err(parent,
7b8033cfca3452 Matti Vaittinen  2019-11-16  470  					"invalid fw match. Use raw_val?\n");
7b8033cfca3452 Matti Vaittinen  2019-11-16  471  				fwnode_handle_put(child);
7b8033cfca3452 Matti Vaittinen  2019-11-16  472  				break;
7b8033cfca3452 Matti Vaittinen  2019-11-16  473  			}
7b8033cfca3452 Matti Vaittinen  2019-11-16  474  		}
7b8033cfca3452 Matti Vaittinen  2019-11-16  475  		kfree(val);
7b8033cfca3452 Matti Vaittinen  2019-11-16  476  	}
7b8033cfca3452 Matti Vaittinen  2019-11-16  477  	if (init_data->of_match)
7b8033cfca3452 Matti Vaittinen  2019-11-16  478  		fw = fwnode_get_named_child_node(fw, init_data->of_match);
7b8033cfca3452 Matti Vaittinen  2019-11-16  479  	else
7b8033cfca3452 Matti Vaittinen  2019-11-16  480  		fw = fwnode_handle_get(fw);
7b8033cfca3452 Matti Vaittinen  2019-11-16  481  
7b8033cfca3452 Matti Vaittinen  2019-11-16  482  	return fw;
7b8033cfca3452 Matti Vaittinen  2019-11-16  483  }

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
Vaittinen, Matti Nov. 18, 2019, 6:34 a.m. UTC | #2
Hello All,

On Mon, 2019-11-18 at 08:59 +0300, Dan Carpenter wrote:
> Hi Matti,
> 
> [auto build test WARNING on 31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c]
> 
> url:    
> https://github.com/0day-ci/linux/commits/Matti-Vaittinen/Support-ROHM-BD71828-PMIC/20191117-030515
> base:    31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> drivers/leds/led-core.c:400 fw_is_match() error: uninitialized symbol
> 'ret'.
> drivers/leds/led-core.c:465 led_find_fwnode() error: double free of
> 'val'

Ouch. The free is clearly a leftover from original code where the
"is_match" was not it's own function. Sorry folks. I'll fix both issues
and send new version.

By the way, I'd appreciate if someone else would also check the
reference-counting for fwnodes - is pumping up the refcount in
led_find_fwnode Ok? My idea is that when-ever the led_find_fwnode
returns an fwnode, the LED class needs to decrease refcount when LED is
unregistered. No matter if fwnode was from device, given by LED driver
or found by the LED class.

Br,
	Matti Vaittinen

Patch
diff mbox series

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 647b1263c579..98173b64a597 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -235,6 +235,29 @@  static int led_classdev_next_name(const char *init_name, char *name,
 	return i;
 }
 
+static void led_add_props(struct led_classdev *ld, struct led_properties *props)
+{
+	if (props->default_trigger)
+		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
+	 */
+	if (props->default_state) {
+		ld->brightness = LED_OFF;
+		if (!strcmp(props->default_state, "on"))
+			ld->brightness = LED_FULL;
+	/*
+	 * We probably should not call the brightness_get prior calling
+	 * the of_parse_cb if one is provided.
+	 * Add a flag to advertice that state should be queried and kept as-is.
+	 */
+		else if (!strcmp(props->default_state, "keep"))
+			props->brightness_keep = true;
+	}
+}
+
 /**
  * led_classdev_register_ext - register a new object of led_classdev class
  *			       with init data.
@@ -251,22 +274,58 @@  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 = {0};
+	struct fwnode_handle *fw;
+
+	/*
+	 * We don't try getting the name based on DT node if init-data is not
+	 * given. We could see if we find LED properties from the device's node
+	 * but that might change LED names for old users of
+	 * led_classdev_register who have been providing the LED name in
+	 * cdev->name. So we seek fwnode for names only if init_data is given
+	 */
 	if (init_data) {
+		led_cdev->init_data = 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);
+
+		fw = led_find_fwnode(parent, init_data);
+		if (!fw) {
+			dev_err(parent, "No fwnode found\n");
+			return -ENOENT;
+		}
+		/*
+		 * We did increase refcount for fwnode. Let's set a flag so we
+		 * can decrease it during deregistration
+		 */
+		led_cdev->fwnode_owned = true;
+
+		ret = led_parse_fwnode_props(parent, fw, &props);
+		if (ret)
+			goto err_out;
+
+		if (init_data->of_parse_cb)
+			ret = init_data->of_parse_cb(led_cdev, fw, &props);
 		if (ret < 0)
-			return ret;
+			goto err_out;
+
+		led_add_props(led_cdev, &props);
+
+		ret = led_compose_name(parent, init_data, &props,
+				       composed_name);
+		if (ret < 0)
+			goto err_out;
 	} else {
 		proposed_name = led_cdev->name;
+		led_cdev->fwnode_owned = false;
+		fw = NULL;
 	}
 
 	ret = led_classdev_next_name(proposed_name, final_name, sizeof(final_name));
 	if (ret < 0)
-		return ret;
+		goto err_out;
 
 	mutex_init(&led_cdev->led_access);
 	mutex_lock(&led_cdev->led_access);
@@ -274,22 +333,28 @@  int led_classdev_register_ext(struct device *parent,
 				led_cdev, led_cdev->groups, "%s", final_name);
 	if (IS_ERR(led_cdev->dev)) {
 		mutex_unlock(&led_cdev->led_access);
-		return PTR_ERR(led_cdev->dev);
+		ret = PTR_ERR(led_cdev->dev);
+		goto err_out;
 	}
-	if (init_data && init_data->fwnode)
-		led_cdev->dev->fwnode = init_data->fwnode;
+	if (fw)
+		led_cdev->dev->fwnode = fw;
 
 	if (ret)
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
 				led_cdev->name, dev_name(led_cdev->dev));
 
+	if (props.brightness_keep)
+		if (led_cdev->brightness_get)
+			led_cdev->brightness =
+				 led_cdev->brightness_get(led_cdev);
+
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
 		ret = led_add_brightness_hw_changed(led_cdev);
 		if (ret) {
 			device_unregister(led_cdev->dev);
 			led_cdev->dev = NULL;
 			mutex_unlock(&led_cdev->led_access);
-			return ret;
+			goto err_out;
 		}
 	}
 
@@ -322,6 +387,10 @@  int led_classdev_register_ext(struct device *parent,
 			led_cdev->name);
 
 	return 0;
+err_out:
+	if (led_cdev->fwnode_owned)
+		fwnode_handle_put(fw);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(led_classdev_register_ext);
 
@@ -355,6 +424,9 @@  void led_classdev_unregister(struct led_classdev *led_cdev)
 	if (led_cdev->flags & LED_BRIGHT_HW_CHANGED)
 		led_remove_brightness_hw_changed(led_cdev);
 
+	if (led_cdev->fwnode_owned)
+		fwnode_handle_put(led_cdev->dev->fwnode);
+
 	device_unregister(led_cdev->dev);
 
 	down_write(&leds_list_lock);
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718dbe0f8..0cd105a5f200 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -365,70 +365,216 @@  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)
+static int fw_is_match(struct fwnode_handle *fw,
+		       struct led_fw_match_property *mp, void *val)
 {
+	void *cmp = mp->raw_val;
 	int ret;
 
+	if (mp->raw_val) {
+		ret = fwnode_property_read_u8_array(fw, mp->name, val,
+						    mp->size);
+	} else if (mp->intval) {
+		cmp = mp->intval;
+		switch (mp->size) {
+		case 1:
+			ret = fwnode_property_read_u8_array(fw, mp->name, val,
+						    mp->size);
+			break;
+		case 2:
+			ret = fwnode_property_read_u16_array(fw, mp->name, val,
+						    mp->size);
+			break;
+		case 4:
+			ret = fwnode_property_read_u32_array(fw, mp->name, val,
+						    mp->size);
+			break;
+		case 8:
+			ret = fwnode_property_read_u64_array(fw, mp->name, val,
+						    mp->size);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+	if (!ret && cmp) {
+		if (!memcmp(val, cmp, mp->size)) {
+			kfree(val);
+			return 1;
+		}
+	}
+	return 0;
+}
+/**
+ * led_find_fwnode - find fwnode for led
+ * @parent	LED controller device
+ * @init_data	led init data with match information
+ *
+ * Scans the firmware nodes and returns node matching the given init_data.
+ * NOTE: Function increases refcount for found node. Caller must decrease
+ * refcount using fwnode_handle_put when finished with node.
+ */
+struct fwnode_handle *led_find_fwnode(struct device *parent,
+				      struct led_init_data *init_data)
+{
+	struct fwnode_handle *fw;
+
+	/*
+	 * This should never be called W/O init data. We could always return
+	 * dev_fwnode() - but then we should pump-up the refcount
+	 */
+	if (!init_data)
+		return NULL;
+
+	if (!init_data->fwnode)
+		fw = dev_fwnode(parent);
+	else
+		fw = init_data->fwnode;
+
+	if (!fw)
+		return NULL;
+
+	/*
+	 * 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.
+	 *
+	 * If this is not working we can define specific match property which
+	 * value we scan and use for matching for LEDs connected to the
+	 * controller.
+	 */
+	if (init_data->match_property.name && init_data->match_property.size) {
+		u8 *val;
+		int ret;
+		struct fwnode_handle *child;
+		struct led_fw_match_property *mp;
+
+		mp = &init_data->match_property;
+
+		val = kzalloc(mp->size, GFP_KERNEL);
+		if (!val)
+			return ERR_PTR(-ENOMEM);
+
+		fwnode_for_each_child_node(fw, child) {
+			ret = fw_is_match(child, mp, val);
+			if (ret > 0) {
+				kfree(val);
+				return child;
+			}
+			if (ret < 0) {
+				dev_err(parent,
+					"invalid fw match. Use raw_val?\n");
+				fwnode_handle_put(child);
+				break;
+			}
+		}
+		kfree(val);
+	}
+	if (init_data->of_match)
+		fw = fwnode_get_named_child_node(fw, init_data->of_match);
+	else
+		fw = fwnode_handle_get(fw);
+
+	return fw;
+}
+EXPORT_SYMBOL(led_find_fwnode);
+
+int led_parse_fwnode_props(struct device *dev, struct fwnode_handle *fwnode,
+			   struct led_properties *props)
+{
+	int ret = 0;
+
 	if (!fwnode)
-		return;
+		return 0;
 
 	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 +582,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",
@@ -468,11 +614,19 @@  int led_compose_name(struct device *dev, struct led_init_data *init_data,
 		}
 		snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s",
 			 devicename, init_data->default_label);
-	} else if (is_of_node(fwnode)) {
-		strscpy(led_classdev_name, to_of_node(fwnode)->name,
-			LED_MAX_NAME_SIZE);
-	} else
-		return -EINVAL;
+	} else {
+		struct fwnode_handle *fwnode = led_find_fwnode(dev, init_data);
+		int ret = -EINVAL;
+
+		if (is_of_node(fwnode)) {
+			ret = 0;
+			strscpy(led_classdev_name, to_of_node(fwnode)->name,
+				LED_MAX_NAME_SIZE);
+		}
+		fwnode_handle_put(fwnode);
+
+		return ret;
+	}
 
 	return 0;
 }
diff --git a/include/linux/leds.h b/include/linux/leds.h
index efb309dba914..aea7d6bc7294 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,8 +33,47 @@  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;
+	bool		brightness_keep;
+};
+
+struct led_fw_match_property {
+	const char *name;	/* Name of the property to match */
+	void *raw_val;		/* Raw property value as present in fwnode */
+	void *intval;		/* Property value if 8,16,32 or 64bit integer */
+	size_t size;		/* Size of value in bytes */
+};
+
 struct led_init_data {
-	/* device fwnode handle */
+	/*
+	 * If DT binding dictates the node name the driver can fill of_match
+	 * corresponding to node name describing this LED. If fwnode is given
+	 * the match is searched from it's child nodes. If not, the match is
+	 * searched from device's own child nodes.
+	 */
+	const char *of_match;
+	/*
+	 * If fwnode contains property with known value the driver can specify
+	 * correct propertty-value pair here to do the matching. This has higher
+	 * priority than of_match. If fwnode is given the match is searched
+	 * from it's child nodes. If not match is searched from device's
+	 * own child nodes.
+	 */
+	struct led_fw_match_property match_property;
+	/*
+	 * device fwnode handle. If of_match or led_compatible are not given
+	 * this is used for LED as given. If of_match or led_compatible are
+	 * given then this is used as a parent node whose child nodes are
+	 * scanned for given match.
+	 */
 	struct fwnode_handle *fwnode;
 	/*
 	 * default <color:function> tuple, for backward compatibility
@@ -53,9 +94,17 @@  struct led_init_data {
 	 * set it to true
 	 */
 	bool devname_mandatory;
+	/*
+	 * 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
+	 */
+	int (*of_parse_cb)(struct led_classdev *ld, struct fwnode_handle *fw,
+			    struct led_properties *props);
 };
 
 struct led_classdev {
+	struct led_init_data	*init_data;
 	const char		*name;
 	enum led_brightness	 brightness;
 	enum led_brightness	 max_brightness;
@@ -148,6 +197,7 @@  struct led_classdev {
 
 	/* Ensures consistent access to the LED Flash Class device */
 	struct mutex		led_access;
+	bool			fwnode_owned;
 };
 
 /**
@@ -302,6 +352,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 +362,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 +376,33 @@  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 init data
+ * @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.
+ * NOTE: Function increases refcount for found node. Caller must decrease
+ * refcount using fwnode_handle_put when finished with node.
+ *
+ * Returns: node handle or NULL if matching fw node was not found
+ */
+struct fwnode_handle *led_find_fwnode(struct device *parent,
+				      struct led_init_data *init_data);
+
+/**
+ * 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 +569,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,