From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform Date: Thu, 16 Jul 2015 10:27:34 +0200 Message-ID: <55A76AF6.2090805@samsung.com> References: <1435740638-16259-1-git-send-email-hegdevasant@linux.vnet.ibm.com> <1435740638-16259-4-git-send-email-hegdevasant@linux.vnet.ibm.com> <55A4CFA0.6090306@samsung.com> <55A75541.4060008@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:38982 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753932AbbGPI1i (ORCPT ); Thu, 16 Jul 2015 04:27:38 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NRK00AK4NHZXA60@mailout1.w1.samsung.com> for linux-leds@vger.kernel.org; Thu, 16 Jul 2015 09:27:35 +0100 (BST) In-reply-to: <55A75541.4060008@linux.vnet.ibm.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Vasant Hegde Cc: stewart@linux.vnet.ibm.com, arnd@arndb.de, j.anaszewski81@gmail.com, cooloney@gmail.com, rpurdie@rpsys.net, linuxppc-dev@lists.ozlabs.org, linux-leds@vger.kernel.org, khandual@linux.vnet.ibm.com Hi Vasan, On 07/16/2015 08:54 AM, Vasant Hegde wrote: > On 07/14/2015 02:30 PM, Jacek Anaszewski wrote: >> Hi Vasant, > > Jacek, > >> >> Thanks for the update. I think that we have still room >> for improvements, please look at my comments below. > > Thanks for the detailed review. You're welcome. > .../... > >>> @@ -0,0 +1,24 @@ >>> +Device Tree binding for LEDs on IBM Power Systems >>> +------------------------------------------------- >>> + >> >> Please start with: >> >> --------- >> >> Required properties: >> - compatible : Should be "ibm,opal-v3-led". >> >> Each location code of FRU/Enclosure must be expressed in the >> form of a sub-node. >> >> Required properties for the sub nodes: >> - led-types : Supported LED types (attention/identify/fault) provided >> in the form of string array. >> >> --------- >> >> or something of this flavour. The example should be at the end. >> > > Fixed. > >> >> >>> +The 'leds' node under '/ibm,opal' lists service indicators available in the >>> +system and their capabilities. >>> + >>> +leds { >>> + compatible = "ibm,opal-v3-led"; >>> + led-mode = "lightpath"; >> >> What about led-mode property? If it is generated by firmware I think, >> that this should be mentioned somehow. > > Yes.. Its generated by firmware. Added this property to documentation file. > >> >>> + >>> + U78C9.001.RST0027-P1-C1 { >>> + led-types = "identify", "fault"; >>> + }; >>> + ... >>> + ... >>> +}; >>> + >>> +Each node under 'leds' node describes location code of FRU/Enclosure. >>> + >>> +compatible : should be : "ibm,opal-v3-led". >> >> Second colon was redundant here. >> > > I have added as > - compatible : "ibm,opal-v3-led". Please retain "Should be :". > >>> + >>> +The properties under each node: >>> + >>> + led-types : Supported LED types (attention/identify/fault). >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 4191614..4f56c7a 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -505,6 +505,17 @@ config LEDS_BLINKM >>> This option enables support for the BlinkM RGB LED connected >>> through I2C. Say Y to enable support for the BlinkM LED. >>> >>> +config LEDS_POWERNV >>> + tristate "LED support for PowerNV Platform" >>> + depends on LEDS_CLASS >>> + depends on PPC_POWERNV >>> + depends on OF >>> + help >>> + This option enables support for the system LEDs present on >>> + PowerNV platforms. Say 'y' to enable this support in kernel. >>> + To compile this driver as a module, choose 'm' here: the module >>> + will be called leds-powernv. >>> + >>> config LEDS_SYSCON >>> bool "LED support for LEDs on system controllers" >>> depends on LEDS_CLASS=y >>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile >>> index bf46093..480814a 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o >>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o >>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o >>> obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o >>> +obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o >>> >>> # LED SPI Drivers >>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o >>> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c >>> new file mode 100644 >>> index 0000000..b5a307c >>> --- /dev/null >>> +++ b/drivers/leds/leds-powernv.c >>> @@ -0,0 +1,463 @@ >>> +/* >>> + * PowerNV LED Driver >>> + * >>> + * Copyright IBM Corp. 2015 >>> + * >>> + * Author: Vasant Hegde >>> + * Author: Anshuman Khandual >>> + * >>> + * This program is free software; you can redistribute it and/or >>> + * modify it under the terms of the GNU General Public License >>> + * as published by the Free Software Foundation; either version >>> + * 2 of the License, or (at your option) any later version. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> + >>> +/* >>> + * By default unload path resets all the LEDs. But on PowerNV platform >>> + * we want to retain LED state across reboot as these are controlled by >>> + * firmware. Also service processor can modify the LEDs independent of >>> + * OS. Hence avoid resetting LEDs in unload path. >>> + */ >>> +static bool led_disabled; >>> + >>> +/* Map LED type to description. */ >>> +struct led_type_map { >>> + const int type; >>> + const char *desc; >>> +}; >>> +static const struct led_type_map led_type_map[] = { >>> + {OPAL_SLOT_LED_TYPE_ID, POWERNV_LED_TYPE_IDENTIFY}, >>> + {OPAL_SLOT_LED_TYPE_FAULT, POWERNV_LED_TYPE_FAULT}, >>> + {OPAL_SLOT_LED_TYPE_ATTN, POWERNV_LED_TYPE_ATTENTION}, >>> + {-1, NULL}, >>> +}; >>> + >>> +/* >>> + * LED set routines have been implemented as work queue tasks scheduled >>> + * on the global work queue. Individual task calls OPAL interface to set >>> + * the LED state which might sleep for some time. >>> + */ >>> +struct powernv_led_data { >>> + struct led_classdev cdev; >>> + enum led_brightness value; /* Brightness value */ >>> + struct mutex lock; >>> + struct work_struct work_led; /* LED update workqueue */ >>> +}; >>> + >>> +struct powernv_leds_priv { >>> + int num_leds; >>> + struct powernv_led_data powernv_leds[]; >>> +}; >>> + >>> + >>> +static inline int sizeof_powernv_leds_priv(int num_leds) >>> +{ >>> + return sizeof(struct powernv_leds_priv) + >>> + (sizeof(struct powernv_led_data) * num_leds); >>> +} >>> + >>> +/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */ >>> +static int powernv_get_led_type(struct led_classdev *led_cdev) >>> +{ >>> + char *desc; >>> + int i; >>> + >>> + desc = strstr(led_cdev->name, ":"); >>> + if (!desc) >>> + return -1; >>> + desc++; >>> + if (!desc) >>> + return -1; >>> + >>> + for (i = 0; i < ARRAY_SIZE(led_type_map); i++) >>> + if (!strcmp(led_type_map[i].desc, desc)) >>> + return led_type_map[i].type; >>> + >>> + return -1; >>> +} >>> + >>> +/* This function gets LED location code for given LED classdev */ >>> +static char *powernv_get_location_code(struct led_classdev *led_cdev) >>> +{ >>> + char *loc_code; >>> + char *colon; >>> + >>> + /* Location code of the LED */ >>> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name); >>> + if (!loc_code) { >>> + dev_err(led_cdev->dev, >>> + "%s: Memory allocation failed\n", __func__); >>> + return NULL; >>> + } >>> + >>> + colon = strstr(loc_code, ":"); >>> + if (!colon) { >>> + kfree(loc_code); >>> + return NULL; >>> + } >>> + >>> + *colon = '\0'; >>> + return loc_code; >>> +} >>> + >>> +/* >>> + * This commits the state change of the requested LED through an OPAL call. >>> + * This function is called from work queue task context when ever it gets >>> + * scheduled. This function can sleep at opal_async_wait_response call. >>> + */ >>> +static void powernv_led_set(struct led_classdev *led_cdev, >>> + enum led_brightness value) >>> +{ >>> + char *loc_code; >>> + int rc, token, led_type; >>> + u64 led_mask, led_value = 0; >>> + __be64 max_led_type; >>> + struct opal_msg msg; >>> + >>> + led_type = powernv_get_led_type(led_cdev); >>> + if (led_type == -1) >>> + return; >> >> Please parse the led type once upon initialization and add related >> property to the struct powernv_led_data that will hold the value. > > I thought we can get location code and type using class dev name itself. Hence I > didn't add these two properties to structure.. This way you are doing extra work for parsing the name each time the brightness is set. > Do you want me to add them to structure itself? Yes, please add them. >> >>> + loc_code = powernv_get_location_code(led_cdev); >>> + if (!loc_code) >>> + return; >> >> The same situation as in case of led type. >> >>> + /* Prepare for the OPAL call */ >>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX); >> >> This value could be also calculated only once. > > Yeah. May be I can move this to powernv_leds_priv structure. > >> >>> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type; >>> + if (value) >>> + led_value = led_mask; >>> + >>> + /* OPAL async call */ >>> + token = opal_async_get_token_interruptible(); >>> + if (token < 0) { >>> + if (token != -ERESTARTSYS) >>> + dev_err(led_cdev->dev, >>> + "%s: Couldn't get OPAL async token\n", >>> + __func__); >>> + goto out_loc; >>> + } >>> + >>> + rc = opal_leds_set_ind(token, loc_code, >>> + led_mask, led_value, &max_led_type); >>> + if (rc != OPAL_ASYNC_COMPLETION) { >>> + dev_err(led_cdev->dev, >>> + "%s: OPAL set LED call failed for %s [rc=%d]\n", >>> + __func__, loc_code, rc); >>> + goto out_token; >>> + } >>> + >>> + rc = opal_async_wait_response(token, &msg); >>> + if (rc) { >>> + dev_err(led_cdev->dev, >>> + "%s: Failed to wait for the async response [rc=%d]\n", >>> + __func__, rc); >>> + goto out_token; >>> + } >>> + >>> + rc = be64_to_cpu(msg.params[1]); >>> + if (rc != OPAL_SUCCESS) >>> + dev_err(led_cdev->dev, >>> + "%s : OAPL async call returned failed [rc=%d]\n", >>> + __func__, rc); >>> + >>> +out_token: >>> + opal_async_release_token(token); >>> + >>> +out_loc: >>> + kfree(loc_code); >>> +} >>> + >>> +/* >>> + * This function fetches the LED state for a given LED type for >>> + * mentioned LED classdev structure. >>> + */ >>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev) >>> +{ >>> + char *loc_code; >>> + int rc, led_type; >>> + __be64 led_mask, led_value, max_led_type; >>> + >>> + led_type = powernv_get_led_type(led_cdev); >>> + if (led_type == -1) >>> + return LED_OFF; >>> + >>> + loc_code = powernv_get_location_code(led_cdev); >>> + if (!loc_code) >>> + return LED_OFF; >>> + >>> + /* Fetch all LED status */ >>> + led_mask = cpu_to_be64(0); >>> + led_value = cpu_to_be64(0); >>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX); >>> + >>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type); >>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) { >>> + dev_err(led_cdev->dev, >>> + "%s: OPAL get led call failed [rc=%d]\n", >>> + __func__, rc); >>> + goto led_fail; >>> + } >>> + >>> + led_mask = be64_to_cpu(led_mask); >>> + led_value = be64_to_cpu(led_value); >> >> be64_to_cpu result should be assigned to the variable of u64/s64 type. > > PowerNV platform is capable of running both big/little endian mode.. But > presently our firmware is big endian. These variable contains big endian values. > Hence I have created as __be64 .. (This is the convention we follow in other > places as well). It is correct that the argument is of __be64 type, but be64_to_cpu returns u64 type, whereas you assign it to __be64. >> >>> + /* LED status available */ >>> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) { >>> + dev_err(led_cdev->dev, >>> + "%s: LED status not available for %s\n", >>> + __func__, led_cdev->name); >>> + goto led_fail; >>> + } >>> + >>> + /* LED status value */ >>> + if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) { >>> + kfree(loc_code); >>> + return LED_FULL; >>> + } >>> + >>> +led_fail: >>> + kfree(loc_code); >>> + return LED_OFF; >>> +} >>> + >>> +/* Execute LED set task for given led classdev */ >>> +static void powernv_deferred_led_set(struct work_struct *work) >>> +{ >>> + struct powernv_led_data *powernv_led = >>> + container_of(work, struct powernv_led_data, work_led); >>> + >>> + mutex_lock(&powernv_led->lock); >>> + powernv_led_set(&powernv_led->cdev, powernv_led->value); >>> + mutex_unlock(&powernv_led->lock); >>> +} >>> + >>> +/* >>> + * LED classdev 'brightness_get' function. This schedules work >>> + * to update LED state. >>> + */ >>> +static void powernv_brightness_set(struct led_classdev *led_cdev, >>> + enum led_brightness value) >>> +{ >>> + struct powernv_led_data *powernv_led = >>> + container_of(led_cdev, struct powernv_led_data, cdev); >>> + >>> + /* Do not modify LED in unload path */ >>> + if (led_disabled) >>> + return; >>> + >>> + /* Prepare the request */ >>> + powernv_led->value = value; >>> + >>> + /* Schedule the new task */ >>> + schedule_work(&powernv_led->work_led); >>> +} >>> + >>> +/* LED classdev 'brightness_get' function */ >>> +static enum led_brightness >>> +powernv_brightness_get(struct led_classdev *led_cdev) >>> +{ >>> + return powernv_led_get(led_cdev); >>> +} >>> + >>> + >>> +/* >>> + * This function registers classdev structure for any given type of LED on >>> + * a given child LED device node. >>> + */ >>> +static int powernv_led_create(struct device *dev, >>> + struct powernv_led_data *powernv_led, >>> + const char *led_name, const char *led_type_desc) >>> +{ >>> + int rc; >>> + >>> + /* Create the name for classdev */ >>> + powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s", >>> + led_name, led_type_desc); >> >> Please use devm_kasprintf. > > Done. > >> >>> + if (!powernv_led->cdev.name) { >>> + dev_err(dev, >>> + "%s: Memory allocation failed for classdev name\n", >>> + __func__); >>> + return -ENOMEM; >>> + } >>> + >>> + /* Make sure LED type is supported */ >>> + if (powernv_get_led_type(&powernv_led->cdev) == -1) { >>> + kfree(powernv_led->cdev.name); >>> + return -EINVAL; >>> + } >>> + >>> + powernv_led->cdev.brightness_set = powernv_brightness_set; >>> + powernv_led->cdev.brightness_get = powernv_brightness_get; >>> + powernv_led->cdev.brightness = LED_OFF; >>> + powernv_led->cdev.max_brightness = LED_FULL; >>> + >>> + mutex_init(&powernv_led->lock); >>> + INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set); >>> + >>> + /* Register the classdev */ >>> + rc = led_classdev_register(dev, &powernv_led->cdev); >> >> devm_led_classdev_register is also available. > > Looks like most of the existing drivers are using led_classdev_register function.. > Which one is preferred here? It is quite new API, but it is now preferable. >> >>> + if (rc) { >>> + dev_err(dev, "%s: Classdev registration failed for %s\n", >>> + __func__, powernv_led->cdev.name); >>> + kfree(powernv_led->cdev.name); >>> + } >>> + >>> + return rc; >>> +} >>> + >>> +/* Unregister classdev structure for any given LED */ >>> +static void powernv_led_delete(struct powernv_led_data *powernv_led) >>> +{ >>> + led_classdev_unregister(&powernv_led->cdev); >>> +} >> >> This function is redundant. > > Like powernv_led_create, I just added this function ... hoping this will improve > code readability. > Will remove this function. >> >>> +/* Go through LED device tree node and register LED classdev structure */ >>> +static int powernv_led_classdev(struct platform_device *pdev, >>> + struct device_node *led_node, >>> + struct powernv_leds_priv *priv, int num_leds) >>> +{ >>> + const char *cur = NULL; >>> + int i, rc = -1; >>> + struct property *p; >>> + struct device_node *np; >>> + struct powernv_led_data *powernv_led; >>> + struct device *dev = &pdev->dev; >>> + >>> + for_each_child_of_node(led_node, np) { >>> + p = of_find_property(np, "led-types", NULL); >>> + if (!p) >>> + continue; >>> + >>> + while ((cur = of_prop_next_string(p, cur)) != NULL) { >>> + powernv_led = &priv->powernv_leds[priv->num_leds++]; >>> + if (priv->num_leds > num_leds) { >>> + rc = -ENOMEM; >>> + goto classdev_fail; >>> + } >>> + rc = powernv_led_create(dev, >>> + powernv_led, np->name, cur); >>> + if (rc) >>> + goto classdev_fail; >>> + } /* while end */ >>> + } >>> + >>> + platform_set_drvdata(pdev, priv); >>> + return rc; >>> + >>> +classdev_fail: >>> + for (i = priv->num_leds - 2; i > 0; i--) >> >> Why do you leave element with id == 0 unreleased? > > It was my mistake. Fixed. > >> >>> + powernv_led_delete(&priv->powernv_leds[i]); >>> + >>> + return rc; >>> +} >>> + >>> +/* >>> + * We want to populate LED device for each LED type. Hence we >>> + * have to calculate count explicitly. >>> + */ >>> +static int powernv_leds_count(struct device_node *led_node) >>> +{ >>> + const char *cur = NULL; >>> + int num_leds = 0; >>> + struct property *p; >>> + struct device_node *np; >>> + >>> + for_each_child_of_node(led_node, np) { >>> + p = of_find_property(np, "led-types", NULL); >>> + if (!p) >>> + continue; >>> + >>> + while ((cur = of_prop_next_string(p, cur)) != NULL) >>> + num_leds++; >>> + } >>> + >>> + return num_leds; >>> +} >> >> Does it mean that if the node exists but does't have led-types >> property we are not going to register it? > > Yes.. No point in registering location code ..which doesn't have led-types > property. > > >> I assume that this is >> firmware which generates the nodes, otherwise it would make >> no sense to have the node, am I right? > > That correct. Our firmware generates this property. Thanks for the clarification. >> >>> +/* Platform driver probe */ >>> +static int powernv_led_probe(struct platform_device *pdev) >>> +{ >>> + int num_leds; >>> + struct device_node *led_node; >>> + struct powernv_leds_priv *priv; >>> + >>> + led_node = of_find_node_by_path("/ibm,opal/leds"); >>> + if (!led_node) { >>> + dev_err(&pdev->dev, >>> + "%s: LED parent device node not found\n", __func__); >>> + return -EINVAL; >>> + } >>> + >>> + num_leds = powernv_leds_count(led_node); >>> + if (num_leds <= 0) { >>> + dev_err(&pdev->dev, >>> + "%s: No location code found under LED node\n", >>> + __func__); >>> + return -EINVAL; >>> + } >>> + >>> + priv = devm_kzalloc(&pdev->dev, >>> + sizeof_powernv_leds_priv(num_leds), >>> + GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + return powernv_led_classdev(pdev, led_node, priv, num_leds); >>> +} >>> + >>> +/* Platform driver remove */ >>> +static int powernv_led_remove(struct platform_device *pdev) >>> +{ >>> + int i; >>> + struct powernv_led_data *powernv_led; >>> + struct powernv_leds_priv *priv; >>> + >>> + /* Disable LED operation */ >>> + led_disabled = true; >>> + >>> + priv = platform_get_drvdata(pdev); >>> + >>> + for (i = 0; i < priv->num_leds; i++) { >>> + powernv_led = &priv->powernv_leds[i]; >>> + powernv_led_delete(powernv_led); >>> + flush_work(&powernv_led->work_led); >>> + } >> >> You are missing mutex_destroy here. > > Oh yeah.. I missed it. Fixed. > > -Vasant > > -- Best Regards, Jacek Anaszewski