From mboxrd@z Thu Jan 1 00:00:00 1970 From: Baolin Wang Subject: Re: [PATCH 02/24] leds: core: Add support for composing LED class device names Date: Fri, 9 Nov 2018 10:35:27 +0800 Message-ID: References: <1541542052-10081-1-git-send-email-jacek.anaszewski@gmail.com> <1541542052-10081-3-git-send-email-jacek.anaszewski@gmail.com> <3a5ca343-5c48-b257-4ba4-5a7e223efbf8@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <3a5ca343-5c48-b257-4ba4-5a7e223efbf8@gmail.com> Sender: linux-kernel-owner@vger.kernel.org To: Jacek Anaszewski Cc: Linux LED Subsystem , DTML , LKML , Pavel Machek , Rob Herring , Daniel Mack , Dan Murphy , Linus Walleij , Oleh Kravchenko , Sakari Ailus , Simon Shields , Xiaotong Lu List-Id: linux-leds@vger.kernel.org Hi Jacek, On 9 November 2018 at 04:47, Jacek Anaszewski wrote: > Hi Baolin, > > Thanks for the review. > > On 11/07/2018 08:20 AM, Baolin Wang wrote: >> Hi Jacek, >> >> On 7 November 2018 at 06:07, Jacek Anaszewski >> wrote: >>> Add public led_compose_name() API for composing LED class device >>> name basing on fwnode_handle data. The function composes device name >>> according to either a new pattern or the legacy >>> pattern. The decision on using the >>> particular pattern is made basing on whether fwnode contains new >>> "function" and "color" properties, or the legacy "label" proeprty. >>> >>> Backwards compatibility with in-driver hard-coded LED class device >>> names is assured thanks to the default_desc argument. >>> >>> In case none of the aformentioned properties was found, then, for OF >>> nodes, the node name is adopted for LED class device name. >>> >>> Signed-off-by: Jacek Anaszewski >>> Cc: Baolin Wang >>> Cc: Daniel Mack >>> Cc: Dan Murphy >>> Cc: Linus Walleij >>> Cc: Oleh Kravchenko >>> Cc: Sakari Ailus >>> Cc: Simon Shields >>> Cc: Xiaotong Lu >>> --- >>> Documentation/leds/leds-class.txt | 2 +- >>> drivers/leds/led-core.c | 71 +++++++++++++++++++++++++++++++++++++++ >>> include/linux/leds.h | 32 ++++++++++++++++++ >>> 3 files changed, 104 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/leds/leds-class.txt b/Documentation/leds/leds-class.txt >>> index 836cb16..e9009c4 100644 >>> --- a/Documentation/leds/leds-class.txt >>> +++ b/Documentation/leds/leds-class.txt >>> @@ -43,7 +43,7 @@ LED Device Naming >>> >>> Is currently of the form: >>> >>> -"devicename:colour:function" >>> +"colour:function" >>> >>> There have been calls for LED properties such as colour to be exported as >>> individual led class attributes. As a solution which doesn't incur as much >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index ede4fa0..f7371fc 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -16,6 +16,8 @@ >>> #include >>> #include >>> #include >>> +#include >>> +#include >>> #include >>> #include "leds.h" >>> >>> @@ -327,3 +329,72 @@ void led_sysfs_enable(struct led_classdev *led_cdev) >>> led_cdev->flags &= ~LED_SYSFS_DISABLE; >>> } >>> EXPORT_SYMBOL_GPL(led_sysfs_enable); >>> + >>> +static void led_parse_properties(struct fwnode_handle *fwnode, >>> + struct led_properties *props) >>> +{ >>> + int ret; >>> + >>> + if (!fwnode) >>> + return; >>> + >>> + ret = fwnode_property_read_string(fwnode, "label", &props->label); >>> + if (!ret) >>> + return; >>> + >>> + ret = fwnode_property_read_string(fwnode, "function", &props->function); >>> + if (ret) >>> + pr_info("Failed to parse function property\n"); >>> + >>> + ret = fwnode_property_read_string(fwnode, "color", &props->color); >>> + if (ret) >>> + pr_info("Failed to parse color property\n"); >> >> Now the color and function properties can be optional, so we should >> deal with different return errors. >> -EINVAL means we have not set color or function property, which means >> we should not give failure message for users (maybe "not supply color >> property\n") and return 0 as success. > > The lack of any of these properties is not critical, therefore this > function does not return the error code, but only prints the status. > Please note that I'm not using pr_error(), but pr_info(). > Related to the message text - I agree, I will change it to e.g. > "color property not found" Yes, that's reasonable. > >> For other errors, we should not ignore them, instead we should give >> failure messages and return errors. > > I've skimmed through other kernel drivers and vast majority of > them don't check exact failure code in case of non-zero return, > but assume that property is missing, and eventually report > error code. I'd do the same here. Fair enough. You can still add my tested tag. Thanks. >> Tested-by: Baolin Wang >> >>> +} >>> + >>> +int led_compose_name(struct fwnode_handle *fwnode, const char *led_hw_name, >>> + const char *default_desc, char *led_classdev_name) >>> +{ >>> + struct led_properties props = {}; >>> + >>> + if (!led_classdev_name) >>> + return -EINVAL; >>> + >>> + led_parse_properties(fwnode, &props); >>> + >>> + if (props.label) { >>> + /* >>> + * Presence of 'label' DT property implies legacy LED name, >>> + * formatted as , with possible >>> + * section omission if doesn't apply to given device. >>> + * >>> + * If no led_hw_name has been passed, then it indicates that >>> + * DT label should be used as-is for LED class device name. >>> + * Otherwise the label is prepended with led_hw_name to compose >>> + * the final LED class device name. >>> + */ >>> + if (!led_hw_name) { >>> + strncpy(led_classdev_name, props.label, >>> + LED_MAX_NAME_SIZE); >>> + } else { >>> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s", >>> + led_hw_name, props.label); >>> + } >>> + } else if (props.function || props.color) { >>> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s", >>> + props.color ?: "", props.function ?: ""); >>> + } else if (default_desc) { >>> + if (!led_hw_name) { >>> + pr_err("Legacy LED naming requires devicename segment"); >>> + return -EINVAL; >>> + } >>> + snprintf(led_classdev_name, LED_MAX_NAME_SIZE, "%s:%s", >>> + led_hw_name, default_desc); >>> + } else if (is_of_node(fwnode)) { >>> + strncpy(led_classdev_name, to_of_node(fwnode)->name, >>> + LED_MAX_NAME_SIZE); >>> + } else >>> + return -EINVAL; >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(led_compose_name); >>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>> index 646c49a..ddb4001 100644 >>> --- a/include/linux/leds.h >>> +++ b/include/linux/leds.h >>> @@ -238,6 +238,32 @@ extern void led_sysfs_disable(struct led_classdev *led_cdev); >>> extern void led_sysfs_enable(struct led_classdev *led_cdev); >>> >>> /** >>> + * led_compose_name - compose LED class device name >>> + * @child: child fwnode_handle describing a LED, >>> + * or a group of synchronized LEDs. >>> + * @led_hw_name: name of the LED controller, used when falling back to legacy >>> + * LED naming; it should be set to NULL in new LED class drivers >>> + * @default_desc: default tuple, for backwards compatibility >>> + * with in-driver hard-coded LED names used as a fallback when >>> + * "label" DT property was absent; it should be set to NULL >>> + * in new LED class drivers >>> + * @led_classdev_name: composed LED class device name >>> + * >>> + * Create LED class device name basing on the configuration provided by the >>> + * board firmware. The name can have a legacy form , >>> + * or a new form . The latter is chosen if at least one of >>> + * "color" or "function" properties is present in the fwnode, leaving the >>> + * section blank if the related property is absent. The former is applied >>> + * when legacy "label" property is present in the fwnode. In case none of the >>> + * aformentioned properties was found, then, for OF nodes, the node name >>> + * is adopted for LED class device name. >>> + * >>> + * Returns: 0 on success or negative error value on failure >>> + */ >>> +extern int led_compose_name(struct fwnode_handle *child, const char *led_hw_name, >>> + const char *default_desc, char *led_classdev_name); >>> + >>> +/** >>> * led_sysfs_is_disabled - check if LED sysfs interface is disabled >>> * @led_cdev: the LED to query >>> * >>> @@ -414,6 +440,12 @@ struct led_platform_data { >>> struct led_info *leds; >>> }; >>> >>> +struct led_properties { >>> + const char *color; >>> + const char *function; >>> + const char *label; >>> +}; >>> + >>> struct gpio_desc; >>> typedef int (*gpio_blink_set_t)(struct gpio_desc *desc, int state, >>> unsigned long *delay_on, >>> -- >>> 2.1.4 >>> >> >> >> > > -- > Best regards, > Jacek Anaszewski -- Baolin Wang Best Regards