| Message ID | 258b5c9934e2b31a5f433a7dbb908dfe5da3d30c.1574059625.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Support ROHM BD71828 PMIC | expand |
Hi Matti, Thank you for the patch. If your driver does not depend on it then please send is separately. Besides, we would require to convert many of current LED drivers to verify how the proposed parsing mechanism will work with them. I've been testing my LED name composition series using QEMU and stubbing things in drivers where necessary and I propose to use the same approach in this case. Best regards, Jacek Anaszewski On 11/18/19 8:03 AM, 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. > > 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> > --- > > 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(-) >
Hello Jacek, On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: > Hi Matti, > > Thank you for the patch. If your driver does not depend > on it then please send is separately. The BD71828 depends on device-tree node look-up. It does not utilize the common property parsing. I could of course do the child dt-node walking in BD71828 driver - but it kind of breaks my motivation to do the LED core improvement if I anyways need to do the parsing in BD71828 driver ;) > Besides, we would require > to convert many of current LED drivers to verify how the > proposed parsing mechanism will work with them. I see the risk you are pointing out. And I actually think we could default to old mechanism if of_match or match_property is not given (for now). I could then see the existing drivers who use init_data - and ensure those are initializing the new match_property and of_match in init_data with 0. That would be quite trivial task. That would allow us to convert and test existing drivers one-by-one while allowing new drivers to offload the LED node look-up and common property parsing to LED core. No risk, but less drivers to convert in the future - and simpler drivers to maintain when all of them do not need to duplicate node look-up or basic property parsing ;) To make this more concrete: We can only do the new DT node look-up if either if (init_data->match_property.name && init_data->match_property.size) or if (init_data->of_match) That would keep the node-lookup same for all existing drivers. Eg, led_find_fwnode could for now just do: struct fwnode_handle *led_find_fwnode(struct device *parent, struct led_init_data *init_data) { /* * This should never be called W/O init data. */ if (!init_data) return NULL; /* * For old drivers which do not populate new match information * just directly use the given init_data->fwnode no matter if * it is NULL or not. - as old functionality did. */ if ( (!init_data->match_property.name || !init_data->match_property.size) && !init_data->of_match) return init_data->fwnode; /* match information was given - do node look-up */ ... } Furthermore, the common property parsing could also be (for now) done only if match data is given: /* * For now only allow core to parse DT properties if * parsing is explicitly requested by driver or if core has * found new match data from init_data and then set the flag */ if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) led_add_props(led_cdev, &props); or just simply: if ((init_data->match_property.name && init_data->match_property.size) || init_data->of_match) led_add_props(led_cdev, &props); (but this won't allow driver to ask for common parsing even if it was verified for this drv to work - hence I like the flag better) And if you don't feel confident I can even drop the "common property parsing" from the series and leave only the "node look-up if match-data was given" to it. Anyways, I would like to introduce this support while I am working with the BD71828 driver which really has the LEDs - but I can modify the patch series so that it only impacts to drivers which implement the new match data in init_data and leave old drivers to be converted one-by- one when they can be tested. > I've been testing > my LED name composition series using QEMU and stubbing things in > drivers where necessary and I propose to use the same approach > in this case. I don't plan to do any mass-conversion as it is somewhat risky. I can do conversion to some of the drivers (simple ones which I can understand without too much of pain) - and ask if anyone having access to actual HW with LEDs could be kind enough to test the patch for the device. Tested drivers can then be taken in-tree as examples. And who knows, maybe there is some developers looking for a hobby project with access to LED controller to help with the rest ;) I don't have the ambition to change all of the LED drivers but I think I can give my 10 cents by contributing the mechanism and doing few examples :) Anyways, please let me know if you think you could accept patch which won't change existing driver functionality - but allows new drivers to not duplicate the code. Else I'll just duplicate the lookup code in one more driver and hope I don't have another PMIC with LED controller on my table too soon... (I am having "some" pressure to do few other tasks I recently got. So I am afraid I won't have too much time to invest on LEDs this year :( Thus setting up the qemu and starting with stubbing is really not an option for me at this phase). Br, Matti Vaittinen > > Best regards, > Jacek Anaszewski > > On 11/18/19 8:03 AM, 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. > > > > 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> > > --- > > > > 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(-) > >
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@fi.rohmeurope.com> > --- > > 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 > >
Hi Matti, On 11/19/19 8:21 AM, Vaittinen, Matti wrote: > Hello Jacek, > > On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: >> Hi Matti, >> >> Thank you for the patch. If your driver does not depend >> on it then please send is separately. > > The BD71828 depends on device-tree node look-up. It does not utilize > the common property parsing. I could of course do the child dt-node > walking in BD71828 driver - but it kind of breaks my motivation to do > the LED core improvement if I anyways need to do the parsing in BD71828 > driver ;) If you do not plan on spending too much time on contributing this set then I propose adhering to the currently used parsing schema :-) And you have to know that from this development cycle I handed over LED tree maintenance to Pavel Machek, so you will require to have his acceptance in the first place. >> Besides, we would require >> to convert many of current LED drivers to verify how the >> proposed parsing mechanism will work with them. > > I see the risk you are pointing out. And I actually think we could > default to old mechanism if of_match or match_property is not given > (for now). I could then see the existing drivers who use init_data - > and ensure those are initializing the new match_property and of_match > in init_data with 0. That would be quite trivial task. > > That would allow us to convert and test existing drivers one-by-one > while allowing new drivers to offload the LED node look-up and common > property parsing to LED core. No risk, but less drivers to convert in > the future - and simpler drivers to maintain when all of them do not > need to duplicate node look-up or basic property parsing ;) I personally would prefer to do the massive driver update to using the new mechanism. I know that this is time consuming but we are not in a hurry. > To make this more concrete: > > We can only do the new DT node look-up if either > if (init_data->match_property.name && init_data->match_property.size) > or > if (init_data->of_match) > That would keep the node-lookup same for all existing drivers. > > Eg, > led_find_fwnode could for now just do: > > struct fwnode_handle *led_find_fwnode(struct device *parent, > struct led_init_data *init_data) > { > /* > * This should never be called W/O init data. > */ > if (!init_data) > return NULL; > > /* > * For old drivers which do not populate new match information > * just directly use the given init_data->fwnode no matter if > * it is NULL or not. - as old functionality did. > */ > if ( (!init_data->match_property.name || > !init_data->match_property.size) && !init_data->of_match) > return init_data->fwnode; > > /* match information was given - do node look-up */ > ... > } > > Furthermore, the common property parsing could also be (for now) done > only if match data is given: > > /* > * For now only allow core to parse DT properties if > * parsing is explicitly requested by driver or if core has > * found new match data from init_data and then set the flag > */ > if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) > led_add_props(led_cdev, &props); > > or just simply: > if ((init_data->match_property.name && > init_data->match_property.size) || init_data->of_match) > led_add_props(led_cdev, &props); > > (but this won't allow driver to ask for common parsing even if it was > verified for this drv to work - hence I like the flag better) > > And if you don't feel confident I can even drop the "common property > parsing" from the series and leave only the "node look-up if match-data > was given" to it. > > Anyways, I would like to introduce this support while I am working with > the BD71828 driver which really has the LEDs - but I can modify the > patch series so that it only impacts to drivers which implement the new > match data in init_data and leave old drivers to be converted one-by- > one when they can be tested. > >> I've been testing >> my LED name composition series using QEMU and stubbing things in >> drivers where necessary and I propose to use the same approach >> in this case. > > I don't plan to do any mass-conversion as it is somewhat risky. I can You do not need hardware to test DT parsing as I mentioned before, so I don't see too much risk involved. > do conversion to some of the drivers (simple ones which I can > understand without too much of pain) - and ask if anyone having access > to actual HW with LEDs could be kind enough to test the patch for the > device. Tested drivers can then be taken in-tree as examples. And who > knows, maybe there is some developers looking for a hobby project with > access to LED controller to help with the rest ;) I don't have the > ambition to change all of the LED drivers but I think I can give my 10 > cents by contributing the mechanism and doing few examples :) If you want to introduce good, robust mechanism, then it should be tested against widest possible spectrum of use cases. > Anyways, please let me know if you think you could accept patch which > won't change existing driver functionality - but allows new drivers to > not duplicate the code. Else I'll just duplicate the lookup code in one > more driver and hope I don't have another PMIC with LED controller on > my table too soon... > > (I am having "some" pressure to do few other tasks I recently got. So I > am afraid I won't have too much time to invest on LEDs this year :( > Thus setting up the qemu and starting with stubbing is really not an > option for me at this phase). As mentioned before - I no longer apply patches so you will need to consult Pavel, but I bet he will have similar opinion.
On Tue, 2019-11-19 at 20:30 +0100, Jacek Anaszewski wrote: > Hi Matti, > > On 11/19/19 8:21 AM, Vaittinen, Matti wrote: > > Hello Jacek, > > > > On Mon, 2019-11-18 at 22:55 +0100, Jacek Anaszewski wrote: > > > Hi Matti, > > > > > > Thank you for the patch. If your driver does not depend > > > on it then please send is separately. > > > > The BD71828 depends on device-tree node look-up. It does not > > utilize > > the common property parsing. I could of course do the child dt-node > > walking in BD71828 driver - but it kind of breaks my motivation to > > do > > the LED core improvement if I anyways need to do the parsing in > > BD71828 > > driver ;) > > If you do not plan on spending too much time on contributing this > set then I propose adhering to the currently used parsing schema :-) I have no objections on doing few iterations of the patches. And I tend to take care of problems my changes cause. So I am prepared to spend the required time fixing the node look-up and common property parsing for drivers I do break. What I am not prepared is to change and test all of the existing drivers - so it's better to not promise such :) > And you have to know that from this development cycle I handed > over LED tree maintenance to Pavel Machek, so you will require > to have his acceptance in the first place. Well, then I for sure wait for Pavel's take on this. In general I have had some positive feedback about doing the DT node look-up and common property parsing in a centralized manner in LED core. So maybe also Pavel sees the value of adding this now for new drivers - instead of adding one more driver with copy-paste node look-up code. I want to thank you for all the comments though, it's nice that you have been active on this topic! > > > Besides, we would require > > > to convert many of current LED drivers to verify how the > > > proposed parsing mechanism will work with them. > > > > I see the risk you are pointing out. And I actually think we could > > default to old mechanism if of_match or match_property is not given > > (for now). I could then see the existing drivers who use init_data > > - > > and ensure those are initializing the new match_property and > > of_match > > in init_data with 0. That would be quite trivial task. > > > > That would allow us to convert and test existing drivers one-by-one > > while allowing new drivers to offload the LED node look-up and > > common > > property parsing to LED core. No risk, but less drivers to convert > > in > > the future - and simpler drivers to maintain when all of them do > > not > > need to duplicate node look-up or basic property parsing ;) > > I personally would prefer to do the massive driver update to using > the new mechanism. I know that this is time consuming but we are not > in a hurry. I understand the preference of massive update - but I also know that if we wait for someone to do a massive update and neglect improvements done in small steps, then there is a risk that there won't be any updates at all... > > To make this more concrete: > > > > We can only do the new DT node look-up if either > > if (init_data->match_property.name && init_data- > > >match_property.size) > > or > > if (init_data->of_match) > > That would keep the node-lookup same for all existing drivers. > > > > Eg, > > led_find_fwnode could for now just do: > > > > struct fwnode_handle *led_find_fwnode(struct device *parent, > > struct led_init_data *init_data) > > { > > /* > > * This should never be called W/O init data. > > */ > > if (!init_data) > > return NULL; > > > > /* > > * For old drivers which do not populate new match information > > * just directly use the given init_data->fwnode no matter if > > * it is NULL or not. - as old functionality did. > > */ > > if ( (!init_data->match_property.name || > > !init_data->match_property.size) && !init_data->of_match) > > return init_data->fwnode; > > > > /* match information was given - do node look-up */ > > ... > > } > > > > Furthermore, the common property parsing could also be (for now) > > done > > only if match data is given: > > > > /* > > * For now only allow core to parse DT properties if > > * parsing is explicitly requested by driver or if core has > > * found new match data from init_data and then set the flag > > */ > > if (INVENT_A_COOL_NEW_FLAG_NAME_HERE) > > led_add_props(led_cdev, &props); > > > > or just simply: > > if ((init_data->match_property.name && > > init_data->match_property.size) || init_data->of_match) > > led_add_props(led_cdev, &props); > > > > (but this won't allow driver to ask for common parsing even if it > > was > > verified for this drv to work - hence I like the flag better) > > > > And if you don't feel confident I can even drop the "common > > property > > parsing" from the series and leave only the "node look-up if match- > > data > > was given" to it. > > > > Anyways, I would like to introduce this support while I am working > > with > > the BD71828 driver which really has the LEDs - but I can modify the > > patch series so that it only impacts to drivers which implement the > > new > > match data in init_data and leave old drivers to be converted one- > > by- > > one when they can be tested. > > > > > I've been testing > > > my LED name composition series using QEMU and stubbing things in > > > drivers where necessary and I propose to use the same approach > > > in this case. > > > > I don't plan to do any mass-conversion as it is somewhat risky. I > > can > > You do not need hardware to test DT parsing as I mentioned before, > so I don't see too much risk involved. Yes - if you have the time to test all the drivers at once - and assuming you don't do some silly mistake there. Final verification should always be done in HW. But as I said, I want to ensure all the drivers I convert to new mechanism will work (the best I can) - and as I can't test all the drivers I won't do mass-conversion. I have offered a initial solution in the patch (and suggested reduced version to mitigate the risk of breaking anything in this email) - and I see this beneficial and as a good starting point enabling the rest of the improvements. But as you said, we need also Pavel's take on this. > > do conversion to some of the drivers (simple ones which I can > > understand without too much of pain) - and ask if anyone having > > access > > to actual HW with LEDs could be kind enough to test the patch for > > the > > device. Tested drivers can then be taken in-tree as examples. And > > who > > knows, maybe there is some developers looking for a hobby project > > with > > access to LED controller to help with the rest ;) I don't have the > > ambition to change all of the LED drivers but I think I can give my > > 10 > > cents by contributing the mechanism and doing few examples :) > > If you want to introduce good, robust mechanism, then it should be > tested against widest possible spectrum of use cases. Yes. OTOH, if the mechanism is sub-optimal, then the beauty of open source is that it can be improved. Preparing in advance for something that never happens is also a waste. But I guess we don't need to discuss this philosphy here :) > > Anyways, please let me know if you think you could accept patch > > which > > won't change existing driver functionality - but allows new drivers > > to > > not duplicate the code. Else I'll just duplicate the lookup code in > > one > > more driver and hope I don't have another PMIC with LED controller > > on > > my table too soon... > > > > (I am having "some" pressure to do few other tasks I recently got. > > So I > > am afraid I won't have too much time to invest on LEDs this year :( > > Thus setting up the qemu and starting with stubbing is really not > > an > > option for me at this phase). > > As mentioned before - I no longer apply patches so you will need to > consult Pavel, but I bet he will have similar opinion. Who knows, maybe he can see this differently :) Thanks anyways! Br, Matti Vaittinen
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,
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> --- 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(-)