On Mon, 2019-11-18 at 09:03 +0200, Matti Vaittinen wrote:
> 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.
This is actually not completely true. I originally thought of adding
led-compatible - but I changed my mind after I saw that at least some
of the existing drivers used value of "reg"-property to do the matching
in-driver. So, to make it simple for them, I did add property
name/value pair in init data for matching. This allows one to do led-
compatible, or to use existing fixed reg (or other) property value. I'd
better to change the commit message to reflect this too.
>
> 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.
And same here.
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@xxxxxxxxxxxxxxxxx>
> ---
>
> Changes from v4:
> Fixed issues reported by Dan Carpenter and kbuild-bot.
> (leftover kfree and uninitialized return value)
>
> drivers/leds/led-class.c | 88 ++++++++++++--
> drivers/leds/led-core.c | 246 +++++++++++++++++++++++++++++++----
> ----
> include/linux/leds.h | 90 ++++++++++++--
> 3 files changed, 359 insertions(+), 65 deletions(-)
>
> 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..9369f03ee540 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -365,70 +365,214 @@ 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)
> {
> - int ret;
> + void *cmp = mp->raw_val;
> + int ret = -EINVAL;
> +
> + 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))
> + 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 +580,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 +612,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,
> --
> 2.21.0
>
>
