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.
.../...
>> @@ -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".
>> +
>> +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 <hegdevasant@xxxxxxxxxxxxxxxxxx>
>> + * Author: Anshuman Khandual <khandual@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * 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 <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +
>> +#include <asm/opal.h>
>> +
>> +/*
>> + * 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..
Do you want me to add them to structure itself?
>
>> + 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).
>
>> + /* 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?
>
>> + 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.
>
>> +/* 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
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
