* [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-03-20 11:04 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-03-20 11:04 UTC (permalink / raw)
To: linuxppc-dev, linux-leds; +Cc: stewart, cooloney, rpurdie, khandual
From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
This patch implements LED driver for PowerNV platform using the existing
generic LED class framework. It registers classdev structures for all
individual LEDs detected on the system through LED specific device tree
nodes. Device tree nodes specify what all kind of LEDs present on the
same location code. It registers LED classdev structure for each of them.
The platform level implementation of LED get and set state has been
achieved through OPAL calls. These calls are made available for the
driver by exporting from architecture specific codes.
As per the LED class framework, the 'brightness_set' function should not
sleep. Hence these functions have been implemented through global work
queue tasks which might sleep on OPAL async call completion.
All the system LEDs can be found in the same regular path /sys/class/leds/.
There are two different kind of LEDs present for the same location code,
one being the identify indicator and other one being the fault indicator.
We don't use LED colors. Hence our LEDs have names in this format.
<location_code>:<ATTENTION|IDENTIFY|FAULT>
Any positive brightness value would turn on the LED and a zero value
would turn off the LED. The driver will return LED_FULL (255) for any
turned on LED and LED_OFF for any turned off LED.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
---
Changes in v2:
- Added System Attention indicator support
- Moved common code to powernv_led_set_queue()
-Vasant
arch/powerpc/platforms/powernv/opal.c | 12 +
drivers/leds/Kconfig | 9
drivers/leds/Makefile | 1
drivers/leds/leds-powernv.c | 620 +++++++++++++++++++++++++++++++++
4 files changed, 641 insertions(+), 1 deletion(-)
create mode 100644 drivers/leds/leds-powernv.c
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 142a08a..fbfd9c1 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
static int __init opal_init(void)
{
- struct device_node *np, *consoles;
+ struct device_node *np, *consoles, *led;
int rc;
opal_node = of_find_node_by_path("/ibm,opal");
@@ -772,6 +772,13 @@ static int __init opal_init(void)
/* Create i2c platform devices */
opal_i2c_create_devs();
+ /* Create led platform devices */
+ led = of_find_node_by_path("/ibm,opal/led");
+ if (led) {
+ of_platform_device_create(led, "opal_led", NULL);
+ of_node_put(led);
+ }
+
/* Find all OPAL interrupts and request them */
opal_irq_init(opal_node);
@@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
EXPORT_SYMBOL_GPL(opal_tpo_read);
EXPORT_SYMBOL_GPL(opal_tpo_write);
EXPORT_SYMBOL_GPL(opal_i2c_request);
+/* Export these symbols for PowerNV LED class driver */
+EXPORT_SYMBOL_GPL(opal_leds_get_ind);
+EXPORT_SYMBOL_GPL(opal_leds_set_ind);
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 25b320d..a93223c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -508,6 +508,15 @@ 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
+ help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ Say 'm' enable this support as module.
+
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 cbba921..604ffc9 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,620 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
+ * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
+ *
+ * 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.
+ */
+
+#define PREFIX "POWERNV_LED"
+#define pr_fmt(fmt) PREFIX ": " fmt
+
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+
+#include <asm/opal.h>
+
+#define LED_STR_ATTENTION ":ATTENTION"
+#define LED_STR_IDENT ":IDENTIFY"
+#define LED_STR_FAULT ":FAULT"
+
+/*
+ * LED operation state
+ *
+ * led_classdev_unregister resets the brightness values. However
+ * we want to retain the LED state across boot. Hence disable
+ * LED operation before calling led_classdev_unregister.
+ */
+static bool led_disabled = false;
+
+/*
+ * LED type map
+ *
+ * Converts LED event type into it's description.
+ */
+static const char *led_type_map[OPAL_SLOT_LED_TYPE_MAX] = {
+ "Attention", "Identify", "Fault"
+};
+
+/*
+ * 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. A lot of work queue
+ * tasks attempting to set the LED states can prevent each other from
+ * completing their task. So there is a need to restrict the total number
+ * of work queue tasks at any point in time related to LED.
+ */
+#define MAX_LED_REQ_COUNT 100
+static int led_req_count;
+
+/*
+ * LED classdev
+ *
+ * Each LED classdev structure registered on the platform will be put into
+ * a list which will be eventually used for de-registration purpose.
+ */
+struct powernv_led {
+ struct led_classdev cdev;
+ struct list_head link;
+};
+static LIST_HEAD(powernv_led_list);
+static DEFINE_SPINLOCK(powernv_led_spinlock);
+
+/*
+ * LED set state command
+ *
+ * Each set LED state request will be saved and put into a list to be
+ * processed later by a work queue task.
+ */
+struct powernv_led_cmd {
+ struct led_classdev *led_cdev; /* Points to classdev structure */
+ enum led_brightness value; /* Brightness value */
+ u64 led_type; /* Identify or Fault */
+ struct list_head link;
+};
+static LIST_HEAD(powernv_led_cmd_list);
+static DEFINE_SPINLOCK(powernv_led_cmd_spinlock);
+
+/*
+ * LED set state task
+ *
+ * Each set LED state request added into the request list for post
+ * processing will be followed by one of this structure. The work
+ * struct here will be scheduled on the global work queue to process
+ * one of the requests from the request list. Though the order of
+ * execution is not guaraneteed.
+ */
+struct powernv_led_work {
+ struct work_struct work; /* Individual task */
+ bool deletion; /* Need deletion */
+ struct list_head link;
+};
+static LIST_HEAD(powernv_led_work_list);
+static DEFINE_SPINLOCK(powernv_led_work_spinlock);
+
+/*
+ * powernv_led_compact_work_list
+ *
+ * This function goes through the entire list of scheduled powernv_led_work
+ * nodes and removes the nodes which have already processed one set LED
+ * state request from request list and has been marked for deletion. This is
+ * essential for cleaning the list before adding new elements into it. This
+ * also tracks the total number of pending tasks. Once it reaches the
+ * threshold the function will throttle till all the scheduled tasks completes
+ * execution during which the user space thread will block and will be
+ * prevented from queuing up more LED state change requests.
+ */
+static void powernv_led_compact_work_list(void)
+{
+ struct powernv_led_work *pwork, *pwork_ne;
+ unsigned long flags;
+
+ spin_lock_irqsave(&powernv_led_work_spinlock, flags);
+ led_req_count = 0;
+ list_for_each_entry_safe(pwork, pwork_ne,
+ &powernv_led_work_list, link) {
+ if (pwork->deletion) {
+ list_del(&pwork->link);
+ kfree(pwork);
+ continue;
+ }
+ led_req_count++;
+ }
+ spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
+
+ /* Throttle if the threshold reached */
+ if (led_req_count == MAX_LED_REQ_COUNT) {
+ list_for_each_entry(pwork, &powernv_led_work_list, link)
+ flush_work(&pwork->work);
+ }
+}
+
+/*
+ * powernv_led_set
+ *
+ * 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, u64 led_type)
+{
+ char *loc_code;
+ int rc, token;
+ u64 led_mask, max_led_type, led_value = 0;
+ struct opal_msg msg;
+
+ /* Location code of the LED */
+ loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
+ if (!loc_code) {
+ pr_err(PREFIX "Memory allocation failed at %s\n", __func__);
+ return;
+ }
+
+ switch (led_type) {
+ case OPAL_SLOT_LED_TYPE_ATTN:
+ loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
+ break;
+ case OPAL_SLOT_LED_TYPE_ID:
+ loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
+ break;
+ case OPAL_SLOT_LED_TYPE_FAULT:
+ loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
+ break;
+ default: /* Unknown LED type */
+ goto out_loc;
+ }
+
+ /* Prepare for the OPAL call */
+ max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+ led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
+ if (value)
+ led_value = OPAL_SLOT_LED_STATE_ON << led_type;
+
+ /* OPAL async call */
+ token = opal_async_get_token_interruptible();
+ if (token < 0) {
+ if (token != -ERESTARTSYS)
+ pr_err("%s: Couldn't get the token, returning\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) {
+ pr_err("OPAL call opal_leds_set_ind failed for %s with %d\n",
+ loc_code, rc);
+ goto out_token;
+ }
+
+ rc = opal_async_wait_response(token, &msg);
+ if (rc) {
+ pr_err("%s: Failed to wait for the async response, %d\n",
+ __func__, rc);
+ goto out_token;
+ }
+
+ rc = be64_to_cpu(msg.params[1]);
+ if (rc != OPAL_SUCCESS)
+ pr_err("Async call returned with failed status: %d\n", rc);
+
+out_token:
+ opal_async_release_token(token);
+
+out_loc:
+ kfree(loc_code);
+}
+
+/*
+ * powernv_led_get
+ *
+ * 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,
+ u64 led_type)
+{
+ char *loc_code;
+ int rc;
+ u64 led_mask, led_value, max_led_type;
+
+ /* LED location code */
+ loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
+ if (!loc_code) {
+ pr_err("Memory allocation failed at: %s\n", __func__);
+ return -ENOMEM;
+ }
+
+ switch (led_type) {
+ case OPAL_SLOT_LED_TYPE_ATTN:
+ loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
+ break;
+ case OPAL_SLOT_LED_TYPE_ID:
+ loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
+ break;
+ case OPAL_SLOT_LED_TYPE_FAULT:
+ loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
+ break;
+ default: /* Unsupported LED type */
+ goto led_fail;
+ }
+
+ /* 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) {
+ pr_err("OPAL call opal_leds_get_ind failed with %d\n", rc);
+ goto led_fail;
+ }
+
+ led_mask = be64_to_cpu(led_mask);
+ led_value = be64_to_cpu(led_value);
+
+ /* LED status available */
+ if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
+ pr_err("%s LED status not available for %s\n",
+ led_type_map[led_type], loc_code);
+ 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;
+}
+
+/*
+ * powernv_led_work_func
+ *
+ * This the function which will be executed by any LED work task on the
+ * global work queue. This function de-queues one of the request node
+ * from the request list, processes it and then deletes the request node.
+ * This also accesses it's own work list node and sets the deletion flag
+ * in there making itself a candidate for removal the next time the
+ * compact function gets called.
+ */
+static void powernv_led_work_func(struct work_struct *work)
+{
+ struct powernv_led_work *pwork;
+ struct powernv_led_cmd *req;
+ unsigned long flags;
+
+ /* De-queue one request, process it and then delete */
+ spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
+ if (list_empty(&powernv_led_cmd_list)) {
+ pr_err("Request list empty, but work queue task queued\n");
+ spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
+ return;
+ }
+
+ req = list_first_entry(&powernv_led_cmd_list,
+ struct powernv_led_cmd, link);
+ list_del(&req->link);
+ spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
+
+ powernv_led_set(req->led_cdev, req->value, req->led_type);
+ kfree(req);
+
+ /* Mark the work queue task for deletion */
+ spin_lock_irqsave(&powernv_led_work_spinlock, flags);
+ pwork = container_of(work, struct powernv_led_work, work);
+ pwork->deletion = true;
+ spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
+}
+
+/*
+ * powernv_led_set_queue
+ *
+ * LED classdev 'brightness_set' function for LEDs
+ */
+static void powernv_led_set_queue(struct led_classdev *led_cdev,
+ enum led_brightness value, u64 led_type)
+{
+ struct powernv_led_work *pwork;
+ struct powernv_led_cmd *attn;
+ unsigned long flags;
+
+ if (led_disabled)
+ return;
+
+ /* Allocate the request */
+ attn = kzalloc(sizeof(struct powernv_led_cmd), GFP_KERNEL);
+ if (!attn) {
+ pr_err("Memory allocation failed at: %s\n", __func__);
+ return;
+ }
+
+ /* Allocate the work */
+ pwork = kzalloc(sizeof(struct powernv_led_work), GFP_KERNEL);
+ if (!pwork) {
+ pr_err("Memory allocation failed at: %s\n", __func__);
+ kfree(attn);
+ return;
+ }
+
+ /* Prepare the request */
+ attn->led_cdev = led_cdev;
+ attn->value = value;
+ attn->led_type = led_type;
+
+ /* Queue the request */
+ spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
+ list_add_tail(&attn->link, &powernv_led_cmd_list);
+ spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
+
+ /* Compact the work list */
+ powernv_led_compact_work_list();
+
+ /* Add the new task into the work list */
+ spin_lock_irqsave(&powernv_led_work_spinlock, flags);
+ list_add_tail(&pwork->link, &powernv_led_work_list);
+ spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
+
+ /* Schedule the new task */
+ INIT_WORK(&pwork->work, powernv_led_work_func);
+ schedule_work(&pwork->work);
+}
+/*
+ * powernv_led_set_attn
+ *
+ * LED classdev 'brightness_set' function for attention LED.
+ */
+static void powernv_led_set_attn(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ return powernv_led_set_queue(led_cdev, value,
+ OPAL_SLOT_LED_TYPE_ATTN);
+}
+
+/*
+ * powernv_led_get_attn
+ *
+ * LED classdev 'brightness_get' function for attention LED.
+ */
+static enum led_brightness powernv_led_get_attn(struct led_classdev *led_cdev)
+{
+ return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ATTN);
+}
+
+/*
+ * powernv_led_set_ident
+ *
+ * LED classdev 'brightness_set' function for identify LED types.
+ */
+static void powernv_led_set_ident(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ return powernv_led_set_queue(led_cdev, value, OPAL_SLOT_LED_TYPE_ID);
+}
+
+/*
+ * powernv_led_get_ident
+ *
+ * LED classdev 'brightness_get' function for identify LED types.
+ */
+static enum led_brightness powernv_led_get_ident(struct led_classdev *led_cdev)
+{
+ return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ID);
+}
+
+/*
+ * powernv_led_set_fault
+ *
+ * LED classdev 'brightness_set' function for fault LED types.
+ */
+static void powernv_led_set_fault(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ return powernv_led_set_queue(led_cdev, value,
+ OPAL_SLOT_LED_TYPE_FAULT);
+}
+
+/*
+ * powernv_led_get_fault
+ *
+ * LED classdev 'brightness_get' function for fault LED types.
+ */
+static enum led_brightness powernv_led_get_fault(struct led_classdev *led_cdev)
+{
+ return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_FAULT);
+}
+
+/*
+ * has_led_type
+ *
+ * This function verifies whether the child LED device node supports certain
+ * type of LED or not. This will be used to register LEDclassdev structures
+ * for that particual type of LED for a given device tree node.
+ */
+static bool has_led_type(struct device_node *cled, const char *led_type)
+{
+ bool result = false;
+
+ if (of_property_match_string(cled, "led-types", led_type) >= 0)
+ result = true;
+
+ return result;
+}
+
+/*
+ * power_led_classdev
+ *
+ * This function registers classdev structure for any given type of LED on
+ * a given child LED device node.
+ */
+static int power_led_classdev(struct platform_device *pdev,
+ struct device_node *cled, u64 led_type)
+{
+ int rc;
+ unsigned long flags;
+ struct powernv_led *cpled;
+
+ cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
+ if (!cpled) {
+ pr_err("Memory allocation failed at %s\n", __func__);
+ return -ENOMEM;
+ }
+
+ /* Create the name for classdev */
+ switch (led_type) {
+ case OPAL_SLOT_LED_TYPE_ATTN:
+ cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
+ cled->name, LED_STR_ATTENTION);
+ cpled->cdev.brightness_set = powernv_led_set_attn;
+ cpled->cdev.brightness_get = powernv_led_get_attn;
+ break;
+ case OPAL_SLOT_LED_TYPE_ID:
+ cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
+ cled->name, LED_STR_IDENT);
+ cpled->cdev.brightness_set = powernv_led_set_ident;
+ cpled->cdev.brightness_get = powernv_led_get_ident;
+ break;
+ case OPAL_SLOT_LED_TYPE_FAULT:
+ cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
+ cled->name, LED_STR_FAULT);
+ cpled->cdev.brightness_set = powernv_led_set_fault;
+ cpled->cdev.brightness_get = powernv_led_get_fault;
+ break;
+ default: /* Unsupported LED type */
+ return -EINVAL;
+ }
+
+ if (!cpled->cdev.name) {
+ pr_err("Memory allocation failed for classdev name\n");
+ return -ENOMEM;
+ }
+
+ cpled->cdev.brightness = LED_OFF;
+ cpled->cdev.max_brightness = LED_FULL;
+ cpled->cdev.flags = LED_CORE_SUSPENDRESUME;
+
+ /* Register the classdev */
+ rc = led_classdev_register(&pdev->dev, &cpled->cdev);
+ if (rc) {
+ pr_err("Classdev registration failed for %s\n",
+ cpled->cdev.name);
+ } else {
+ spin_lock_irqsave(&powernv_led_spinlock, flags);
+ list_add_tail(&cpled->link, &powernv_led_list);
+ spin_unlock_irqrestore(&powernv_led_spinlock, flags);
+ pr_debug("Classdev registration successful for %s\n",
+ cpled->cdev.name);
+ }
+ return rc;
+}
+
+/* Platform driver probe */
+static int powernv_led_probe(struct platform_device *pdev)
+{
+ struct device_node *led, *cled;
+ int rc = 0;
+
+ led = of_find_node_by_path("/ibm,opal/led");
+ if (!led) {
+ pr_err("LED parent device node not found\n");
+ return -EINVAL;
+ }
+
+ for_each_child_of_node(led, cled) {
+ if (has_led_type(cled, LED_TYPE_ATTENTION))
+ rc = power_led_classdev(pdev, cled,
+ OPAL_SLOT_LED_TYPE_ATTN);
+
+ if (has_led_type(cled, LED_TYPE_IDENTIFY))
+ rc = power_led_classdev(pdev, cled,
+ OPAL_SLOT_LED_TYPE_ID);
+
+ if (has_led_type(cled, LED_TYPE_FAULT))
+ rc = power_led_classdev(pdev, cled,
+ OPAL_SLOT_LED_TYPE_FAULT);
+ }
+ return rc;
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ struct powernv_led *pled;
+ struct powernv_led_work *pwork;
+ unsigned long flags;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ pr_info("Unregister all classdev structures\n");
+ list_for_each_entry(pled, &powernv_led_list, link)
+ led_classdev_unregister(&pled->cdev);
+
+ pr_info("Wait for all work tasks to finish\n");
+ list_for_each_entry(pwork, &powernv_led_work_list, link)
+ flush_work(&pwork->work);
+
+ /* Free nodes from the work list */
+ powernv_led_compact_work_list();
+
+ /* Free nodes from the classdev list */
+ spin_lock_irqsave(&powernv_led_spinlock, flags);
+ while (!list_empty(&powernv_led_list)) {
+ pled = list_first_entry(&powernv_led_list,
+ struct powernv_led, link);
+ list_del(&pled->link);
+ kfree(pled);
+ }
+ spin_unlock_irqrestore(&powernv_led_spinlock, flags);
+
+ /* Check for memory leaks */
+ if (!list_empty(&powernv_led_work_list))
+ pr_warn("Work list not empty, memory leak\n");
+
+ if (!list_empty(&powernv_led_cmd_list))
+ pr_warn("Request list not empty, memory leak\n");
+
+ if (!list_empty(&powernv_led_list))
+ pr_warn("Classdev list not empty, memory leak\n");
+
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Anshuman Khandual <khandual@linux.vnet.ibm.com>");
^ permalink raw reply related [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-03-20 11:04 ` Vasant Hegde
@ 2015-03-25 5:21 ` Benjamin Herrenschmidt
-1 siblings, 0 replies; 65+ messages in thread
From: Benjamin Herrenschmidt @ 2015-03-25 5:21 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, cooloney, rpurdie, khandual
On Fri, 2015-03-20 at 16:34 +0530, Vasant Hegde wrote:
> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>
> This patch implements LED driver for PowerNV platform using the existing
> generic LED class framework. It registers classdev structures for all
> individual LEDs detected on the system through LED specific device tree
> nodes. Device tree nodes specify what all kind of LEDs present on the
> same location code. It registers LED classdev structure for each of them.
>
> The platform level implementation of LED get and set state has been
> achieved through OPAL calls. These calls are made available for the
> driver by exporting from architecture specific codes.
>
> As per the LED class framework, the 'brightness_set' function should not
> sleep. Hence these functions have been implemented through global work
> queue tasks which might sleep on OPAL async call completion.
>
> All the system LEDs can be found in the same regular path /sys/class/leds/.
> There are two different kind of LEDs present for the same location code,
> one being the identify indicator and other one being the fault indicator.
> We don't use LED colors. Hence our LEDs have names in this format.
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>
> Any positive brightness value would turn on the LED and a zero value
> would turn off the LED. The driver will return LED_FULL (255) for any
> turned on LED and LED_OFF for any turned off LED.
Any comment from the LEDs folks ? I am not too familiar with the LED
subsystem so I would appreciate at least a cursory review of the high
level design :-)
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
> Changes in v2:
> - Added System Attention indicator support
> - Moved common code to powernv_led_set_queue()
>
> -Vasant
>
>
> arch/powerpc/platforms/powernv/opal.c | 12 +
> drivers/leds/Kconfig | 9
> drivers/leds/Makefile | 1
> drivers/leds/leds-powernv.c | 620 +++++++++++++++++++++++++++++++++
> 4 files changed, 641 insertions(+), 1 deletion(-)
> create mode 100644 drivers/leds/leds-powernv.c
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 142a08a..fbfd9c1 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>
> static int __init opal_init(void)
> {
> - struct device_node *np, *consoles;
> + struct device_node *np, *consoles, *led;
> int rc;
>
> opal_node = of_find_node_by_path("/ibm,opal");
> @@ -772,6 +772,13 @@ static int __init opal_init(void)
> /* Create i2c platform devices */
> opal_i2c_create_devs();
>
> + /* Create led platform devices */
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (led) {
> + of_platform_device_create(led, "opal_led", NULL);
> + of_node_put(led);
> + }
> +
> /* Find all OPAL interrupts and request them */
> opal_irq_init(opal_node);
>
> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
> EXPORT_SYMBOL_GPL(opal_tpo_read);
> EXPORT_SYMBOL_GPL(opal_tpo_write);
> EXPORT_SYMBOL_GPL(opal_i2c_request);
> +/* Export these symbols for PowerNV LED class driver */
> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 25b320d..a93223c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -508,6 +508,15 @@ 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
> + help
> + This option enables support for the system LEDs present on
> + PowerNV platforms. Say 'y' to enable this support in kernel.
> + Say 'm' enable this support as module.
> +
> 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 cbba921..604ffc9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,620 @@
> +/*
> + * PowerNV LED Driver
> + *
> + * Copyright IBM Corp. 2015
> + *
> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> + *
> + * 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.
> + */
> +
> +#define PREFIX "POWERNV_LED"
> +#define pr_fmt(fmt) PREFIX ": " fmt
> +
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +
> +#include <asm/opal.h>
> +
> +#define LED_STR_ATTENTION ":ATTENTION"
> +#define LED_STR_IDENT ":IDENTIFY"
> +#define LED_STR_FAULT ":FAULT"
> +
> +/*
> + * LED operation state
> + *
> + * led_classdev_unregister resets the brightness values. However
> + * we want to retain the LED state across boot. Hence disable
> + * LED operation before calling led_classdev_unregister.
> + */
> +static bool led_disabled = false;
> +
> +/*
> + * LED type map
> + *
> + * Converts LED event type into it's description.
> + */
> +static const char *led_type_map[OPAL_SLOT_LED_TYPE_MAX] = {
> + "Attention", "Identify", "Fault"
> +};
> +
> +/*
> + * 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. A lot of work queue
> + * tasks attempting to set the LED states can prevent each other from
> + * completing their task. So there is a need to restrict the total number
> + * of work queue tasks at any point in time related to LED.
> + */
> +#define MAX_LED_REQ_COUNT 100
> +static int led_req_count;
> +
> +/*
> + * LED classdev
> + *
> + * Each LED classdev structure registered on the platform will be put into
> + * a list which will be eventually used for de-registration purpose.
> + */
> +struct powernv_led {
> + struct led_classdev cdev;
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_list);
> +static DEFINE_SPINLOCK(powernv_led_spinlock);
> +
> +/*
> + * LED set state command
> + *
> + * Each set LED state request will be saved and put into a list to be
> + * processed later by a work queue task.
> + */
> +struct powernv_led_cmd {
> + struct led_classdev *led_cdev; /* Points to classdev structure */
> + enum led_brightness value; /* Brightness value */
> + u64 led_type; /* Identify or Fault */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_cmd_list);
> +static DEFINE_SPINLOCK(powernv_led_cmd_spinlock);
> +
> +/*
> + * LED set state task
> + *
> + * Each set LED state request added into the request list for post
> + * processing will be followed by one of this structure. The work
> + * struct here will be scheduled on the global work queue to process
> + * one of the requests from the request list. Though the order of
> + * execution is not guaraneteed.
> + */
> +struct powernv_led_work {
> + struct work_struct work; /* Individual task */
> + bool deletion; /* Need deletion */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_work_list);
> +static DEFINE_SPINLOCK(powernv_led_work_spinlock);
> +
> +/*
> + * powernv_led_compact_work_list
> + *
> + * This function goes through the entire list of scheduled powernv_led_work
> + * nodes and removes the nodes which have already processed one set LED
> + * state request from request list and has been marked for deletion. This is
> + * essential for cleaning the list before adding new elements into it. This
> + * also tracks the total number of pending tasks. Once it reaches the
> + * threshold the function will throttle till all the scheduled tasks completes
> + * execution during which the user space thread will block and will be
> + * prevented from queuing up more LED state change requests.
> + */
> +static void powernv_led_compact_work_list(void)
> +{
> + struct powernv_led_work *pwork, *pwork_ne;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + led_req_count = 0;
> + list_for_each_entry_safe(pwork, pwork_ne,
> + &powernv_led_work_list, link) {
> + if (pwork->deletion) {
> + list_del(&pwork->link);
> + kfree(pwork);
> + continue;
> + }
> + led_req_count++;
> + }
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Throttle if the threshold reached */
> + if (led_req_count == MAX_LED_REQ_COUNT) {
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> + }
> +}
> +
> +/*
> + * powernv_led_set
> + *
> + * 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, u64 led_type)
> +{
> + char *loc_code;
> + int rc, token;
> + u64 led_mask, max_led_type, led_value = 0;
> + struct opal_msg msg;
> +
> + /* Location code of the LED */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err(PREFIX "Memory allocation failed at %s\n", __func__);
> + return;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unknown LED type */
> + goto out_loc;
> + }
> +
> + /* Prepare for the OPAL call */
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
> + if (value)
> + led_value = OPAL_SLOT_LED_STATE_ON << led_type;
> +
> + /* OPAL async call */
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + pr_err("%s: Couldn't get the token, returning\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) {
> + pr_err("OPAL call opal_leds_set_ind failed for %s with %d\n",
> + loc_code, rc);
> + goto out_token;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc) {
> + pr_err("%s: Failed to wait for the async response, %d\n",
> + __func__, rc);
> + goto out_token;
> + }
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS)
> + pr_err("Async call returned with failed status: %d\n", rc);
> +
> +out_token:
> + opal_async_release_token(token);
> +
> +out_loc:
> + kfree(loc_code);
> +}
> +
> +/*
> + * powernv_led_get
> + *
> + * 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,
> + u64 led_type)
> +{
> + char *loc_code;
> + int rc;
> + u64 led_mask, led_value, max_led_type;
> +
> + /* LED location code */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unsupported LED type */
> + goto led_fail;
> + }
> +
> + /* 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) {
> + pr_err("OPAL call opal_leds_get_ind failed with %d\n", rc);
> + goto led_fail;
> + }
> +
> + led_mask = be64_to_cpu(led_mask);
> + led_value = be64_to_cpu(led_value);
> +
> + /* LED status available */
> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
> + pr_err("%s LED status not available for %s\n",
> + led_type_map[led_type], loc_code);
> + 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;
> +}
> +
> +/*
> + * powernv_led_work_func
> + *
> + * This the function which will be executed by any LED work task on the
> + * global work queue. This function de-queues one of the request node
> + * from the request list, processes it and then deletes the request node.
> + * This also accesses it's own work list node and sets the deletion flag
> + * in there making itself a candidate for removal the next time the
> + * compact function gets called.
> + */
> +static void powernv_led_work_func(struct work_struct *work)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *req;
> + unsigned long flags;
> +
> + /* De-queue one request, process it and then delete */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + if (list_empty(&powernv_led_cmd_list)) {
> + pr_err("Request list empty, but work queue task queued\n");
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> + return;
> + }
> +
> + req = list_first_entry(&powernv_led_cmd_list,
> + struct powernv_led_cmd, link);
> + list_del(&req->link);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + powernv_led_set(req->led_cdev, req->value, req->led_type);
> + kfree(req);
> +
> + /* Mark the work queue task for deletion */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + pwork = container_of(work, struct powernv_led_work, work);
> + pwork->deletion = true;
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +}
> +
> +/*
> + * powernv_led_set_queue
> + *
> + * LED classdev 'brightness_set' function for LEDs
> + */
> +static void powernv_led_set_queue(struct led_classdev *led_cdev,
> + enum led_brightness value, u64 led_type)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *attn;
> + unsigned long flags;
> +
> + if (led_disabled)
> + return;
> +
> + /* Allocate the request */
> + attn = kzalloc(sizeof(struct powernv_led_cmd), GFP_KERNEL);
> + if (!attn) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return;
> + }
> +
> + /* Allocate the work */
> + pwork = kzalloc(sizeof(struct powernv_led_work), GFP_KERNEL);
> + if (!pwork) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + kfree(attn);
> + return;
> + }
> +
> + /* Prepare the request */
> + attn->led_cdev = led_cdev;
> + attn->value = value;
> + attn->led_type = led_type;
> +
> + /* Queue the request */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + list_add_tail(&attn->link, &powernv_led_cmd_list);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + /* Compact the work list */
> + powernv_led_compact_work_list();
> +
> + /* Add the new task into the work list */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + list_add_tail(&pwork->link, &powernv_led_work_list);
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Schedule the new task */
> + INIT_WORK(&pwork->work, powernv_led_work_func);
> + schedule_work(&pwork->work);
> +}
> +/*
> + * powernv_led_set_attn
> + *
> + * LED classdev 'brightness_set' function for attention LED.
> + */
> +static void powernv_led_set_attn(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_get_attn
> + *
> + * LED classdev 'brightness_get' function for attention LED.
> + */
> +static enum led_brightness powernv_led_get_attn(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_set_ident
> + *
> + * LED classdev 'brightness_set' function for identify LED types.
> + */
> +static void powernv_led_set_ident(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_get_ident
> + *
> + * LED classdev 'brightness_get' function for identify LED types.
> + */
> +static enum led_brightness powernv_led_get_ident(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_set_fault
> + *
> + * LED classdev 'brightness_set' function for fault LED types.
> + */
> +static void powernv_led_set_fault(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * powernv_led_get_fault
> + *
> + * LED classdev 'brightness_get' function for fault LED types.
> + */
> +static enum led_brightness powernv_led_get_fault(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * has_led_type
> + *
> + * This function verifies whether the child LED device node supports certain
> + * type of LED or not. This will be used to register LEDclassdev structures
> + * for that particual type of LED for a given device tree node.
> + */
> +static bool has_led_type(struct device_node *cled, const char *led_type)
> +{
> + bool result = false;
> +
> + if (of_property_match_string(cled, "led-types", led_type) >= 0)
> + result = true;
> +
> + return result;
> +}
> +
> +/*
> + * power_led_classdev
> + *
> + * This function registers classdev structure for any given type of LED on
> + * a given child LED device node.
> + */
> +static int power_led_classdev(struct platform_device *pdev,
> + struct device_node *cled, u64 led_type)
> +{
> + int rc;
> + unsigned long flags;
> + struct powernv_led *cpled;
> +
> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
> + if (!cpled) {
> + pr_err("Memory allocation failed at %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + /* Create the name for classdev */
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_ATTENTION);
> + cpled->cdev.brightness_set = powernv_led_set_attn;
> + cpled->cdev.brightness_get = powernv_led_get_attn;
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_IDENT);
> + cpled->cdev.brightness_set = powernv_led_set_ident;
> + cpled->cdev.brightness_get = powernv_led_get_ident;
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_FAULT);
> + cpled->cdev.brightness_set = powernv_led_set_fault;
> + cpled->cdev.brightness_get = powernv_led_get_fault;
> + break;
> + default: /* Unsupported LED type */
> + return -EINVAL;
> + }
> +
> + if (!cpled->cdev.name) {
> + pr_err("Memory allocation failed for classdev name\n");
> + return -ENOMEM;
> + }
> +
> + cpled->cdev.brightness = LED_OFF;
> + cpled->cdev.max_brightness = LED_FULL;
> + cpled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> + /* Register the classdev */
> + rc = led_classdev_register(&pdev->dev, &cpled->cdev);
> + if (rc) {
> + pr_err("Classdev registration failed for %s\n",
> + cpled->cdev.name);
> + } else {
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + list_add_tail(&cpled->link, &powernv_led_list);
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> + pr_debug("Classdev registration successful for %s\n",
> + cpled->cdev.name);
> + }
> + return rc;
> +}
> +
> +/* Platform driver probe */
> +static int powernv_led_probe(struct platform_device *pdev)
> +{
> + struct device_node *led, *cled;
> + int rc = 0;
> +
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (!led) {
> + pr_err("LED parent device node not found\n");
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(led, cled) {
> + if (has_led_type(cled, LED_TYPE_ATTENTION))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +
> + if (has_led_type(cled, LED_TYPE_IDENTIFY))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ID);
> +
> + if (has_led_type(cled, LED_TYPE_FAULT))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_FAULT);
> + }
> + return rc;
> +}
> +
> +/* Platform driver remove */
> +static int powernv_led_remove(struct platform_device *pdev)
> +{
> + struct powernv_led *pled;
> + struct powernv_led_work *pwork;
> + unsigned long flags;
> +
> + /* Disable LED operation */
> + led_disabled = true;
> +
> + pr_info("Unregister all classdev structures\n");
> + list_for_each_entry(pled, &powernv_led_list, link)
> + led_classdev_unregister(&pled->cdev);
> +
> + pr_info("Wait for all work tasks to finish\n");
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> +
> + /* Free nodes from the work list */
> + powernv_led_compact_work_list();
> +
> + /* Free nodes from the classdev list */
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + while (!list_empty(&powernv_led_list)) {
> + pled = list_first_entry(&powernv_led_list,
> + struct powernv_led, link);
> + list_del(&pled->link);
> + kfree(pled);
> + }
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> +
> + /* Check for memory leaks */
> + if (!list_empty(&powernv_led_work_list))
> + pr_warn("Work list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_cmd_list))
> + pr_warn("Request list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_list))
> + pr_warn("Classdev list not empty, memory leak\n");
> +
> + return 0;
> +}
> +
> +/* Platform driver property match */
> +static const struct of_device_id powernv_led_match[] = {
> + {
> + .compatible = "ibm,opal-v3-led",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, powernv_led_match);
> +
> +static struct platform_driver powernv_led_driver = {
> + .probe = powernv_led_probe,
> + .remove = powernv_led_remove,
> + .driver = {
> + .name = "powernv-led-driver",
> + .owner = THIS_MODULE,
> + .of_match_table = powernv_led_match,
> + },
> +};
> +
> +module_platform_driver(powernv_led_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("PowerNV LED driver");
> +MODULE_AUTHOR("Anshuman Khandual <khandual@linux.vnet.ibm.com>");
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-03-25 5:21 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 65+ messages in thread
From: Benjamin Herrenschmidt @ 2015-03-25 5:21 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On Fri, 2015-03-20 at 16:34 +0530, Vasant Hegde wrote:
> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>
> This patch implements LED driver for PowerNV platform using the existing
> generic LED class framework. It registers classdev structures for all
> individual LEDs detected on the system through LED specific device tree
> nodes. Device tree nodes specify what all kind of LEDs present on the
> same location code. It registers LED classdev structure for each of them.
>
> The platform level implementation of LED get and set state has been
> achieved through OPAL calls. These calls are made available for the
> driver by exporting from architecture specific codes.
>
> As per the LED class framework, the 'brightness_set' function should not
> sleep. Hence these functions have been implemented through global work
> queue tasks which might sleep on OPAL async call completion.
>
> All the system LEDs can be found in the same regular path /sys/class/leds/.
> There are two different kind of LEDs present for the same location code,
> one being the identify indicator and other one being the fault indicator.
> We don't use LED colors. Hence our LEDs have names in this format.
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>
> Any positive brightness value would turn on the LED and a zero value
> would turn off the LED. The driver will return LED_FULL (255) for any
> turned on LED and LED_OFF for any turned off LED.
Any comment from the LEDs folks ? I am not too familiar with the LED
subsystem so I would appreciate at least a cursory review of the high
level design :-)
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
> Changes in v2:
> - Added System Attention indicator support
> - Moved common code to powernv_led_set_queue()
>
> -Vasant
>
>
> arch/powerpc/platforms/powernv/opal.c | 12 +
> drivers/leds/Kconfig | 9
> drivers/leds/Makefile | 1
> drivers/leds/leds-powernv.c | 620 +++++++++++++++++++++++++++++++++
> 4 files changed, 641 insertions(+), 1 deletion(-)
> create mode 100644 drivers/leds/leds-powernv.c
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 142a08a..fbfd9c1 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>
> static int __init opal_init(void)
> {
> - struct device_node *np, *consoles;
> + struct device_node *np, *consoles, *led;
> int rc;
>
> opal_node = of_find_node_by_path("/ibm,opal");
> @@ -772,6 +772,13 @@ static int __init opal_init(void)
> /* Create i2c platform devices */
> opal_i2c_create_devs();
>
> + /* Create led platform devices */
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (led) {
> + of_platform_device_create(led, "opal_led", NULL);
> + of_node_put(led);
> + }
> +
> /* Find all OPAL interrupts and request them */
> opal_irq_init(opal_node);
>
> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
> EXPORT_SYMBOL_GPL(opal_tpo_read);
> EXPORT_SYMBOL_GPL(opal_tpo_write);
> EXPORT_SYMBOL_GPL(opal_i2c_request);
> +/* Export these symbols for PowerNV LED class driver */
> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 25b320d..a93223c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -508,6 +508,15 @@ 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
> + help
> + This option enables support for the system LEDs present on
> + PowerNV platforms. Say 'y' to enable this support in kernel.
> + Say 'm' enable this support as module.
> +
> 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 cbba921..604ffc9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,620 @@
> +/*
> + * PowerNV LED Driver
> + *
> + * Copyright IBM Corp. 2015
> + *
> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> + *
> + * 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.
> + */
> +
> +#define PREFIX "POWERNV_LED"
> +#define pr_fmt(fmt) PREFIX ": " fmt
> +
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +
> +#include <asm/opal.h>
> +
> +#define LED_STR_ATTENTION ":ATTENTION"
> +#define LED_STR_IDENT ":IDENTIFY"
> +#define LED_STR_FAULT ":FAULT"
> +
> +/*
> + * LED operation state
> + *
> + * led_classdev_unregister resets the brightness values. However
> + * we want to retain the LED state across boot. Hence disable
> + * LED operation before calling led_classdev_unregister.
> + */
> +static bool led_disabled = false;
> +
> +/*
> + * LED type map
> + *
> + * Converts LED event type into it's description.
> + */
> +static const char *led_type_map[OPAL_SLOT_LED_TYPE_MAX] = {
> + "Attention", "Identify", "Fault"
> +};
> +
> +/*
> + * 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. A lot of work queue
> + * tasks attempting to set the LED states can prevent each other from
> + * completing their task. So there is a need to restrict the total number
> + * of work queue tasks at any point in time related to LED.
> + */
> +#define MAX_LED_REQ_COUNT 100
> +static int led_req_count;
> +
> +/*
> + * LED classdev
> + *
> + * Each LED classdev structure registered on the platform will be put into
> + * a list which will be eventually used for de-registration purpose.
> + */
> +struct powernv_led {
> + struct led_classdev cdev;
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_list);
> +static DEFINE_SPINLOCK(powernv_led_spinlock);
> +
> +/*
> + * LED set state command
> + *
> + * Each set LED state request will be saved and put into a list to be
> + * processed later by a work queue task.
> + */
> +struct powernv_led_cmd {
> + struct led_classdev *led_cdev; /* Points to classdev structure */
> + enum led_brightness value; /* Brightness value */
> + u64 led_type; /* Identify or Fault */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_cmd_list);
> +static DEFINE_SPINLOCK(powernv_led_cmd_spinlock);
> +
> +/*
> + * LED set state task
> + *
> + * Each set LED state request added into the request list for post
> + * processing will be followed by one of this structure. The work
> + * struct here will be scheduled on the global work queue to process
> + * one of the requests from the request list. Though the order of
> + * execution is not guaraneteed.
> + */
> +struct powernv_led_work {
> + struct work_struct work; /* Individual task */
> + bool deletion; /* Need deletion */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_work_list);
> +static DEFINE_SPINLOCK(powernv_led_work_spinlock);
> +
> +/*
> + * powernv_led_compact_work_list
> + *
> + * This function goes through the entire list of scheduled powernv_led_work
> + * nodes and removes the nodes which have already processed one set LED
> + * state request from request list and has been marked for deletion. This is
> + * essential for cleaning the list before adding new elements into it. This
> + * also tracks the total number of pending tasks. Once it reaches the
> + * threshold the function will throttle till all the scheduled tasks completes
> + * execution during which the user space thread will block and will be
> + * prevented from queuing up more LED state change requests.
> + */
> +static void powernv_led_compact_work_list(void)
> +{
> + struct powernv_led_work *pwork, *pwork_ne;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + led_req_count = 0;
> + list_for_each_entry_safe(pwork, pwork_ne,
> + &powernv_led_work_list, link) {
> + if (pwork->deletion) {
> + list_del(&pwork->link);
> + kfree(pwork);
> + continue;
> + }
> + led_req_count++;
> + }
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Throttle if the threshold reached */
> + if (led_req_count == MAX_LED_REQ_COUNT) {
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> + }
> +}
> +
> +/*
> + * powernv_led_set
> + *
> + * 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, u64 led_type)
> +{
> + char *loc_code;
> + int rc, token;
> + u64 led_mask, max_led_type, led_value = 0;
> + struct opal_msg msg;
> +
> + /* Location code of the LED */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err(PREFIX "Memory allocation failed at %s\n", __func__);
> + return;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unknown LED type */
> + goto out_loc;
> + }
> +
> + /* Prepare for the OPAL call */
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
> + if (value)
> + led_value = OPAL_SLOT_LED_STATE_ON << led_type;
> +
> + /* OPAL async call */
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + pr_err("%s: Couldn't get the token, returning\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) {
> + pr_err("OPAL call opal_leds_set_ind failed for %s with %d\n",
> + loc_code, rc);
> + goto out_token;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc) {
> + pr_err("%s: Failed to wait for the async response, %d\n",
> + __func__, rc);
> + goto out_token;
> + }
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS)
> + pr_err("Async call returned with failed status: %d\n", rc);
> +
> +out_token:
> + opal_async_release_token(token);
> +
> +out_loc:
> + kfree(loc_code);
> +}
> +
> +/*
> + * powernv_led_get
> + *
> + * 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,
> + u64 led_type)
> +{
> + char *loc_code;
> + int rc;
> + u64 led_mask, led_value, max_led_type;
> +
> + /* LED location code */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unsupported LED type */
> + goto led_fail;
> + }
> +
> + /* 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) {
> + pr_err("OPAL call opal_leds_get_ind failed with %d\n", rc);
> + goto led_fail;
> + }
> +
> + led_mask = be64_to_cpu(led_mask);
> + led_value = be64_to_cpu(led_value);
> +
> + /* LED status available */
> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
> + pr_err("%s LED status not available for %s\n",
> + led_type_map[led_type], loc_code);
> + 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;
> +}
> +
> +/*
> + * powernv_led_work_func
> + *
> + * This the function which will be executed by any LED work task on the
> + * global work queue. This function de-queues one of the request node
> + * from the request list, processes it and then deletes the request node.
> + * This also accesses it's own work list node and sets the deletion flag
> + * in there making itself a candidate for removal the next time the
> + * compact function gets called.
> + */
> +static void powernv_led_work_func(struct work_struct *work)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *req;
> + unsigned long flags;
> +
> + /* De-queue one request, process it and then delete */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + if (list_empty(&powernv_led_cmd_list)) {
> + pr_err("Request list empty, but work queue task queued\n");
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> + return;
> + }
> +
> + req = list_first_entry(&powernv_led_cmd_list,
> + struct powernv_led_cmd, link);
> + list_del(&req->link);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + powernv_led_set(req->led_cdev, req->value, req->led_type);
> + kfree(req);
> +
> + /* Mark the work queue task for deletion */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + pwork = container_of(work, struct powernv_led_work, work);
> + pwork->deletion = true;
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +}
> +
> +/*
> + * powernv_led_set_queue
> + *
> + * LED classdev 'brightness_set' function for LEDs
> + */
> +static void powernv_led_set_queue(struct led_classdev *led_cdev,
> + enum led_brightness value, u64 led_type)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *attn;
> + unsigned long flags;
> +
> + if (led_disabled)
> + return;
> +
> + /* Allocate the request */
> + attn = kzalloc(sizeof(struct powernv_led_cmd), GFP_KERNEL);
> + if (!attn) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return;
> + }
> +
> + /* Allocate the work */
> + pwork = kzalloc(sizeof(struct powernv_led_work), GFP_KERNEL);
> + if (!pwork) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + kfree(attn);
> + return;
> + }
> +
> + /* Prepare the request */
> + attn->led_cdev = led_cdev;
> + attn->value = value;
> + attn->led_type = led_type;
> +
> + /* Queue the request */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + list_add_tail(&attn->link, &powernv_led_cmd_list);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + /* Compact the work list */
> + powernv_led_compact_work_list();
> +
> + /* Add the new task into the work list */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + list_add_tail(&pwork->link, &powernv_led_work_list);
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Schedule the new task */
> + INIT_WORK(&pwork->work, powernv_led_work_func);
> + schedule_work(&pwork->work);
> +}
> +/*
> + * powernv_led_set_attn
> + *
> + * LED classdev 'brightness_set' function for attention LED.
> + */
> +static void powernv_led_set_attn(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_get_attn
> + *
> + * LED classdev 'brightness_get' function for attention LED.
> + */
> +static enum led_brightness powernv_led_get_attn(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_set_ident
> + *
> + * LED classdev 'brightness_set' function for identify LED types.
> + */
> +static void powernv_led_set_ident(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_get_ident
> + *
> + * LED classdev 'brightness_get' function for identify LED types.
> + */
> +static enum led_brightness powernv_led_get_ident(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_set_fault
> + *
> + * LED classdev 'brightness_set' function for fault LED types.
> + */
> +static void powernv_led_set_fault(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * powernv_led_get_fault
> + *
> + * LED classdev 'brightness_get' function for fault LED types.
> + */
> +static enum led_brightness powernv_led_get_fault(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * has_led_type
> + *
> + * This function verifies whether the child LED device node supports certain
> + * type of LED or not. This will be used to register LEDclassdev structures
> + * for that particual type of LED for a given device tree node.
> + */
> +static bool has_led_type(struct device_node *cled, const char *led_type)
> +{
> + bool result = false;
> +
> + if (of_property_match_string(cled, "led-types", led_type) >= 0)
> + result = true;
> +
> + return result;
> +}
> +
> +/*
> + * power_led_classdev
> + *
> + * This function registers classdev structure for any given type of LED on
> + * a given child LED device node.
> + */
> +static int power_led_classdev(struct platform_device *pdev,
> + struct device_node *cled, u64 led_type)
> +{
> + int rc;
> + unsigned long flags;
> + struct powernv_led *cpled;
> +
> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
> + if (!cpled) {
> + pr_err("Memory allocation failed at %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + /* Create the name for classdev */
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_ATTENTION);
> + cpled->cdev.brightness_set = powernv_led_set_attn;
> + cpled->cdev.brightness_get = powernv_led_get_attn;
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_IDENT);
> + cpled->cdev.brightness_set = powernv_led_set_ident;
> + cpled->cdev.brightness_get = powernv_led_get_ident;
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_FAULT);
> + cpled->cdev.brightness_set = powernv_led_set_fault;
> + cpled->cdev.brightness_get = powernv_led_get_fault;
> + break;
> + default: /* Unsupported LED type */
> + return -EINVAL;
> + }
> +
> + if (!cpled->cdev.name) {
> + pr_err("Memory allocation failed for classdev name\n");
> + return -ENOMEM;
> + }
> +
> + cpled->cdev.brightness = LED_OFF;
> + cpled->cdev.max_brightness = LED_FULL;
> + cpled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> + /* Register the classdev */
> + rc = led_classdev_register(&pdev->dev, &cpled->cdev);
> + if (rc) {
> + pr_err("Classdev registration failed for %s\n",
> + cpled->cdev.name);
> + } else {
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + list_add_tail(&cpled->link, &powernv_led_list);
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> + pr_debug("Classdev registration successful for %s\n",
> + cpled->cdev.name);
> + }
> + return rc;
> +}
> +
> +/* Platform driver probe */
> +static int powernv_led_probe(struct platform_device *pdev)
> +{
> + struct device_node *led, *cled;
> + int rc = 0;
> +
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (!led) {
> + pr_err("LED parent device node not found\n");
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(led, cled) {
> + if (has_led_type(cled, LED_TYPE_ATTENTION))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +
> + if (has_led_type(cled, LED_TYPE_IDENTIFY))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ID);
> +
> + if (has_led_type(cled, LED_TYPE_FAULT))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_FAULT);
> + }
> + return rc;
> +}
> +
> +/* Platform driver remove */
> +static int powernv_led_remove(struct platform_device *pdev)
> +{
> + struct powernv_led *pled;
> + struct powernv_led_work *pwork;
> + unsigned long flags;
> +
> + /* Disable LED operation */
> + led_disabled = true;
> +
> + pr_info("Unregister all classdev structures\n");
> + list_for_each_entry(pled, &powernv_led_list, link)
> + led_classdev_unregister(&pled->cdev);
> +
> + pr_info("Wait for all work tasks to finish\n");
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> +
> + /* Free nodes from the work list */
> + powernv_led_compact_work_list();
> +
> + /* Free nodes from the classdev list */
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + while (!list_empty(&powernv_led_list)) {
> + pled = list_first_entry(&powernv_led_list,
> + struct powernv_led, link);
> + list_del(&pled->link);
> + kfree(pled);
> + }
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> +
> + /* Check for memory leaks */
> + if (!list_empty(&powernv_led_work_list))
> + pr_warn("Work list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_cmd_list))
> + pr_warn("Request list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_list))
> + pr_warn("Classdev list not empty, memory leak\n");
> +
> + return 0;
> +}
> +
> +/* Platform driver property match */
> +static const struct of_device_id powernv_led_match[] = {
> + {
> + .compatible = "ibm,opal-v3-led",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, powernv_led_match);
> +
> +static struct platform_driver powernv_led_driver = {
> + .probe = powernv_led_probe,
> + .remove = powernv_led_remove,
> + .driver = {
> + .name = "powernv-led-driver",
> + .owner = THIS_MODULE,
> + .of_match_table = powernv_led_match,
> + },
> +};
> +
> +module_platform_driver(powernv_led_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("PowerNV LED driver");
> +MODULE_AUTHOR("Anshuman Khandual <khandual@linux.vnet.ibm.com>");
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-03-25 5:21 ` Benjamin Herrenschmidt
@ 2015-04-14 5:40 ` Vasant Hegde
-1 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-14 5:40 UTC (permalink / raw)
To: cooloney, rpurdie
Cc: Benjamin Herrenschmidt, linuxppc-dev, linux-leds, stewart, khandual
On 03/25/2015 10:51 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2015-03-20 at 16:34 +0530, Vasant Hegde wrote:
>> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>
>> This patch implements LED driver for PowerNV platform using the existing
>> generic LED class framework. It registers classdev structures for all
>> individual LEDs detected on the system through LED specific device tree
>> nodes. Device tree nodes specify what all kind of LEDs present on the
>> same location code. It registers LED classdev structure for each of them.
>>
>> The platform level implementation of LED get and set state has been
>> achieved through OPAL calls. These calls are made available for the
>> driver by exporting from architecture specific codes.
>>
>> As per the LED class framework, the 'brightness_set' function should not
>> sleep. Hence these functions have been implemented through global work
>> queue tasks which might sleep on OPAL async call completion.
>>
>> All the system LEDs can be found in the same regular path /sys/class/leds/.
>> There are two different kind of LEDs present for the same location code,
>> one being the identify indicator and other one being the fault indicator.
>> We don't use LED colors. Hence our LEDs have names in this format.
>>
>> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>>
>> Any positive brightness value would turn on the LED and a zero value
>> would turn off the LED. The driver will return LED_FULL (255) for any
>> turned on LED and LED_OFF for any turned off LED.
>
Bryan, Richard,
Did you get a chance to review this patchset?
-Vasant
> Any comment from the LEDs folks ? I am not too familiar with the LED
> subsystem so I would appreciate at least a cursory review of the high
> level design :-)
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-14 5:40 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-14 5:40 UTC (permalink / raw)
To: cooloney, rpurdie; +Cc: stewart, linuxppc-dev, linux-leds, khandual
On 03/25/2015 10:51 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2015-03-20 at 16:34 +0530, Vasant Hegde wrote:
>> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>
>> This patch implements LED driver for PowerNV platform using the existing
>> generic LED class framework. It registers classdev structures for all
>> individual LEDs detected on the system through LED specific device tree
>> nodes. Device tree nodes specify what all kind of LEDs present on the
>> same location code. It registers LED classdev structure for each of them.
>>
>> The platform level implementation of LED get and set state has been
>> achieved through OPAL calls. These calls are made available for the
>> driver by exporting from architecture specific codes.
>>
>> As per the LED class framework, the 'brightness_set' function should not
>> sleep. Hence these functions have been implemented through global work
>> queue tasks which might sleep on OPAL async call completion.
>>
>> All the system LEDs can be found in the same regular path /sys/class/leds/.
>> There are two different kind of LEDs present for the same location code,
>> one being the identify indicator and other one being the fault indicator.
>> We don't use LED colors. Hence our LEDs have names in this format.
>>
>> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>>
>> Any positive brightness value would turn on the LED and a zero value
>> would turn off the LED. The driver will return LED_FULL (255) for any
>> turned on LED and LED_OFF for any turned off LED.
>
Bryan, Richard,
Did you get a chance to review this patchset?
-Vasant
> Any comment from the LEDs folks ? I am not too familiar with the LED
> subsystem so I would appreciate at least a cursory review of the high
> level design :-)
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-03-20 11:04 ` Vasant Hegde
@ 2015-04-14 15:20 ` Jacek Anaszewski
-1 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-14 15:20 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
Hi Vasant,
On 03/20/2015 12:04 PM, Vasant Hegde wrote:
> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>
> This patch implements LED driver for PowerNV platform using the existing
> generic LED class framework. It registers classdev structures for all
> individual LEDs detected on the system through LED specific device tree
> nodes. Device tree nodes specify what all kind of LEDs present on the
> same location code. It registers LED classdev structure for each of them.
>
> The platform level implementation of LED get and set state has been
> achieved through OPAL calls. These calls are made available for the
> driver by exporting from architecture specific codes.
>
> As per the LED class framework, the 'brightness_set' function should not
> sleep. Hence these functions have been implemented through global work
> queue tasks which might sleep on OPAL async call completion.
Wouldn't it be easier to implement synchronization on the OPAL side?
> All the system LEDs can be found in the same regular path /sys/class/leds/.
> There are two different kind of LEDs present for the same location code,
> one being the identify indicator and other one being the fault indicator.
> We don't use LED colors. Hence our LEDs have names in this format.
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>
> Any positive brightness value would turn on the LED and a zero value
> would turn off the LED. The driver will return LED_FULL (255) for any
> turned on LED and LED_OFF for any turned off LED.
>
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
> Changes in v2:
> - Added System Attention indicator support
> - Moved common code to powernv_led_set_queue()
>
> -Vasant
>
>
> arch/powerpc/platforms/powernv/opal.c | 12 +
> drivers/leds/Kconfig | 9
> drivers/leds/Makefile | 1
> drivers/leds/leds-powernv.c | 620 +++++++++++++++++++++++++++++++++
> 4 files changed, 641 insertions(+), 1 deletion(-)
> create mode 100644 drivers/leds/leds-powernv.c
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 142a08a..fbfd9c1 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>
> static int __init opal_init(void)
> {
> - struct device_node *np, *consoles;
> + struct device_node *np, *consoles, *led;
> int rc;
>
> opal_node = of_find_node_by_path("/ibm,opal");
> @@ -772,6 +772,13 @@ static int __init opal_init(void)
> /* Create i2c platform devices */
> opal_i2c_create_devs();
>
> + /* Create led platform devices */
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (led) {
> + of_platform_device_create(led, "opal_led", NULL);
> + of_node_put(led);
> + }
> +
> /* Find all OPAL interrupts and request them */
> opal_irq_init(opal_node);
>
> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
> EXPORT_SYMBOL_GPL(opal_tpo_read);
> EXPORT_SYMBOL_GPL(opal_tpo_write);
> EXPORT_SYMBOL_GPL(opal_i2c_request);
> +/* Export these symbols for PowerNV LED class driver */
> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
Please split the above part to the separate patch and put it in the
series before this one.
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 25b320d..a93223c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -508,6 +508,15 @@ 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
OF dependency is missing here.
> + help
> + This option enables support for the system LEDs present on
> + PowerNV platforms. Say 'y' to enable this support in kernel.
> + Say 'm' enable this support as module.
Please change the last line to:
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 cbba921..604ffc9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,620 @@
> +/*
> + * PowerNV LED Driver
> + *
> + * Copyright IBM Corp. 2015
> + *
> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> + *
> + * 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.
> + */
> +
> +#define PREFIX "POWERNV_LED"
> +#define pr_fmt(fmt) PREFIX ": " fmt
Wouldn't you mind using dev_* prefixed logging?
Unless you have a good reason not to do it.
> +
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
Please keep alphabetical order.
> +
> +#include <asm/opal.h>
> +
> +#define LED_STR_ATTENTION ":ATTENTION"
> +#define LED_STR_IDENT ":IDENTIFY"
> +#define LED_STR_FAULT ":FAULT"
Namespacing prefix is required here. LED is reserved for
LED subsystem global macros. How about POWERNV_LED_* ?
> +
> +/*
> + * LED operation state
> + *
> + * led_classdev_unregister resets the brightness values. However
> + * we want to retain the LED state across boot.
What boot are you thinking of here?
> Hence disable
> + * LED operation before calling led_classdev_unregister.
> + */
This comment is unclear for me, as this is static initialization,
unrelated to to any function call.
> +static bool led_disabled = false;
Static variables are initialized to 0 by default.
> +
> +/*
> + * LED type map
> + *
> + * Converts LED event type into it's description.
> + */
> +static const char *led_type_map[OPAL_SLOT_LED_TYPE_MAX] = {
> + "Attention", "Identify", "Fault"
> +};
> +
> +/*
> + * 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. A lot of work queue
> + * tasks attempting to set the LED states can prevent each other from
> + * completing their task. So there is a need to restrict the total number
> + * of work queue tasks at any point in time related to LED.
> + */
> +#define MAX_LED_REQ_COUNT 100
> +static int led_req_count;
> +
> +/*
> + * LED classdev
> + *
> + * Each LED classdev structure registered on the platform will be put into
> + * a list which will be eventually used for de-registration purpose.
> + */
> +struct powernv_led {
> + struct led_classdev cdev;
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_list);
> +static DEFINE_SPINLOCK(powernv_led_spinlock);
> +
> +/*
> + * LED set state command
> + *
> + * Each set LED state request will be saved and put into a list to be
> + * processed later by a work queue task.
> + */
> +struct powernv_led_cmd {
> + struct led_classdev *led_cdev; /* Points to classdev structure */
> + enum led_brightness value; /* Brightness value */
> + u64 led_type; /* Identify or Fault */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_cmd_list);
> +static DEFINE_SPINLOCK(powernv_led_cmd_spinlock);
> +
> +/*
> + * LED set state task
> + *
> + * Each set LED state request added into the request list for post
> + * processing will be followed by one of this structure. The work
> + * struct here will be scheduled on the global work queue to process
> + * one of the requests from the request list. Though the order of
> + * execution is not guaraneteed.
> + */
> +struct powernv_led_work {
> + struct work_struct work; /* Individual task */
> + bool deletion; /* Need deletion */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_work_list);
> +static DEFINE_SPINLOCK(powernv_led_work_spinlock);
> +
> +/*
> + * powernv_led_compact_work_list
> + *
> + * This function goes through the entire list of scheduled powernv_led_work
> + * nodes and removes the nodes which have already processed one set LED
> + * state request from request list and has been marked for deletion. This is
> + * essential for cleaning the list before adding new elements into it. This
> + * also tracks the total number of pending tasks. Once it reaches the
> + * threshold the function will throttle till all the scheduled tasks completes
> + * execution during which the user space thread will block and will be
> + * prevented from queuing up more LED state change requests.
> + */
Did you test the driver with led-triggers?
> +static void powernv_led_compact_work_list(void)
> +{
> + struct powernv_led_work *pwork, *pwork_ne;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + led_req_count = 0;
> + list_for_each_entry_safe(pwork, pwork_ne,
> + &powernv_led_work_list, link) {
> + if (pwork->deletion) {
> + list_del(&pwork->link);
> + kfree(pwork);
> + continue;
> + }
> + led_req_count++;
> + }
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Throttle if the threshold reached */
> + if (led_req_count == MAX_LED_REQ_COUNT) {
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> + }
> +}
> +
> +/*
> + * powernv_led_set
> + *
> + * 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, u64 led_type)
> +{
> + char *loc_code;
> + int rc, token;
> + u64 led_mask, max_led_type, led_value = 0;
> + struct opal_msg msg;
> +
> + /* Location code of the LED */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err(PREFIX "Memory allocation failed at %s\n", __func__);
> + return;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unknown LED type */
> + goto out_loc;
> + }
Above sequence is repeated in the function below - it could be wrapped
with a new function.
> + /* Prepare for the OPAL call */
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
> + if (value)
> + led_value = OPAL_SLOT_LED_STATE_ON << led_type;
> +
> + /* OPAL async call */
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + pr_err("%s: Couldn't get the token, returning\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) {
> + pr_err("OPAL call opal_leds_set_ind failed for %s with %d\n",
> + loc_code, rc);
> + goto out_token;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc) {
> + pr_err("%s: Failed to wait for the async response, %d\n",
> + __func__, rc);
> + goto out_token;
> + }
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS)
> + pr_err("Async call returned with failed status: %d\n", rc);
> +
> +out_token:
> + opal_async_release_token(token);
> +
> +out_loc:
> + kfree(loc_code);
> +}
> +
> +/*
> + * powernv_led_get
> + *
> + * 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,
> + u64 led_type)
> +{
> + char *loc_code;
> + int rc;
> + u64 led_mask, led_value, max_led_type;
> +
> + /* LED location code */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unsupported LED type */
> + goto led_fail;
> + }
> +
> + /* 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) {
> + pr_err("OPAL call opal_leds_get_ind failed with %d\n", rc);
> + goto led_fail;
> + }
> +
> + led_mask = be64_to_cpu(led_mask);
> + led_value = be64_to_cpu(led_value);
> +
> + /* LED status available */
> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
> + pr_err("%s LED status not available for %s\n",
> + led_type_map[led_type], loc_code);
> + 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;
> +}
> +
> +/*
> + * powernv_led_work_func
> + *
> + * This the function which will be executed by any LED work task on the
> + * global work queue. This function de-queues one of the request node
> + * from the request list, processes it and then deletes the request node.
> + * This also accesses it's own work list node and sets the deletion flag
> + * in there making itself a candidate for removal the next time the
> + * compact function gets called.
> + */
> +static void powernv_led_work_func(struct work_struct *work)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *req;
> + unsigned long flags;
> +
> + /* De-queue one request, process it and then delete */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + if (list_empty(&powernv_led_cmd_list)) {
> + pr_err("Request list empty, but work queue task queued\n");
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> + return;
> + }
> +
> + req = list_first_entry(&powernv_led_cmd_list,
> + struct powernv_led_cmd, link);
> + list_del(&req->link);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + powernv_led_set(req->led_cdev, req->value, req->led_type);
> + kfree(req);
> +
> + /* Mark the work queue task for deletion */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + pwork = container_of(work, struct powernv_led_work, work);
> + pwork->deletion = true;
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +}
> +
> +/*
> + * powernv_led_set_queue
> + *
> + * LED classdev 'brightness_set' function for LEDs
> + */
> +static void powernv_led_set_queue(struct led_classdev *led_cdev,
> + enum led_brightness value, u64 led_type)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *attn;
> + unsigned long flags;
> +
> + if (led_disabled)
> + return;
> +
> + /* Allocate the request */
> + attn = kzalloc(sizeof(struct powernv_led_cmd), GFP_KERNEL);
> + if (!attn) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return;
> + }
> +
> + /* Allocate the work */
> + pwork = kzalloc(sizeof(struct powernv_led_work), GFP_KERNEL);
> + if (!pwork) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + kfree(attn);
> + return;
> + }
> +
> + /* Prepare the request */
> + attn->led_cdev = led_cdev;
> + attn->value = value;
> + attn->led_type = led_type;
> +
> + /* Queue the request */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + list_add_tail(&attn->link, &powernv_led_cmd_list);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + /* Compact the work list */
> + powernv_led_compact_work_list();
> +
> + /* Add the new task into the work list */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + list_add_tail(&pwork->link, &powernv_led_work_list);
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Schedule the new task */
> + INIT_WORK(&pwork->work, powernv_led_work_func);
> + schedule_work(&pwork->work);
> +}
> +/*
> + * powernv_led_set_attn
> + *
> + * LED classdev 'brightness_set' function for attention LED.
> + */
> +static void powernv_led_set_attn(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_get_attn
> + *
> + * LED classdev 'brightness_get' function for attention LED.
> + */
> +static enum led_brightness powernv_led_get_attn(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_set_ident
> + *
> + * LED classdev 'brightness_set' function for identify LED types.
> + */
> +static void powernv_led_set_ident(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_get_ident
> + *
> + * LED classdev 'brightness_get' function for identify LED types.
> + */
> +static enum led_brightness powernv_led_get_ident(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_set_fault
> + *
> + * LED classdev 'brightness_set' function for fault LED types.
> + */
> +static void powernv_led_set_fault(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * powernv_led_get_fault
> + *
> + * LED classdev 'brightness_get' function for fault LED types.
> + */
> +static enum led_brightness powernv_led_get_fault(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * has_led_type
> + *
> + * This function verifies whether the child LED device node supports certain
> + * type of LED or not. This will be used to register LEDclassdev structures
> + * for that particual type of LED for a given device tree node.
> + */
> +static bool has_led_type(struct device_node *cled, const char *led_type)
> +{
> + bool result = false;
> +
> + if (of_property_match_string(cled, "led-types", led_type) >= 0)
> + result = true;
> +
> + return result;
> +}
> +
> +/*
> + * power_led_classdev
> + *
> + * This function registers classdev structure for any given type of LED on
> + * a given child LED device node.
> + */
> +static int power_led_classdev(struct platform_device *pdev,
> + struct device_node *cled, u64 led_type)
> +{
> + int rc;
> + unsigned long flags;
> + struct powernv_led *cpled;
> +
> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
> + if (!cpled) {
> + pr_err("Memory allocation failed at %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + /* Create the name for classdev */
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_ATTENTION);
We have a 'label' DT property for naming LED Flash class devices.
Please refer to Documentation/devicetree/bindings/leds/common.txt.
> + cpled->cdev.brightness_set = powernv_led_set_attn;
> + cpled->cdev.brightness_get = powernv_led_get_attn;
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_IDENT);
> + cpled->cdev.brightness_set = powernv_led_set_ident;
> + cpled->cdev.brightness_get = powernv_led_get_ident;
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_FAULT);
> + cpled->cdev.brightness_set = powernv_led_set_fault;
> + cpled->cdev.brightness_get = powernv_led_get_fault;
> + break;
> + default: /* Unsupported LED type */
> + return -EINVAL;
> + }
> +
> + if (!cpled->cdev.name) {
> + pr_err("Memory allocation failed for classdev name\n");
> + return -ENOMEM;
> + }
> +
> + cpled->cdev.brightness = LED_OFF;
> + cpled->cdev.max_brightness = LED_FULL;
> + cpled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> + /* Register the classdev */
> + rc = led_classdev_register(&pdev->dev, &cpled->cdev);
> + if (rc) {
> + pr_err("Classdev registration failed for %s\n",
> + cpled->cdev.name);
> + } else {
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + list_add_tail(&cpled->link, &powernv_led_list);
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> + pr_debug("Classdev registration successful for %s\n",
> + cpled->cdev.name);
> + }
> + return rc;
> +}
> +
> +/* Platform driver probe */
> +static int powernv_led_probe(struct platform_device *pdev)
> +{
> + struct device_node *led, *cled;
> + int rc = 0;
> +
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (!led) {
> + pr_err("LED parent device node not found\n");
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(led, cled) {
> + if (has_led_type(cled, LED_TYPE_ATTENTION))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +
> + if (has_led_type(cled, LED_TYPE_IDENTIFY))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ID);
> +
> + if (has_led_type(cled, LED_TYPE_FAULT))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_FAULT);
> + }
> + return rc;
> +}
> +
> +/* Platform driver remove */
> +static int powernv_led_remove(struct platform_device *pdev)
> +{
> + struct powernv_led *pled;
> + struct powernv_led_work *pwork;
> + unsigned long flags;
> +
> + /* Disable LED operation */
> + led_disabled = true;
> +
> + pr_info("Unregister all classdev structures\n");
> + list_for_each_entry(pled, &powernv_led_list, link)
> + led_classdev_unregister(&pled->cdev);
> +
> + pr_info("Wait for all work tasks to finish\n");
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> +
> + /* Free nodes from the work list */
> + powernv_led_compact_work_list();
> +
> + /* Free nodes from the classdev list */
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + while (!list_empty(&powernv_led_list)) {
> + pled = list_first_entry(&powernv_led_list,
> + struct powernv_led, link);
> + list_del(&pled->link);
> + kfree(pled);
> + }
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> +
> + /* Check for memory leaks */
> + if (!list_empty(&powernv_led_work_list))
> + pr_warn("Work list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_cmd_list))
> + pr_warn("Request list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_list))
> + pr_warn("Classdev list not empty, memory leak\n");
> +
> + return 0;
> +}
> +
> +/* Platform driver property match */
> +static const struct of_device_id powernv_led_match[] = {
> + {
> + .compatible = "ibm,opal-v3-led",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, powernv_led_match);
> +
> +static struct platform_driver powernv_led_driver = {
> + .probe = powernv_led_probe,
> + .remove = powernv_led_remove,
> + .driver = {
> + .name = "powernv-led-driver",
> + .owner = THIS_MODULE,
> + .of_match_table = powernv_led_match,
Is somewhere DT documentation available for these leds?
> + },
> +};
> +
> +module_platform_driver(powernv_led_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("PowerNV LED driver");
> +MODULE_AUTHOR("Anshuman Khandual <khandual@linux.vnet.ibm.com>");
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-14 15:20 ` Jacek Anaszewski
0 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-14 15:20 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
Hi Vasant,
On 03/20/2015 12:04 PM, Vasant Hegde wrote:
> From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>
> This patch implements LED driver for PowerNV platform using the existing
> generic LED class framework. It registers classdev structures for all
> individual LEDs detected on the system through LED specific device tree
> nodes. Device tree nodes specify what all kind of LEDs present on the
> same location code. It registers LED classdev structure for each of them.
>
> The platform level implementation of LED get and set state has been
> achieved through OPAL calls. These calls are made available for the
> driver by exporting from architecture specific codes.
>
> As per the LED class framework, the 'brightness_set' function should not
> sleep. Hence these functions have been implemented through global work
> queue tasks which might sleep on OPAL async call completion.
Wouldn't it be easier to implement synchronization on the OPAL side?
> All the system LEDs can be found in the same regular path /sys/class/leds/.
> There are two different kind of LEDs present for the same location code,
> one being the identify indicator and other one being the fault indicator.
> We don't use LED colors. Hence our LEDs have names in this format.
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
>
> Any positive brightness value would turn on the LED and a zero value
> would turn off the LED. The driver will return LED_FULL (255) for any
> turned on LED and LED_OFF for any turned off LED.
>
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
> Changes in v2:
> - Added System Attention indicator support
> - Moved common code to powernv_led_set_queue()
>
> -Vasant
>
>
> arch/powerpc/platforms/powernv/opal.c | 12 +
> drivers/leds/Kconfig | 9
> drivers/leds/Makefile | 1
> drivers/leds/leds-powernv.c | 620 +++++++++++++++++++++++++++++++++
> 4 files changed, 641 insertions(+), 1 deletion(-)
> create mode 100644 drivers/leds/leds-powernv.c
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 142a08a..fbfd9c1 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>
> static int __init opal_init(void)
> {
> - struct device_node *np, *consoles;
> + struct device_node *np, *consoles, *led;
> int rc;
>
> opal_node = of_find_node_by_path("/ibm,opal");
> @@ -772,6 +772,13 @@ static int __init opal_init(void)
> /* Create i2c platform devices */
> opal_i2c_create_devs();
>
> + /* Create led platform devices */
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (led) {
> + of_platform_device_create(led, "opal_led", NULL);
> + of_node_put(led);
> + }
> +
> /* Find all OPAL interrupts and request them */
> opal_irq_init(opal_node);
>
> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
> EXPORT_SYMBOL_GPL(opal_tpo_read);
> EXPORT_SYMBOL_GPL(opal_tpo_write);
> EXPORT_SYMBOL_GPL(opal_i2c_request);
> +/* Export these symbols for PowerNV LED class driver */
> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
Please split the above part to the separate patch and put it in the
series before this one.
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 25b320d..a93223c 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -508,6 +508,15 @@ 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
OF dependency is missing here.
> + help
> + This option enables support for the system LEDs present on
> + PowerNV platforms. Say 'y' to enable this support in kernel.
> + Say 'm' enable this support as module.
Please change the last line to:
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 cbba921..604ffc9 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,620 @@
> +/*
> + * PowerNV LED Driver
> + *
> + * Copyright IBM Corp. 2015
> + *
> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> + *
> + * 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.
> + */
> +
> +#define PREFIX "POWERNV_LED"
> +#define pr_fmt(fmt) PREFIX ": " fmt
Wouldn't you mind using dev_* prefixed logging?
Unless you have a good reason not to do it.
> +
> +#include <linux/platform_device.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
Please keep alphabetical order.
> +
> +#include <asm/opal.h>
> +
> +#define LED_STR_ATTENTION ":ATTENTION"
> +#define LED_STR_IDENT ":IDENTIFY"
> +#define LED_STR_FAULT ":FAULT"
Namespacing prefix is required here. LED is reserved for
LED subsystem global macros. How about POWERNV_LED_* ?
> +
> +/*
> + * LED operation state
> + *
> + * led_classdev_unregister resets the brightness values. However
> + * we want to retain the LED state across boot.
What boot are you thinking of here?
> Hence disable
> + * LED operation before calling led_classdev_unregister.
> + */
This comment is unclear for me, as this is static initialization,
unrelated to to any function call.
> +static bool led_disabled = false;
Static variables are initialized to 0 by default.
> +
> +/*
> + * LED type map
> + *
> + * Converts LED event type into it's description.
> + */
> +static const char *led_type_map[OPAL_SLOT_LED_TYPE_MAX] = {
> + "Attention", "Identify", "Fault"
> +};
> +
> +/*
> + * 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. A lot of work queue
> + * tasks attempting to set the LED states can prevent each other from
> + * completing their task. So there is a need to restrict the total number
> + * of work queue tasks at any point in time related to LED.
> + */
> +#define MAX_LED_REQ_COUNT 100
> +static int led_req_count;
> +
> +/*
> + * LED classdev
> + *
> + * Each LED classdev structure registered on the platform will be put into
> + * a list which will be eventually used for de-registration purpose.
> + */
> +struct powernv_led {
> + struct led_classdev cdev;
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_list);
> +static DEFINE_SPINLOCK(powernv_led_spinlock);
> +
> +/*
> + * LED set state command
> + *
> + * Each set LED state request will be saved and put into a list to be
> + * processed later by a work queue task.
> + */
> +struct powernv_led_cmd {
> + struct led_classdev *led_cdev; /* Points to classdev structure */
> + enum led_brightness value; /* Brightness value */
> + u64 led_type; /* Identify or Fault */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_cmd_list);
> +static DEFINE_SPINLOCK(powernv_led_cmd_spinlock);
> +
> +/*
> + * LED set state task
> + *
> + * Each set LED state request added into the request list for post
> + * processing will be followed by one of this structure. The work
> + * struct here will be scheduled on the global work queue to process
> + * one of the requests from the request list. Though the order of
> + * execution is not guaraneteed.
> + */
> +struct powernv_led_work {
> + struct work_struct work; /* Individual task */
> + bool deletion; /* Need deletion */
> + struct list_head link;
> +};
> +static LIST_HEAD(powernv_led_work_list);
> +static DEFINE_SPINLOCK(powernv_led_work_spinlock);
> +
> +/*
> + * powernv_led_compact_work_list
> + *
> + * This function goes through the entire list of scheduled powernv_led_work
> + * nodes and removes the nodes which have already processed one set LED
> + * state request from request list and has been marked for deletion. This is
> + * essential for cleaning the list before adding new elements into it. This
> + * also tracks the total number of pending tasks. Once it reaches the
> + * threshold the function will throttle till all the scheduled tasks completes
> + * execution during which the user space thread will block and will be
> + * prevented from queuing up more LED state change requests.
> + */
Did you test the driver with led-triggers?
> +static void powernv_led_compact_work_list(void)
> +{
> + struct powernv_led_work *pwork, *pwork_ne;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + led_req_count = 0;
> + list_for_each_entry_safe(pwork, pwork_ne,
> + &powernv_led_work_list, link) {
> + if (pwork->deletion) {
> + list_del(&pwork->link);
> + kfree(pwork);
> + continue;
> + }
> + led_req_count++;
> + }
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Throttle if the threshold reached */
> + if (led_req_count == MAX_LED_REQ_COUNT) {
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> + }
> +}
> +
> +/*
> + * powernv_led_set
> + *
> + * 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, u64 led_type)
> +{
> + char *loc_code;
> + int rc, token;
> + u64 led_mask, max_led_type, led_value = 0;
> + struct opal_msg msg;
> +
> + /* Location code of the LED */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err(PREFIX "Memory allocation failed at %s\n", __func__);
> + return;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unknown LED type */
> + goto out_loc;
> + }
Above sequence is repeated in the function below - it could be wrapped
with a new function.
> + /* Prepare for the OPAL call */
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
> + if (value)
> + led_value = OPAL_SLOT_LED_STATE_ON << led_type;
> +
> + /* OPAL async call */
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + pr_err("%s: Couldn't get the token, returning\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) {
> + pr_err("OPAL call opal_leds_set_ind failed for %s with %d\n",
> + loc_code, rc);
> + goto out_token;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc) {
> + pr_err("%s: Failed to wait for the async response, %d\n",
> + __func__, rc);
> + goto out_token;
> + }
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS)
> + pr_err("Async call returned with failed status: %d\n", rc);
> +
> +out_token:
> + opal_async_release_token(token);
> +
> +out_loc:
> + kfree(loc_code);
> +}
> +
> +/*
> + * powernv_led_get
> + *
> + * 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,
> + u64 led_type)
> +{
> + char *loc_code;
> + int rc;
> + u64 led_mask, led_value, max_led_type;
> +
> + /* LED location code */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
> + break;
> + default: /* Unsupported LED type */
> + goto led_fail;
> + }
> +
> + /* 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) {
> + pr_err("OPAL call opal_leds_get_ind failed with %d\n", rc);
> + goto led_fail;
> + }
> +
> + led_mask = be64_to_cpu(led_mask);
> + led_value = be64_to_cpu(led_value);
> +
> + /* LED status available */
> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
> + pr_err("%s LED status not available for %s\n",
> + led_type_map[led_type], loc_code);
> + 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;
> +}
> +
> +/*
> + * powernv_led_work_func
> + *
> + * This the function which will be executed by any LED work task on the
> + * global work queue. This function de-queues one of the request node
> + * from the request list, processes it and then deletes the request node.
> + * This also accesses it's own work list node and sets the deletion flag
> + * in there making itself a candidate for removal the next time the
> + * compact function gets called.
> + */
> +static void powernv_led_work_func(struct work_struct *work)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *req;
> + unsigned long flags;
> +
> + /* De-queue one request, process it and then delete */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + if (list_empty(&powernv_led_cmd_list)) {
> + pr_err("Request list empty, but work queue task queued\n");
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> + return;
> + }
> +
> + req = list_first_entry(&powernv_led_cmd_list,
> + struct powernv_led_cmd, link);
> + list_del(&req->link);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + powernv_led_set(req->led_cdev, req->value, req->led_type);
> + kfree(req);
> +
> + /* Mark the work queue task for deletion */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + pwork = container_of(work, struct powernv_led_work, work);
> + pwork->deletion = true;
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +}
> +
> +/*
> + * powernv_led_set_queue
> + *
> + * LED classdev 'brightness_set' function for LEDs
> + */
> +static void powernv_led_set_queue(struct led_classdev *led_cdev,
> + enum led_brightness value, u64 led_type)
> +{
> + struct powernv_led_work *pwork;
> + struct powernv_led_cmd *attn;
> + unsigned long flags;
> +
> + if (led_disabled)
> + return;
> +
> + /* Allocate the request */
> + attn = kzalloc(sizeof(struct powernv_led_cmd), GFP_KERNEL);
> + if (!attn) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + return;
> + }
> +
> + /* Allocate the work */
> + pwork = kzalloc(sizeof(struct powernv_led_work), GFP_KERNEL);
> + if (!pwork) {
> + pr_err("Memory allocation failed at: %s\n", __func__);
> + kfree(attn);
> + return;
> + }
> +
> + /* Prepare the request */
> + attn->led_cdev = led_cdev;
> + attn->value = value;
> + attn->led_type = led_type;
> +
> + /* Queue the request */
> + spin_lock_irqsave(&powernv_led_cmd_spinlock, flags);
> + list_add_tail(&attn->link, &powernv_led_cmd_list);
> + spin_unlock_irqrestore(&powernv_led_cmd_spinlock, flags);
> +
> + /* Compact the work list */
> + powernv_led_compact_work_list();
> +
> + /* Add the new task into the work list */
> + spin_lock_irqsave(&powernv_led_work_spinlock, flags);
> + list_add_tail(&pwork->link, &powernv_led_work_list);
> + spin_unlock_irqrestore(&powernv_led_work_spinlock, flags);
> +
> + /* Schedule the new task */
> + INIT_WORK(&pwork->work, powernv_led_work_func);
> + schedule_work(&pwork->work);
> +}
> +/*
> + * powernv_led_set_attn
> + *
> + * LED classdev 'brightness_set' function for attention LED.
> + */
> +static void powernv_led_set_attn(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_get_attn
> + *
> + * LED classdev 'brightness_get' function for attention LED.
> + */
> +static enum led_brightness powernv_led_get_attn(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ATTN);
> +}
> +
> +/*
> + * powernv_led_set_ident
> + *
> + * LED classdev 'brightness_set' function for identify LED types.
> + */
> +static void powernv_led_set_ident(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_get_ident
> + *
> + * LED classdev 'brightness_get' function for identify LED types.
> + */
> +static enum led_brightness powernv_led_get_ident(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_ID);
> +}
> +
> +/*
> + * powernv_led_set_fault
> + *
> + * LED classdev 'brightness_set' function for fault LED types.
> + */
> +static void powernv_led_set_fault(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + return powernv_led_set_queue(led_cdev, value,
> + OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * powernv_led_get_fault
> + *
> + * LED classdev 'brightness_get' function for fault LED types.
> + */
> +static enum led_brightness powernv_led_get_fault(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev, OPAL_SLOT_LED_TYPE_FAULT);
> +}
> +
> +/*
> + * has_led_type
> + *
> + * This function verifies whether the child LED device node supports certain
> + * type of LED or not. This will be used to register LEDclassdev structures
> + * for that particual type of LED for a given device tree node.
> + */
> +static bool has_led_type(struct device_node *cled, const char *led_type)
> +{
> + bool result = false;
> +
> + if (of_property_match_string(cled, "led-types", led_type) >= 0)
> + result = true;
> +
> + return result;
> +}
> +
> +/*
> + * power_led_classdev
> + *
> + * This function registers classdev structure for any given type of LED on
> + * a given child LED device node.
> + */
> +static int power_led_classdev(struct platform_device *pdev,
> + struct device_node *cled, u64 led_type)
> +{
> + int rc;
> + unsigned long flags;
> + struct powernv_led *cpled;
> +
> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
> + if (!cpled) {
> + pr_err("Memory allocation failed at %s\n", __func__);
> + return -ENOMEM;
> + }
> +
> + /* Create the name for classdev */
> + switch (led_type) {
> + case OPAL_SLOT_LED_TYPE_ATTN:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_ATTENTION);
We have a 'label' DT property for naming LED Flash class devices.
Please refer to Documentation/devicetree/bindings/leds/common.txt.
> + cpled->cdev.brightness_set = powernv_led_set_attn;
> + cpled->cdev.brightness_get = powernv_led_get_attn;
> + break;
> + case OPAL_SLOT_LED_TYPE_ID:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_IDENT);
> + cpled->cdev.brightness_set = powernv_led_set_ident;
> + cpled->cdev.brightness_get = powernv_led_get_ident;
> + break;
> + case OPAL_SLOT_LED_TYPE_FAULT:
> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
> + cled->name, LED_STR_FAULT);
> + cpled->cdev.brightness_set = powernv_led_set_fault;
> + cpled->cdev.brightness_get = powernv_led_get_fault;
> + break;
> + default: /* Unsupported LED type */
> + return -EINVAL;
> + }
> +
> + if (!cpled->cdev.name) {
> + pr_err("Memory allocation failed for classdev name\n");
> + return -ENOMEM;
> + }
> +
> + cpled->cdev.brightness = LED_OFF;
> + cpled->cdev.max_brightness = LED_FULL;
> + cpled->cdev.flags = LED_CORE_SUSPENDRESUME;
> +
> + /* Register the classdev */
> + rc = led_classdev_register(&pdev->dev, &cpled->cdev);
> + if (rc) {
> + pr_err("Classdev registration failed for %s\n",
> + cpled->cdev.name);
> + } else {
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + list_add_tail(&cpled->link, &powernv_led_list);
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> + pr_debug("Classdev registration successful for %s\n",
> + cpled->cdev.name);
> + }
> + return rc;
> +}
> +
> +/* Platform driver probe */
> +static int powernv_led_probe(struct platform_device *pdev)
> +{
> + struct device_node *led, *cled;
> + int rc = 0;
> +
> + led = of_find_node_by_path("/ibm,opal/led");
> + if (!led) {
> + pr_err("LED parent device node not found\n");
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(led, cled) {
> + if (has_led_type(cled, LED_TYPE_ATTENTION))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ATTN);
> +
> + if (has_led_type(cled, LED_TYPE_IDENTIFY))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_ID);
> +
> + if (has_led_type(cled, LED_TYPE_FAULT))
> + rc = power_led_classdev(pdev, cled,
> + OPAL_SLOT_LED_TYPE_FAULT);
> + }
> + return rc;
> +}
> +
> +/* Platform driver remove */
> +static int powernv_led_remove(struct platform_device *pdev)
> +{
> + struct powernv_led *pled;
> + struct powernv_led_work *pwork;
> + unsigned long flags;
> +
> + /* Disable LED operation */
> + led_disabled = true;
> +
> + pr_info("Unregister all classdev structures\n");
> + list_for_each_entry(pled, &powernv_led_list, link)
> + led_classdev_unregister(&pled->cdev);
> +
> + pr_info("Wait for all work tasks to finish\n");
> + list_for_each_entry(pwork, &powernv_led_work_list, link)
> + flush_work(&pwork->work);
> +
> + /* Free nodes from the work list */
> + powernv_led_compact_work_list();
> +
> + /* Free nodes from the classdev list */
> + spin_lock_irqsave(&powernv_led_spinlock, flags);
> + while (!list_empty(&powernv_led_list)) {
> + pled = list_first_entry(&powernv_led_list,
> + struct powernv_led, link);
> + list_del(&pled->link);
> + kfree(pled);
> + }
> + spin_unlock_irqrestore(&powernv_led_spinlock, flags);
> +
> + /* Check for memory leaks */
> + if (!list_empty(&powernv_led_work_list))
> + pr_warn("Work list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_cmd_list))
> + pr_warn("Request list not empty, memory leak\n");
> +
> + if (!list_empty(&powernv_led_list))
> + pr_warn("Classdev list not empty, memory leak\n");
> +
> + return 0;
> +}
> +
> +/* Platform driver property match */
> +static const struct of_device_id powernv_led_match[] = {
> + {
> + .compatible = "ibm,opal-v3-led",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, powernv_led_match);
> +
> +static struct platform_driver powernv_led_driver = {
> + .probe = powernv_led_probe,
> + .remove = powernv_led_remove,
> + .driver = {
> + .name = "powernv-led-driver",
> + .owner = THIS_MODULE,
> + .of_match_table = powernv_led_match,
Is somewhere DT documentation available for these leds?
> + },
> +};
> +
> +module_platform_driver(powernv_led_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("PowerNV LED driver");
> +MODULE_AUTHOR("Anshuman Khandual <khandual@linux.vnet.ibm.com>");
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-leds" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-14 15:20 ` Jacek Anaszewski
@ 2015-04-15 6:26 ` Vasant Hegde
-1 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-15 6:26 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
> Hi Vasant,
Hi Jacek,
>>
>> This patch implements LED driver for PowerNV platform using the existing
>> generic LED class framework. It registers classdev structures for all
>> individual LEDs detected on the system through LED specific device tree
>> nodes. Device tree nodes specify what all kind of LEDs present on the
>> same location code. It registers LED classdev structure for each of them.
>>
>> The platform level implementation of LED get and set state has been
>> achieved through OPAL calls. These calls are made available for the
>> driver by exporting from architecture specific codes.
>>
>> As per the LED class framework, the 'brightness_set' function should not
>> sleep. Hence these functions have been implemented through global work
>> queue tasks which might sleep on OPAL async call completion.
>
> Wouldn't it be easier to implement synchronization on the OPAL side?
We had thought about this.. But OPAL intern depends on service processor to
enable/disable indicator. So we can't make this as synchronous one.
.../...
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal.c
>> b/arch/powerpc/platforms/powernv/opal.c
>> index 142a08a..fbfd9c1 100644
>> --- a/arch/powerpc/platforms/powernv/opal.c
>> +++ b/arch/powerpc/platforms/powernv/opal.c
>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>>
>> static int __init opal_init(void)
>> {
>> - struct device_node *np, *consoles;
>> + struct device_node *np, *consoles, *led;
>> int rc;
>>
>> opal_node = of_find_node_by_path("/ibm,opal");
>> @@ -772,6 +772,13 @@ static int __init opal_init(void)
>> /* Create i2c platform devices */
>> opal_i2c_create_devs();
>>
>> + /* Create led platform devices */
>> + led = of_find_node_by_path("/ibm,opal/led");
>> + if (led) {
>> + of_platform_device_create(led, "opal_led", NULL);
>> + of_node_put(led);
>> + }
>> +
>> /* Find all OPAL interrupts and request them */
>> opal_irq_init(opal_node);
>>
>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
>> EXPORT_SYMBOL_GPL(opal_tpo_read);
>> EXPORT_SYMBOL_GPL(opal_tpo_write);
>> EXPORT_SYMBOL_GPL(opal_i2c_request);
>> +/* Export these symbols for PowerNV LED class driver */
>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
>
> Please split the above part to the separate patch and put it in the
> series before this one.
Sure. Will split it in next version.
>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 25b320d..a93223c 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -508,6 +508,15 @@ 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
>
> OF dependency is missing here.
Agree. Will fix.
>
>> + help
>> + This option enables support for the system LEDs present on
>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>> + Say 'm' enable this support as module.
>
> Please change the last line to:
>
> To compile this driver as a module, choose M here: the module will
> be called leds-powernv.
>
Sure. Will fix.
>
>> +
>> 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 cbba921..604ffc9 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
>> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
>> --- /dev/null
>> +++ b/drivers/leds/leds-powernv.c
>> @@ -0,0 +1,620 @@
>> +/*
>> + * PowerNV LED Driver
>> + *
>> + * Copyright IBM Corp. 2015
>> + *
>> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> + *
>> + * 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.
>> + */
>> +
>> +#define PREFIX "POWERNV_LED"
>> +#define pr_fmt(fmt) PREFIX ": " fmt
>
> Wouldn't you mind using dev_* prefixed logging?
> Unless you have a good reason not to do it.
>
I don't see any specific reason to use pr_fmt. Will convert this to dev_*.
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>
> Please keep alphabetical order.
Sure.
>
>> +
>> +#include <asm/opal.h>
>> +
>> +#define LED_STR_ATTENTION ":ATTENTION"
>> +#define LED_STR_IDENT ":IDENTIFY"
>> +#define LED_STR_FAULT ":FAULT"
>
> Namespacing prefix is required here. LED is reserved for
> LED subsystem global macros. How about POWERNV_LED_* ?
Yep... Its my fault.. Makes sense to have POWERNV_*.
>
>> +
>> +/*
>> + * LED operation state
>> + *
>> + * led_classdev_unregister resets the brightness values. However
>> + * we want to retain the LED state across boot.
>
> What boot are you thinking of here?
>
>> Hence disable
>> + * LED operation before calling led_classdev_unregister.
>> + */
>
> This comment is unclear for me, as this is static initialization,
> unrelated to to any function call.
>
I mean, we have to retain the state of LED across system reboot.
>> +static bool led_disabled = false;
>
> Static variables are initialized to 0 by default.
Fixed.
.../...
>> +/*
>> + * powernv_led_compact_work_list
>> + *
>> + * This function goes through the entire list of scheduled powernv_led_work
>> + * nodes and removes the nodes which have already processed one set LED
>> + * state request from request list and has been marked for deletion. This is
>> + * essential for cleaning the list before adding new elements into it. This
>> + * also tracks the total number of pending tasks. Once it reaches the
>> + * threshold the function will throttle till all the scheduled tasks completes
>> + * execution during which the user space thread will block and will be
>> + * prevented from queuing up more LED state change requests.
>> + */
>
> Did you test the driver with led-triggers?
I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>> +
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_ID:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_FAULT:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>> + break;
>> + default: /* Unknown LED type */
>> + goto out_loc;
>> + }
>
> Above sequence is repeated in the function below - it could be wrapped
> with a new function.
Agree. Will fix.
.../...
>> +
>> +/*
>> + * power_led_classdev
>> + *
>> + * This function registers classdev structure for any given type of LED on
>> + * a given child LED device node.
>> + */
>> +static int power_led_classdev(struct platform_device *pdev,
>> + struct device_node *cled, u64 led_type)
>> +{
>> + int rc;
>> + unsigned long flags;
>> + struct powernv_led *cpled;
>> +
>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>> + if (!cpled) {
>> + pr_err("Memory allocation failed at %s\n", __func__);
>> + return -ENOMEM;
>> + }
>> +
>> + /* Create the name for classdev */
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>> + cled->name, LED_STR_ATTENTION);
>
> We have a 'label' DT property for naming LED Flash class devices.
> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>
In Power Systems LEDs are overloaded (meaning same LED is used for identify and
fault depending on their state --- blinking = identify and solid = fault).
Hence here append LED type info.
.../...
>> +
>> +static struct platform_driver powernv_led_driver = {
>> + .probe = powernv_led_probe,
>> + .remove = powernv_led_remove,
>> + .driver = {
>> + .name = "powernv-led-driver",
>> + .owner = THIS_MODULE,
>> + .of_match_table = powernv_led_match,
>
> Is somewhere DT documentation available for these leds?
These are PowerNV platform specific properties. I don't think its
specified/documented in kernel side.
These are documented in OPAL (firmware) side.
Thanks for valuable review. Will post v3 ASAP.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-15 6:26 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-15 6:26 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
> Hi Vasant,
Hi Jacek,
>>
>> This patch implements LED driver for PowerNV platform using the existing
>> generic LED class framework. It registers classdev structures for all
>> individual LEDs detected on the system through LED specific device tree
>> nodes. Device tree nodes specify what all kind of LEDs present on the
>> same location code. It registers LED classdev structure for each of them.
>>
>> The platform level implementation of LED get and set state has been
>> achieved through OPAL calls. These calls are made available for the
>> driver by exporting from architecture specific codes.
>>
>> As per the LED class framework, the 'brightness_set' function should not
>> sleep. Hence these functions have been implemented through global work
>> queue tasks which might sleep on OPAL async call completion.
>
> Wouldn't it be easier to implement synchronization on the OPAL side?
We had thought about this.. But OPAL intern depends on service processor to
enable/disable indicator. So we can't make this as synchronous one.
.../...
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal.c
>> b/arch/powerpc/platforms/powernv/opal.c
>> index 142a08a..fbfd9c1 100644
>> --- a/arch/powerpc/platforms/powernv/opal.c
>> +++ b/arch/powerpc/platforms/powernv/opal.c
>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>>
>> static int __init opal_init(void)
>> {
>> - struct device_node *np, *consoles;
>> + struct device_node *np, *consoles, *led;
>> int rc;
>>
>> opal_node = of_find_node_by_path("/ibm,opal");
>> @@ -772,6 +772,13 @@ static int __init opal_init(void)
>> /* Create i2c platform devices */
>> opal_i2c_create_devs();
>>
>> + /* Create led platform devices */
>> + led = of_find_node_by_path("/ibm,opal/led");
>> + if (led) {
>> + of_platform_device_create(led, "opal_led", NULL);
>> + of_node_put(led);
>> + }
>> +
>> /* Find all OPAL interrupts and request them */
>> opal_irq_init(opal_node);
>>
>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
>> EXPORT_SYMBOL_GPL(opal_tpo_read);
>> EXPORT_SYMBOL_GPL(opal_tpo_write);
>> EXPORT_SYMBOL_GPL(opal_i2c_request);
>> +/* Export these symbols for PowerNV LED class driver */
>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
>
> Please split the above part to the separate patch and put it in the
> series before this one.
Sure. Will split it in next version.
>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 25b320d..a93223c 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -508,6 +508,15 @@ 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
>
> OF dependency is missing here.
Agree. Will fix.
>
>> + help
>> + This option enables support for the system LEDs present on
>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>> + Say 'm' enable this support as module.
>
> Please change the last line to:
>
> To compile this driver as a module, choose M here: the module will
> be called leds-powernv.
>
Sure. Will fix.
>
>> +
>> 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 cbba921..604ffc9 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
>> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
>> --- /dev/null
>> +++ b/drivers/leds/leds-powernv.c
>> @@ -0,0 +1,620 @@
>> +/*
>> + * PowerNV LED Driver
>> + *
>> + * Copyright IBM Corp. 2015
>> + *
>> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> + *
>> + * 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.
>> + */
>> +
>> +#define PREFIX "POWERNV_LED"
>> +#define pr_fmt(fmt) PREFIX ": " fmt
>
> Wouldn't you mind using dev_* prefixed logging?
> Unless you have a good reason not to do it.
>
I don't see any specific reason to use pr_fmt. Will convert this to dev_*.
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>
> Please keep alphabetical order.
Sure.
>
>> +
>> +#include <asm/opal.h>
>> +
>> +#define LED_STR_ATTENTION ":ATTENTION"
>> +#define LED_STR_IDENT ":IDENTIFY"
>> +#define LED_STR_FAULT ":FAULT"
>
> Namespacing prefix is required here. LED is reserved for
> LED subsystem global macros. How about POWERNV_LED_* ?
Yep... Its my fault.. Makes sense to have POWERNV_*.
>
>> +
>> +/*
>> + * LED operation state
>> + *
>> + * led_classdev_unregister resets the brightness values. However
>> + * we want to retain the LED state across boot.
>
> What boot are you thinking of here?
>
>> Hence disable
>> + * LED operation before calling led_classdev_unregister.
>> + */
>
> This comment is unclear for me, as this is static initialization,
> unrelated to to any function call.
>
I mean, we have to retain the state of LED across system reboot.
>> +static bool led_disabled = false;
>
> Static variables are initialized to 0 by default.
Fixed.
.../...
>> +/*
>> + * powernv_led_compact_work_list
>> + *
>> + * This function goes through the entire list of scheduled powernv_led_work
>> + * nodes and removes the nodes which have already processed one set LED
>> + * state request from request list and has been marked for deletion. This is
>> + * essential for cleaning the list before adding new elements into it. This
>> + * also tracks the total number of pending tasks. Once it reaches the
>> + * threshold the function will throttle till all the scheduled tasks completes
>> + * execution during which the user space thread will block and will be
>> + * prevented from queuing up more LED state change requests.
>> + */
>
> Did you test the driver with led-triggers?
I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>> +
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_ID:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_FAULT:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>> + break;
>> + default: /* Unknown LED type */
>> + goto out_loc;
>> + }
>
> Above sequence is repeated in the function below - it could be wrapped
> with a new function.
Agree. Will fix.
.../...
>> +
>> +/*
>> + * power_led_classdev
>> + *
>> + * This function registers classdev structure for any given type of LED on
>> + * a given child LED device node.
>> + */
>> +static int power_led_classdev(struct platform_device *pdev,
>> + struct device_node *cled, u64 led_type)
>> +{
>> + int rc;
>> + unsigned long flags;
>> + struct powernv_led *cpled;
>> +
>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>> + if (!cpled) {
>> + pr_err("Memory allocation failed at %s\n", __func__);
>> + return -ENOMEM;
>> + }
>> +
>> + /* Create the name for classdev */
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>> + cled->name, LED_STR_ATTENTION);
>
> We have a 'label' DT property for naming LED Flash class devices.
> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>
In Power Systems LEDs are overloaded (meaning same LED is used for identify and
fault depending on their state --- blinking = identify and solid = fault).
Hence here append LED type info.
.../...
>> +
>> +static struct platform_driver powernv_led_driver = {
>> + .probe = powernv_led_probe,
>> + .remove = powernv_led_remove,
>> + .driver = {
>> + .name = "powernv-led-driver",
>> + .owner = THIS_MODULE,
>> + .of_match_table = powernv_led_match,
>
> Is somewhere DT documentation available for these leds?
These are PowerNV platform specific properties. I don't think its
specified/documented in kernel side.
These are documented in OPAL (firmware) side.
Thanks for valuable review. Will post v3 ASAP.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 6:26 ` Vasant Hegde
(?)
@ 2015-04-15 8:42 ` Jacek Anaszewski
2015-04-15 10:15 ` Vasant Hegde
-1 siblings, 1 reply; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-15 8:42 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
Hi Vasant,
On 04/15/2015 08:26 AM, Vasant Hegde wrote:
> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>
> Hi Jacek,
>
>>>
>>> This patch implements LED driver for PowerNV platform using the existing
>>> generic LED class framework. It registers classdev structures for all
>>> individual LEDs detected on the system through LED specific device tree
>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>> same location code. It registers LED classdev structure for each of them.
>>>
>>> The platform level implementation of LED get and set state has been
>>> achieved through OPAL calls. These calls are made available for the
>>> driver by exporting from architecture specific codes.
>>>
>>> As per the LED class framework, the 'brightness_set' function should not
>>> sleep. Hence these functions have been implemented through global work
>>> queue tasks which might sleep on OPAL async call completion.
>>
>> Wouldn't it be easier to implement synchronization on the OPAL side?
>
> We had thought about this.. But OPAL intern depends on service processor to
> enable/disable indicator. So we can't make this as synchronous one.
I've revised this one more time and I think that use of spin locks is an
overkill in this case. It would suffice to define three work queues -
one per led and one common mutex.
powernv_led_set and powernv_led_get would have to be called under the
mutex. Please refer to the existing LED class drivers of the LED
controllers with many sub-LEDs to control: e.g.:
drivers/leds/leds-lm355x.c
>
> .../...
>
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/opal.c
>>> b/arch/powerpc/platforms/powernv/opal.c
>>> index 142a08a..fbfd9c1 100644
>>> --- a/arch/powerpc/platforms/powernv/opal.c
>>> +++ b/arch/powerpc/platforms/powernv/opal.c
>>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>>>
>>> static int __init opal_init(void)
>>> {
>>> - struct device_node *np, *consoles;
>>> + struct device_node *np, *consoles, *led;
>>> int rc;
>>>
>>> opal_node = of_find_node_by_path("/ibm,opal");
>>> @@ -772,6 +772,13 @@ static int __init opal_init(void)
>>> /* Create i2c platform devices */
>>> opal_i2c_create_devs();
>>>
>>> + /* Create led platform devices */
>>> + led = of_find_node_by_path("/ibm,opal/led");
>>> + if (led) {
>>> + of_platform_device_create(led, "opal_led", NULL);
>>> + of_node_put(led);
>>> + }
>>> +
>>> /* Find all OPAL interrupts and request them */
>>> opal_irq_init(opal_node);
>>>
>>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
>>> EXPORT_SYMBOL_GPL(opal_tpo_read);
>>> EXPORT_SYMBOL_GPL(opal_tpo_write);
>>> EXPORT_SYMBOL_GPL(opal_i2c_request);
>>> +/* Export these symbols for PowerNV LED class driver */
>>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
>>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
>>
>> Please split the above part to the separate patch and put it in the
>> series before this one.
>
> Sure. Will split it in next version.
>
>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 25b320d..a93223c 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -508,6 +508,15 @@ 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
>>
>> OF dependency is missing here.
>
> Agree. Will fix.
>
>>
>>> + help
>>> + This option enables support for the system LEDs present on
>>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>>> + Say 'm' enable this support as module.
>>
>> Please change the last line to:
>>
>> To compile this driver as a module, choose M here: the module will
>> be called leds-powernv.
>>
>
> Sure. Will fix.
>
>>
>>> +
>>> 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 cbba921..604ffc9 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
>>> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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..0c9f958
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-powernv.c
>>> @@ -0,0 +1,620 @@
>>> +/*
>>> + * PowerNV LED Driver
>>> + *
>>> + * Copyright IBM Corp. 2015
>>> + *
>>> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#define PREFIX "POWERNV_LED"
>>> +#define pr_fmt(fmt) PREFIX ": " fmt
>>
>> Wouldn't you mind using dev_* prefixed logging?
>> Unless you have a good reason not to do it.
>>
>
> I don't see any specific reason to use pr_fmt. Will convert this to dev_*.
>
>
>>> +
>>> +#include <linux/platform_device.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/list.h>
>>
>> Please keep alphabetical order.
>
> Sure.
>
>>
>>> +
>>> +#include <asm/opal.h>
>>> +
>>> +#define LED_STR_ATTENTION ":ATTENTION"
>>> +#define LED_STR_IDENT ":IDENTIFY"
>>> +#define LED_STR_FAULT ":FAULT"
>>
>> Namespacing prefix is required here. LED is reserved for
>> LED subsystem global macros. How about POWERNV_LED_* ?
>
> Yep... Its my fault.. Makes sense to have POWERNV_*.
You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.
>>
>>> +
>>> +/*
>>> + * LED operation state
>>> + *
>>> + * led_classdev_unregister resets the brightness values. However
>>> + * we want to retain the LED state across boot.
>>
>> What boot are you thinking of here?
>>
>>> Hence disable
>>> + * LED operation before calling led_classdev_unregister.
>>> + */
>>
>> This comment is unclear for me, as this is static initialization,
>> unrelated to to any function call.
>>
>
> I mean, we have to retain the state of LED across system reboot.
Static variables are reinitialized on system reboot, aren't they?
>>> +static bool led_disabled = false;
>>
>> Static variables are initialized to 0 by default.
>
> Fixed.
>
> .../...
>
>>> +/*
>>> + * powernv_led_compact_work_list
>>> + *
>>> + * This function goes through the entire list of scheduled powernv_led_work
>>> + * nodes and removes the nodes which have already processed one set LED
>>> + * state request from request list and has been marked for deletion. This is
>>> + * essential for cleaning the list before adding new elements into it. This
>>> + * also tracks the total number of pending tasks. Once it reaches the
>>> + * threshold the function will throttle till all the scheduled tasks completes
>>> + * execution during which the user space thread will block and will be
>>> + * prevented from queuing up more LED state change requests.
>>> + */
>>
>> Did you test the driver with led-triggers?
>
> I have tested set/reset LEDs .. not triggers as our user space dont' use that.
At this moment it doesn't, but it can use them in the future. Besides,
I think that you wouldn't like to see someone testing your driver with
led-triggers and having it crashed? :)
Testing LED subsystem drivers with led-triggers is useful also because
some triggers (e.g. timer) call brightness_set op from the interrupt
context. It allows to detect some non-obvious issues.
>>> +
>>> + switch (led_type) {
>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>> + break;
>>> + case OPAL_SLOT_LED_TYPE_ID:
>>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>> + break;
>>> + case OPAL_SLOT_LED_TYPE_FAULT:
>>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>> + break;
>>> + default: /* Unknown LED type */
>>> + goto out_loc;
>>> + }
>>
>> Above sequence is repeated in the function below - it could be wrapped
>> with a new function.
>
> Agree. Will fix.
>
> .../...
>
>>> +
>>> +/*
>>> + * power_led_classdev
>>> + *
>>> + * This function registers classdev structure for any given type of LED on
>>> + * a given child LED device node.
>>> + */
>>> +static int power_led_classdev(struct platform_device *pdev,
>>> + struct device_node *cled, u64 led_type)
>>> +{
>>> + int rc;
>>> + unsigned long flags;
>>> + struct powernv_led *cpled;
>>> +
>>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>> + if (!cpled) {
>>> + pr_err("Memory allocation failed at %s\n", __func__);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + /* Create the name for classdev */
>>> + switch (led_type) {
>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>> + cled->name, LED_STR_ATTENTION);
>>
>> We have a 'label' DT property for naming LED Flash class devices.
>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>
>
> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
> fault depending on their state --- blinking = identify and solid = fault).
> Hence here append LED type info.
The label could be composed of segments and an ordinal number as
labels have to be unique, e.g. attn_ident_0, attn_ident_1.
The segments would have to be parsed by the driver to discover
all the LED's available modes.
nitpicking: identify is a verb and is not a proper name for the LED.
Could you describe the purpose of this mode, so that we could come
up with a better name?
>
> .../...
>
>>> +
>>> +static struct platform_driver powernv_led_driver = {
>>> + .probe = powernv_led_probe,
>>> + .remove = powernv_led_remove,
>>> + .driver = {
>>> + .name = "powernv-led-driver",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = powernv_led_match,
>>
>> Is somewhere DT documentation available for these leds?
>
> These are PowerNV platform specific properties. I don't think its
> specified/documented in kernel side.
> These are documented in OPAL (firmware) side.
Every driver using DT bindings needs DT documentation.
Moreover the one exists for powerenv platform:
Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
Bindings for these LEDs should be added there probably.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 8:42 ` Jacek Anaszewski
@ 2015-04-15 10:15 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-15 10:15 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
> Hi Vasant,
>
> On 04/15/2015 08:26 AM, Vasant Hegde wrote:
>> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
>>
>> Hi Jacek,
>>
>>>>
>>>> This patch implements LED driver for PowerNV platform using the existing
>>>> generic LED class framework. It registers classdev structures for all
>>>> individual LEDs detected on the system through LED specific device tree
>>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>>> same location code. It registers LED classdev structure for each of them.
>>>>
>>>> The platform level implementation of LED get and set state has been
>>>> achieved through OPAL calls. These calls are made available for the
>>>> driver by exporting from architecture specific codes.
>>>>
>>>> As per the LED class framework, the 'brightness_set' function should not
>>>> sleep. Hence these functions have been implemented through global work
>>>> queue tasks which might sleep on OPAL async call completion.
>>>
>>> Wouldn't it be easier to implement synchronization on the OPAL side?
>>
>> We had thought about this.. But OPAL intern depends on service processor to
>> enable/disable indicator. So we can't make this as synchronous one.
>
> I've revised this one more time and I think that use of spin locks is an
> overkill in this case. It would suffice to define three work queues -
> one per led and one common mutex.
Will try work queue option/
>
> powernv_led_set and powernv_led_get would have to be called under the
> mutex. Please refer to the existing LED class drivers of the LED
> controllers with many sub-LEDs to control: e.g.:
> drivers/leds/leds-lm355x.c
Ok. Will look into it. Thanks!
.../...
>>
>>>
>>>> +
>>>> +#include <asm/opal.h>
>>>> +
>>>> +#define LED_STR_ATTENTION ":ATTENTION"
>>>> +#define LED_STR_IDENT ":IDENTIFY"
>>>> +#define LED_STR_FAULT ":FAULT"
>>>
>>> Namespacing prefix is required here. LED is reserved for
>>> LED subsystem global macros. How about POWERNV_LED_* ?
>>
>> Yep... Its my fault.. Makes sense to have POWERNV_*.
>
> You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.
Agree. Fixed.
>
>>>
>>>> +
>>>> +/*
>>>> + * LED operation state
>>>> + *
>>>> + * led_classdev_unregister resets the brightness values. However
>>>> + * we want to retain the LED state across boot.
>>>
>>> What boot are you thinking of here?
>>>
>>>> Hence disable
>>>> + * LED operation before calling led_classdev_unregister.
>>>> + */
>>>
>>> This comment is unclear for me, as this is static initialization,
>>> unrelated to to any function call.
>>>
>>
>> I mean, we have to retain the state of LED across system reboot.
>
> Static variables are reinitialized on system reboot, aren't they?
Sorry. I think comment was confusing..
As I understood, classdev_unregister call resets all LEDs state. However in our
case, we don't
want to change the LED state during system shutdown/reboot.
Hence I have introduced state variable here. So during register call, I just
disable LEDs so that any further call will just return. That way we retain LED
state even after unloading module.
Please let me know if there is any better way to achieve this.
>
>>>> +static bool led_disabled = false;
>>>
>>> Static variables are initialized to 0 by default.
>>
>> Fixed.
>>
>> .../...
>>
>>>> +/*
>>>> + * powernv_led_compact_work_list
>>>> + *
>>>> + * This function goes through the entire list of scheduled powernv_led_work
>>>> + * nodes and removes the nodes which have already processed one set LED
>>>> + * state request from request list and has been marked for deletion. This is
>>>> + * essential for cleaning the list before adding new elements into it. This
>>>> + * also tracks the total number of pending tasks. Once it reaches the
>>>> + * threshold the function will throttle till all the scheduled tasks completes
>>>> + * execution during which the user space thread will block and will be
>>>> + * prevented from queuing up more LED state change requests.
>>>> + */
>>>
>>> Did you test the driver with led-triggers?
>>
>> I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>
> At this moment it doesn't, but it can use them in the future. Besides,
> I think that you wouldn't like to see someone testing your driver with
> led-triggers and having it crashed? :)
Yep. I will test this before posting next version.
>
> Testing LED subsystem drivers with led-triggers is useful also because
> some triggers (e.g. timer) call brightness_set op from the interrupt
> context. It allows to detect some non-obvious issues.
>
>>>> +
>>>> + switch (led_type) {
>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>>> + break;
>>>> + case OPAL_SLOT_LED_TYPE_ID:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>>> + break;
>>>> + case OPAL_SLOT_LED_TYPE_FAULT:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>>> + break;
>>>> + default: /* Unknown LED type */
>>>> + goto out_loc;
>>>> + }
>>>
>>> Above sequence is repeated in the function below - it could be wrapped
>>> with a new function.
>>
>> Agree. Will fix.
>>
>> .../...
>>
>>>> +
>>>> +/*
>>>> + * power_led_classdev
>>>> + *
>>>> + * This function registers classdev structure for any given type of LED on
>>>> + * a given child LED device node.
>>>> + */
>>>> +static int power_led_classdev(struct platform_device *pdev,
>>>> + struct device_node *cled, u64 led_type)
>>>> +{
>>>> + int rc;
>>>> + unsigned long flags;
>>>> + struct powernv_led *cpled;
>>>> +
>>>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>>> + if (!cpled) {
>>>> + pr_err("Memory allocation failed at %s\n", __func__);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + /* Create the name for classdev */
>>>> + switch (led_type) {
>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>>> + cled->name, LED_STR_ATTENTION);
>>>
>>> We have a 'label' DT property for naming LED Flash class devices.
>>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>>
>>
>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>> fault depending on their state --- blinking = identify and solid = fault).
>> Hence here append LED type info.
>
> The label could be composed of segments and an ordinal number as
> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
> The segments would have to be parsed by the driver to discover
> all the LED's available modes.
>
> nitpicking: identify is a verb and is not a proper name for the LED.
> Could you describe the purpose of this mode, so that we could come
> up with a better name?
Each component (Field Replacement Unit) will have service indicator (LEDS) which
can have below states :
- OFF : no action
- Identify: blinking state (user can use this state to identify particular
component).
In Power Systems world we call it as "identify" indicator.. Hence I
retained same name here.
How about just "ident" ?
- fault : solid state (when component goes bad, LED goes to solid state)
Note that our FW is capable of isolating some of the issues and it can turn
on LEDs without OS
interference.
We have one more System level LED (System Attention Indicator).. This LED has
two states:
- OFF : Everything is fine
- ON : Some component has issues and needs attention.
>
>
>>
>> .../...
>>
>>>> +
>>>> +static struct platform_driver powernv_led_driver = {
>>>> + .probe = powernv_led_probe,
>>>> + .remove = powernv_led_remove,
>>>> + .driver = {
>>>> + .name = "powernv-led-driver",
>>>> + .owner = THIS_MODULE,
>>>> + .of_match_table = powernv_led_match,
>>>
>>> Is somewhere DT documentation available for these leds?
>>
>> These are PowerNV platform specific properties. I don't think its
>> specified/documented in kernel side.
>> These are documented in OPAL (firmware) side.
>
> Every driver using DT bindings needs DT documentation.
> Moreover the one exists for powerenv platform:
>
> Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
Right. I missed to add binding for LEDs.
I will add Documentation/devicetree/bindings/leds/leds-powernv.txt.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-15 10:15 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-15 10:15 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
> Hi Vasant,
>
> On 04/15/2015 08:26 AM, Vasant Hegde wrote:
>> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
>>
>> Hi Jacek,
>>
>>>>
>>>> This patch implements LED driver for PowerNV platform using the existing
>>>> generic LED class framework. It registers classdev structures for all
>>>> individual LEDs detected on the system through LED specific device tree
>>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>>> same location code. It registers LED classdev structure for each of them.
>>>>
>>>> The platform level implementation of LED get and set state has been
>>>> achieved through OPAL calls. These calls are made available for the
>>>> driver by exporting from architecture specific codes.
>>>>
>>>> As per the LED class framework, the 'brightness_set' function should not
>>>> sleep. Hence these functions have been implemented through global work
>>>> queue tasks which might sleep on OPAL async call completion.
>>>
>>> Wouldn't it be easier to implement synchronization on the OPAL side?
>>
>> We had thought about this.. But OPAL intern depends on service processor to
>> enable/disable indicator. So we can't make this as synchronous one.
>
> I've revised this one more time and I think that use of spin locks is an
> overkill in this case. It would suffice to define three work queues -
> one per led and one common mutex.
Will try work queue option/
>
> powernv_led_set and powernv_led_get would have to be called under the
> mutex. Please refer to the existing LED class drivers of the LED
> controllers with many sub-LEDs to control: e.g.:
> drivers/leds/leds-lm355x.c
Ok. Will look into it. Thanks!
.../...
>>
>>>
>>>> +
>>>> +#include <asm/opal.h>
>>>> +
>>>> +#define LED_STR_ATTENTION ":ATTENTION"
>>>> +#define LED_STR_IDENT ":IDENTIFY"
>>>> +#define LED_STR_FAULT ":FAULT"
>>>
>>> Namespacing prefix is required here. LED is reserved for
>>> LED subsystem global macros. How about POWERNV_LED_* ?
>>
>> Yep... Its my fault.. Makes sense to have POWERNV_*.
>
> You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.
Agree. Fixed.
>
>>>
>>>> +
>>>> +/*
>>>> + * LED operation state
>>>> + *
>>>> + * led_classdev_unregister resets the brightness values. However
>>>> + * we want to retain the LED state across boot.
>>>
>>> What boot are you thinking of here?
>>>
>>>> Hence disable
>>>> + * LED operation before calling led_classdev_unregister.
>>>> + */
>>>
>>> This comment is unclear for me, as this is static initialization,
>>> unrelated to to any function call.
>>>
>>
>> I mean, we have to retain the state of LED across system reboot.
>
> Static variables are reinitialized on system reboot, aren't they?
Sorry. I think comment was confusing..
As I understood, classdev_unregister call resets all LEDs state. However in our
case, we don't
want to change the LED state during system shutdown/reboot.
Hence I have introduced state variable here. So during register call, I just
disable LEDs so that any further call will just return. That way we retain LED
state even after unloading module.
Please let me know if there is any better way to achieve this.
>
>>>> +static bool led_disabled = false;
>>>
>>> Static variables are initialized to 0 by default.
>>
>> Fixed.
>>
>> .../...
>>
>>>> +/*
>>>> + * powernv_led_compact_work_list
>>>> + *
>>>> + * This function goes through the entire list of scheduled powernv_led_work
>>>> + * nodes and removes the nodes which have already processed one set LED
>>>> + * state request from request list and has been marked for deletion. This is
>>>> + * essential for cleaning the list before adding new elements into it. This
>>>> + * also tracks the total number of pending tasks. Once it reaches the
>>>> + * threshold the function will throttle till all the scheduled tasks completes
>>>> + * execution during which the user space thread will block and will be
>>>> + * prevented from queuing up more LED state change requests.
>>>> + */
>>>
>>> Did you test the driver with led-triggers?
>>
>> I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>
> At this moment it doesn't, but it can use them in the future. Besides,
> I think that you wouldn't like to see someone testing your driver with
> led-triggers and having it crashed? :)
Yep. I will test this before posting next version.
>
> Testing LED subsystem drivers with led-triggers is useful also because
> some triggers (e.g. timer) call brightness_set op from the interrupt
> context. It allows to detect some non-obvious issues.
>
>>>> +
>>>> + switch (led_type) {
>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>>> + break;
>>>> + case OPAL_SLOT_LED_TYPE_ID:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>>> + break;
>>>> + case OPAL_SLOT_LED_TYPE_FAULT:
>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>>> + break;
>>>> + default: /* Unknown LED type */
>>>> + goto out_loc;
>>>> + }
>>>
>>> Above sequence is repeated in the function below - it could be wrapped
>>> with a new function.
>>
>> Agree. Will fix.
>>
>> .../...
>>
>>>> +
>>>> +/*
>>>> + * power_led_classdev
>>>> + *
>>>> + * This function registers classdev structure for any given type of LED on
>>>> + * a given child LED device node.
>>>> + */
>>>> +static int power_led_classdev(struct platform_device *pdev,
>>>> + struct device_node *cled, u64 led_type)
>>>> +{
>>>> + int rc;
>>>> + unsigned long flags;
>>>> + struct powernv_led *cpled;
>>>> +
>>>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>>> + if (!cpled) {
>>>> + pr_err("Memory allocation failed at %s\n", __func__);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + /* Create the name for classdev */
>>>> + switch (led_type) {
>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>>> + cled->name, LED_STR_ATTENTION);
>>>
>>> We have a 'label' DT property for naming LED Flash class devices.
>>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>>
>>
>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>> fault depending on their state --- blinking = identify and solid = fault).
>> Hence here append LED type info.
>
> The label could be composed of segments and an ordinal number as
> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
> The segments would have to be parsed by the driver to discover
> all the LED's available modes.
>
> nitpicking: identify is a verb and is not a proper name for the LED.
> Could you describe the purpose of this mode, so that we could come
> up with a better name?
Each component (Field Replacement Unit) will have service indicator (LEDS) which
can have below states :
- OFF : no action
- Identify: blinking state (user can use this state to identify particular
component).
In Power Systems world we call it as "identify" indicator.. Hence I
retained same name here.
How about just "ident" ?
- fault : solid state (when component goes bad, LED goes to solid state)
Note that our FW is capable of isolating some of the issues and it can turn
on LEDs without OS
interference.
We have one more System level LED (System Attention Indicator).. This LED has
two states:
- OFF : Everything is fine
- ON : Some component has issues and needs attention.
>
>
>>
>> .../...
>>
>>>> +
>>>> +static struct platform_driver powernv_led_driver = {
>>>> + .probe = powernv_led_probe,
>>>> + .remove = powernv_led_remove,
>>>> + .driver = {
>>>> + .name = "powernv-led-driver",
>>>> + .owner = THIS_MODULE,
>>>> + .of_match_table = powernv_led_match,
>>>
>>> Is somewhere DT documentation available for these leds?
>>
>> These are PowerNV platform specific properties. I don't think its
>> specified/documented in kernel side.
>> These are documented in OPAL (firmware) side.
>
> Every driver using DT bindings needs DT documentation.
> Moreover the one exists for powerenv platform:
>
> Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
Right. I missed to add binding for LEDs.
I will add Documentation/devicetree/bindings/leds/leds-powernv.txt.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 10:15 ` Vasant Hegde
(?)
@ 2015-04-15 13:12 ` Jacek Anaszewski
2015-04-16 6:47 ` Jacek Anaszewski
2015-04-16 6:52 ` Vasant Hegde
-1 siblings, 2 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-15 13:12 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
On 04/15/2015 12:15 PM, Vasant Hegde wrote:
> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/15/2015 08:26 AM, Vasant Hegde wrote:
>>> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>>>>
>>>>> This patch implements LED driver for PowerNV platform using the existing
>>>>> generic LED class framework. It registers classdev structures for all
>>>>> individual LEDs detected on the system through LED specific device tree
>>>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>>>> same location code. It registers LED classdev structure for each of them.
>>>>>
>>>>> The platform level implementation of LED get and set state has been
>>>>> achieved through OPAL calls. These calls are made available for the
>>>>> driver by exporting from architecture specific codes.
>>>>>
>>>>> As per the LED class framework, the 'brightness_set' function should not
>>>>> sleep. Hence these functions have been implemented through global work
>>>>> queue tasks which might sleep on OPAL async call completion.
>>>>
>>>> Wouldn't it be easier to implement synchronization on the OPAL side?
>>>
>>> We had thought about this.. But OPAL intern depends on service processor to
>>> enable/disable indicator. So we can't make this as synchronous one.
>>
>> I've revised this one more time and I think that use of spin locks is an
>> overkill in this case. It would suffice to define three work queues -
>> one per led and one common mutex.
>
> Will try work queue option/
>
>>
>> powernv_led_set and powernv_led_get would have to be called under the
>> mutex. Please refer to the existing LED class drivers of the LED
>> controllers with many sub-LEDs to control: e.g.:
>> drivers/leds/leds-lm355x.c
>
> Ok. Will look into it. Thanks!
>
> .../...
>
>>>
>>>>
>>>>> +
>>>>> +#include <asm/opal.h>
>>>>> +
>>>>> +#define LED_STR_ATTENTION ":ATTENTION"
>>>>> +#define LED_STR_IDENT ":IDENTIFY"
>>>>> +#define LED_STR_FAULT ":FAULT"
>>>>
>>>> Namespacing prefix is required here. LED is reserved for
>>>> LED subsystem global macros. How about POWERNV_LED_* ?
>>>
>>> Yep... Its my fault.. Makes sense to have POWERNV_*.
>>
>> You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.
>
> Agree. Fixed.
>
>>
>>>>
>>>>> +
>>>>> +/*
>>>>> + * LED operation state
>>>>> + *
>>>>> + * led_classdev_unregister resets the brightness values. However
>>>>> + * we want to retain the LED state across boot.
>>>>
>>>> What boot are you thinking of here?
>>>>
>>>>> Hence disable
>>>>> + * LED operation before calling led_classdev_unregister.
>>>>> + */
>>>>
>>>> This comment is unclear for me, as this is static initialization,
>>>> unrelated to to any function call.
>>>>
>>>
>>> I mean, we have to retain the state of LED across system reboot.
>>
>> Static variables are reinitialized on system reboot, aren't they?
>
> Sorry. I think comment was confusing..
>
> As I understood, classdev_unregister call resets all LEDs state. However in our
> case, we don't
> want to change the LED state during system shutdown/reboot.
>
> Hence I have introduced state variable here. So during register call, I just
> disable LEDs so that any further call will just return. That way we retain LED
> state even after unloading module.
>
> Please let me know if there is any better way to achieve this.
Since this is not a feature of the device, but rather a use case, then
it cannot be hard coded in the driver this way. The solution I see is
introducing a sysfs attribute that would determine if we want the
LED to be turned off on unregistration or not.
You can refer to the following driver to find out how to add the
sysfs attribute:
drivers/leds/leds-lm3533.c
The attribute will also have to be documented, similarly to these:
Documentation/ABI/testing/sysfs-class-led-driver-lm3533
Currently I don't have a good candidate for attribute
name, so feel free to propose one.
>
>>
>>>>> +static bool led_disabled = false;
>>>>
>>>> Static variables are initialized to 0 by default.
>>>
>>> Fixed.
>>>
>>> .../...
>>>
>>>>> +/*
>>>>> + * powernv_led_compact_work_list
>>>>> + *
>>>>> + * This function goes through the entire list of scheduled powernv_led_work
>>>>> + * nodes and removes the nodes which have already processed one set LED
>>>>> + * state request from request list and has been marked for deletion. This is
>>>>> + * essential for cleaning the list before adding new elements into it. This
>>>>> + * also tracks the total number of pending tasks. Once it reaches the
>>>>> + * threshold the function will throttle till all the scheduled tasks completes
>>>>> + * execution during which the user space thread will block and will be
>>>>> + * prevented from queuing up more LED state change requests.
>>>>> + */
>>>>
>>>> Did you test the driver with led-triggers?
>>>
>>> I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>>
>> At this moment it doesn't, but it can use them in the future. Besides,
>> I think that you wouldn't like to see someone testing your driver with
>> led-triggers and having it crashed? :)
>
> Yep. I will test this before posting next version.
>
>>
>> Testing LED subsystem drivers with led-triggers is useful also because
>> some triggers (e.g. timer) call brightness_set op from the interrupt
>> context. It allows to detect some non-obvious issues.
>>
>>>>> +
>>>>> + switch (led_type) {
>>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>>>> + break;
>>>>> + case OPAL_SLOT_LED_TYPE_ID:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>>>> + break;
>>>>> + case OPAL_SLOT_LED_TYPE_FAULT:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>>>> + break;
>>>>> + default: /* Unknown LED type */
>>>>> + goto out_loc;
>>>>> + }
>>>>
>>>> Above sequence is repeated in the function below - it could be wrapped
>>>> with a new function.
>>>
>>> Agree. Will fix.
>>>
>>> .../...
>>>
>>>>> +
>>>>> +/*
>>>>> + * power_led_classdev
>>>>> + *
>>>>> + * This function registers classdev structure for any given type of LED on
>>>>> + * a given child LED device node.
>>>>> + */
>>>>> +static int power_led_classdev(struct platform_device *pdev,
>>>>> + struct device_node *cled, u64 led_type)
>>>>> +{
>>>>> + int rc;
>>>>> + unsigned long flags;
>>>>> + struct powernv_led *cpled;
>>>>> +
>>>>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>>>> + if (!cpled) {
>>>>> + pr_err("Memory allocation failed at %s\n", __func__);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> +
>>>>> + /* Create the name for classdev */
>>>>> + switch (led_type) {
>>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>>>> + cled->name, LED_STR_ATTENTION);
>>>>
>>>> We have a 'label' DT property for naming LED Flash class devices.
>>>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>>>
>>>
>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>> fault depending on their state --- blinking = identify and solid = fault).
>>> Hence here append LED type info.
>>
>> The label could be composed of segments and an ordinal number as
>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>> The segments would have to be parsed by the driver to discover
>> all the LED's available modes.
>>
>> nitpicking: identify is a verb and is not a proper name for the LED.
>> Could you describe the purpose of this mode, so that we could come
>> up with a better name?
>
> Each component (Field Replacement Unit) will have service indicator (LEDS) which
> can have below states :
> - OFF : no action
> - Identify: blinking state (user can use this state to identify particular
> component).
> In Power Systems world we call it as "identify" indicator.. Hence I
> retained same name here.
> How about just "ident" ?
Sounds good.
> - fault : solid state (when component goes bad, LED goes to solid state)
> Note that our FW is capable of isolating some of the issues and it can turn
> on LEDs without OS
> interference.
Does it mean that the LED can be controlled from hardware?
If so, what would be software use cases then? The same question is
related to the attn and indent states.
> We have one more System level LED (System Attention Indicator).. This LED has
> two states:
> - OFF : Everything is fine
> - ON : Some component has issues and needs attention.
>
>>
>>
>>>
>>> .../...
>>>
>>>>> +
>>>>> +static struct platform_driver powernv_led_driver = {
>>>>> + .probe = powernv_led_probe,
>>>>> + .remove = powernv_led_remove,
>>>>> + .driver = {
>>>>> + .name = "powernv-led-driver",
>>>>> + .owner = THIS_MODULE,
>>>>> + .of_match_table = powernv_led_match,
>>>>
>>>> Is somewhere DT documentation available for these leds?
>>>
>>> These are PowerNV platform specific properties. I don't think its
>>> specified/documented in kernel side.
>>> These are documented in OPAL (firmware) side.
>>
>> Every driver using DT bindings needs DT documentation.
>> Moreover the one exists for powerenv platform:
>>
>> Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
>
> Right. I missed to add binding for LEDs.
>
> I will add Documentation/devicetree/bindings/leds/leds-powernv.txt.
Thanks.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 13:12 ` Jacek Anaszewski
@ 2015-04-16 6:47 ` Jacek Anaszewski
2015-04-16 6:52 ` Vasant Hegde
1 sibling, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-16 6:47 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, Linux LED Subsystem, stewart, mpe, Bryan Wu,
Richard Purdie, khandual
Resending the message as previously it failed to reach the linuxppc-dev
list.
-------- Original Message --------
Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Date: Wed, 15 Apr 2015 15:12:50 +0200
From: Jacek Anaszewski <j.anaszewski@samsung.com>
To: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
CC: linuxppc-dev@lists.ozlabs.org, linux-leds@vger.kernel.org,
stewart@linux.vnet.ibm.com, mpe@ellerman.id.au, cooloney@gmail.com,
rpurdie@rpsys.net, khandual@linux.vnet.ibm.com
On 04/15/2015 12:15 PM, Vasant Hegde wrote:
> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/15/2015 08:26 AM, Vasant Hegde wrote:
>>> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>>>>
>>>>> This patch implements LED driver for PowerNV platform using the existing
>>>>> generic LED class framework. It registers classdev structures for all
>>>>> individual LEDs detected on the system through LED specific device tree
>>>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>>>> same location code. It registers LED classdev structure for each of them.
>>>>>
>>>>> The platform level implementation of LED get and set state has been
>>>>> achieved through OPAL calls. These calls are made available for the
>>>>> driver by exporting from architecture specific codes.
>>>>>
>>>>> As per the LED class framework, the 'brightness_set' function should not
>>>>> sleep. Hence these functions have been implemented through global work
>>>>> queue tasks which might sleep on OPAL async call completion.
>>>>
>>>> Wouldn't it be easier to implement synchronization on the OPAL side?
>>>
>>> We had thought about this.. But OPAL intern depends on service processor to
>>> enable/disable indicator. So we can't make this as synchronous one.
>>
>> I've revised this one more time and I think that use of spin locks is an
>> overkill in this case. It would suffice to define three work queues -
>> one per led and one common mutex.
>
> Will try work queue option/
>
>>
>> powernv_led_set and powernv_led_get would have to be called under the
>> mutex. Please refer to the existing LED class drivers of the LED
>> controllers with many sub-LEDs to control: e.g.:
>> drivers/leds/leds-lm355x.c
>
> Ok. Will look into it. Thanks!
>
> .../...
>
>>>
>>>>
>>>>> +
>>>>> +#include <asm/opal.h>
>>>>> +
>>>>> +#define LED_STR_ATTENTION ":ATTENTION"
>>>>> +#define LED_STR_IDENT ":IDENTIFY"
>>>>> +#define LED_STR_FAULT ":FAULT"
>>>>
>>>> Namespacing prefix is required here. LED is reserved for
>>>> LED subsystem global macros. How about POWERNV_LED_* ?
>>>
>>> Yep... Its my fault.. Makes sense to have POWERNV_*.
>>
>> You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.
>
> Agree. Fixed.
>
>>
>>>>
>>>>> +
>>>>> +/*
>>>>> + * LED operation state
>>>>> + *
>>>>> + * led_classdev_unregister resets the brightness values. However
>>>>> + * we want to retain the LED state across boot.
>>>>
>>>> What boot are you thinking of here?
>>>>
>>>>> Hence disable
>>>>> + * LED operation before calling led_classdev_unregister.
>>>>> + */
>>>>
>>>> This comment is unclear for me, as this is static initialization,
>>>> unrelated to to any function call.
>>>>
>>>
>>> I mean, we have to retain the state of LED across system reboot.
>>
>> Static variables are reinitialized on system reboot, aren't they?
>
> Sorry. I think comment was confusing..
>
> As I understood, classdev_unregister call resets all LEDs state. However in our
> case, we don't
> want to change the LED state during system shutdown/reboot.
>
> Hence I have introduced state variable here. So during register call, I just
> disable LEDs so that any further call will just return. That way we retain LED
> state even after unloading module.
>
> Please let me know if there is any better way to achieve this.
Since this is not a feature of the device, but rather a use case, then
it cannot be hard coded in the driver this way. The solution I see is
introducing a sysfs attribute that would determine if we want the
LED to be turned off on unregistration or not.
You can refer to the following driver to find out how to add the
sysfs attribute:
drivers/leds/leds-lm3533.c
The attribute will also have to be documented, similarly to these:
Documentation/ABI/testing/sysfs-class-led-driver-lm3533
Currently I don't have a good candidate for attribute
name, so feel free to propose one.
>
>>
>>>>> +static bool led_disabled = false;
>>>>
>>>> Static variables are initialized to 0 by default.
>>>
>>> Fixed.
>>>
>>> .../...
>>>
>>>>> +/*
>>>>> + * powernv_led_compact_work_list
>>>>> + *
>>>>> + * This function goes through the entire list of scheduled powernv_led_work
>>>>> + * nodes and removes the nodes which have already processed one set LED
>>>>> + * state request from request list and has been marked for deletion. This is
>>>>> + * essential for cleaning the list before adding new elements into it. This
>>>>> + * also tracks the total number of pending tasks. Once it reaches the
>>>>> + * threshold the function will throttle till all the scheduled tasks completes
>>>>> + * execution during which the user space thread will block and will be
>>>>> + * prevented from queuing up more LED state change requests.
>>>>> + */
>>>>
>>>> Did you test the driver with led-triggers?
>>>
>>> I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>>
>> At this moment it doesn't, but it can use them in the future. Besides,
>> I think that you wouldn't like to see someone testing your driver with
>> led-triggers and having it crashed? :)
>
> Yep. I will test this before posting next version.
>
>>
>> Testing LED subsystem drivers with led-triggers is useful also because
>> some triggers (e.g. timer) call brightness_set op from the interrupt
>> context. It allows to detect some non-obvious issues.
>>
>>>>> +
>>>>> + switch (led_type) {
>>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>>>> + break;
>>>>> + case OPAL_SLOT_LED_TYPE_ID:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>>>> + break;
>>>>> + case OPAL_SLOT_LED_TYPE_FAULT:
>>>>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>>>> + break;
>>>>> + default: /* Unknown LED type */
>>>>> + goto out_loc;
>>>>> + }
>>>>
>>>> Above sequence is repeated in the function below - it could be wrapped
>>>> with a new function.
>>>
>>> Agree. Will fix.
>>>
>>> .../...
>>>
>>>>> +
>>>>> +/*
>>>>> + * power_led_classdev
>>>>> + *
>>>>> + * This function registers classdev structure for any given type of LED on
>>>>> + * a given child LED device node.
>>>>> + */
>>>>> +static int power_led_classdev(struct platform_device *pdev,
>>>>> + struct device_node *cled, u64 led_type)
>>>>> +{
>>>>> + int rc;
>>>>> + unsigned long flags;
>>>>> + struct powernv_led *cpled;
>>>>> +
>>>>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>>>> + if (!cpled) {
>>>>> + pr_err("Memory allocation failed at %s\n", __func__);
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> +
>>>>> + /* Create the name for classdev */
>>>>> + switch (led_type) {
>>>>> + case OPAL_SLOT_LED_TYPE_ATTN:
>>>>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>>>> + cled->name, LED_STR_ATTENTION);
>>>>
>>>> We have a 'label' DT property for naming LED Flash class devices.
>>>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>>>
>>>
>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>> fault depending on their state --- blinking = identify and solid = fault).
>>> Hence here append LED type info.
>>
>> The label could be composed of segments and an ordinal number as
>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>> The segments would have to be parsed by the driver to discover
>> all the LED's available modes.
>>
>> nitpicking: identify is a verb and is not a proper name for the LED.
>> Could you describe the purpose of this mode, so that we could come
>> up with a better name?
>
> Each component (Field Replacement Unit) will have service indicator (LEDS) which
> can have below states :
> - OFF : no action
> - Identify: blinking state (user can use this state to identify particular
> component).
> In Power Systems world we call it as "identify" indicator.. Hence I
> retained same name here.
> How about just "ident" ?
Sounds good.
> - fault : solid state (when component goes bad, LED goes to solid state)
> Note that our FW is capable of isolating some of the issues and it can turn
> on LEDs without OS
> interference.
Does it mean that the LED can be controlled from hardware?
If so, what would be software use cases then? The same question is
related to the attn and indent states.
> We have one more System level LED (System Attention Indicator).. This LED has
> two states:
> - OFF : Everything is fine
> - ON : Some component has issues and needs attention.
>
>>
>>
>>>
>>> .../...
>>>
>>>>> +
>>>>> +static struct platform_driver powernv_led_driver = {
>>>>> + .probe = powernv_led_probe,
>>>>> + .remove = powernv_led_remove,
>>>>> + .driver = {
>>>>> + .name = "powernv-led-driver",
>>>>> + .owner = THIS_MODULE,
>>>>> + .of_match_table = powernv_led_match,
>>>>
>>>> Is somewhere DT documentation available for these leds?
>>>
>>> These are PowerNV platform specific properties. I don't think its
>>> specified/documented in kernel side.
>>> These are documented in OPAL (firmware) side.
>>
>> Every driver using DT bindings needs DT documentation.
>> Moreover the one exists for powerenv platform:
>>
>> Documentation/devicetree/bindings/hwmon/ibmpowernv.txt
>
> Right. I missed to add binding for LEDs.
>
> I will add Documentation/devicetree/bindings/leds/leds-powernv.txt.
Thanks.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 13:12 ` Jacek Anaszewski
@ 2015-04-16 6:52 ` Vasant Hegde
2015-04-16 6:52 ` Vasant Hegde
1 sibling, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 6:52 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
Hi Jacek,
.../...
>>>>>
>>>>
>>>> I mean, we have to retain the state of LED across system reboot.
>>>
>>> Static variables are reinitialized on system reboot, aren't they?
>>
>> Sorry. I think comment was confusing..
>>
>> As I understood, classdev_unregister call resets all LEDs state. However in our
>> case, we don't
>> want to change the LED state during system shutdown/reboot.
>>
>> Hence I have introduced state variable here. So during register call, I just
>> disable LEDs so that any further call will just return. That way we retain LED
>> state even after unloading module.
>>
>> Please let me know if there is any better way to achieve this.
>
> Since this is not a feature of the device, but rather a use case, then
> it cannot be hard coded in the driver this way. The solution I see is
> introducing a sysfs attribute that would determine if we want the
> LED to be turned off on unregistration or not.
Hmmm. I thought having simple various is enough ..
I don't know the usage of other LED drivers .. Just a thought .. How about
enabling this feature in LED class itself ...Something like:
- Add new attribute to generic LED class (say persistent)?
- If drivers wants to retain LED state across reboot, then enable this option
- During unregister call check for this attribute
>
> You can refer to the following driver to find out how to add the
> sysfs attribute:
>
> drivers/leds/leds-lm3533.c
>
> The attribute will also have to be documented, similarly to these:
>
> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>
> Currently I don't have a good candidate for attribute
> name, so feel free to propose one.
If you don't like generic attribute, then shall I introduce "persistent"
attribute inside my driver. ?
- By default this attribute is ON and if user wants he can disable this .
- And I will have another variable (say op_support).. which I will disable in
unload path..
.../...
>>>
>>> The label could be composed of segments and an ordinal number as
>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>> The segments would have to be parsed by the driver to discover
>>> all the LED's available modes.
>>>
>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>> Could you describe the purpose of this mode, so that we could come
>>> up with a better name?
>>
>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>> can have below states :
>> - OFF : no action
>> - Identify: blinking state (user can use this state to identify particular
>> component).
>> In Power Systems world we call it as "identify" indicator.. Hence I
>> retained same name here.
>> How about just "ident" ?
>
> Sounds good.
>
>> - fault : solid state (when component goes bad, LED goes to solid state)
>> Note that our FW is capable of isolating some of the issues and it can turn
>> on LEDs without OS
>> interference.
>
> Does it mean that the LED can be controlled from hardware?
> If so, what would be software use cases then? The same question is
> related to the attn and indent states.
- Identify LEDs:
We have service processor interface to set/reset this LEDs.. Similar operation
can be done from inband interface as well (via user space tools in Linux)..
Later management layer can make use of this.
- Fault / Attention :
FW can SET these LEDs if its capable of isolating problem.
Similarly host/user space can SET these LEDs if then can do fine grained
problem isolation etc.
Host/user space can RESET these LEDs.
Hence we are adding host support to all the LEDs which can be modified by host.
Hope this clarifies.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-16 6:52 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 6:52 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
Hi Jacek,
.../...
>>>>>
>>>>
>>>> I mean, we have to retain the state of LED across system reboot.
>>>
>>> Static variables are reinitialized on system reboot, aren't they?
>>
>> Sorry. I think comment was confusing..
>>
>> As I understood, classdev_unregister call resets all LEDs state. However in our
>> case, we don't
>> want to change the LED state during system shutdown/reboot.
>>
>> Hence I have introduced state variable here. So during register call, I just
>> disable LEDs so that any further call will just return. That way we retain LED
>> state even after unloading module.
>>
>> Please let me know if there is any better way to achieve this.
>
> Since this is not a feature of the device, but rather a use case, then
> it cannot be hard coded in the driver this way. The solution I see is
> introducing a sysfs attribute that would determine if we want the
> LED to be turned off on unregistration or not.
Hmmm. I thought having simple various is enough ..
I don't know the usage of other LED drivers .. Just a thought .. How about
enabling this feature in LED class itself ...Something like:
- Add new attribute to generic LED class (say persistent)?
- If drivers wants to retain LED state across reboot, then enable this option
- During unregister call check for this attribute
>
> You can refer to the following driver to find out how to add the
> sysfs attribute:
>
> drivers/leds/leds-lm3533.c
>
> The attribute will also have to be documented, similarly to these:
>
> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>
> Currently I don't have a good candidate for attribute
> name, so feel free to propose one.
If you don't like generic attribute, then shall I introduce "persistent"
attribute inside my driver. ?
- By default this attribute is ON and if user wants he can disable this .
- And I will have another variable (say op_support).. which I will disable in
unload path..
.../...
>>>
>>> The label could be composed of segments and an ordinal number as
>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>> The segments would have to be parsed by the driver to discover
>>> all the LED's available modes.
>>>
>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>> Could you describe the purpose of this mode, so that we could come
>>> up with a better name?
>>
>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>> can have below states :
>> - OFF : no action
>> - Identify: blinking state (user can use this state to identify particular
>> component).
>> In Power Systems world we call it as "identify" indicator.. Hence I
>> retained same name here.
>> How about just "ident" ?
>
> Sounds good.
>
>> - fault : solid state (when component goes bad, LED goes to solid state)
>> Note that our FW is capable of isolating some of the issues and it can turn
>> on LEDs without OS
>> interference.
>
> Does it mean that the LED can be controlled from hardware?
> If so, what would be software use cases then? The same question is
> related to the attn and indent states.
- Identify LEDs:
We have service processor interface to set/reset this LEDs.. Similar operation
can be done from inband interface as well (via user space tools in Linux)..
Later management layer can make use of this.
- Fault / Attention :
FW can SET these LEDs if its capable of isolating problem.
Similarly host/user space can SET these LEDs if then can do fine grained
problem isolation etc.
Host/user space can RESET these LEDs.
Hence we are adding host support to all the LEDs which can be modified by host.
Hope this clarifies.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-16 6:52 ` Vasant Hegde
@ 2015-04-16 8:51 ` Jacek Anaszewski
-1 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-16 8:51 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
Hi Vasant,
On 04/16/2015 08:52 AM, Vasant Hegde wrote:
> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>> Hi Vasant,
>
> Hi Jacek,
>
> .../...
>
>>>>>>
>>>>>
>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>
>>>> Static variables are reinitialized on system reboot, aren't they?
>>>
>>> Sorry. I think comment was confusing..
>>>
>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>> case, we don't
>>> want to change the LED state during system shutdown/reboot.
>>>
>>> Hence I have introduced state variable here. So during register call, I just
>>> disable LEDs so that any further call will just return. That way we retain LED
>>> state even after unloading module.
>>>
>>> Please let me know if there is any better way to achieve this.
>>
>> Since this is not a feature of the device, but rather a use case, then
>> it cannot be hard coded in the driver this way. The solution I see is
>> introducing a sysfs attribute that would determine if we want the
>> LED to be turned off on unregistration or not.
>
> Hmmm. I thought having simple various is enough ..
>
> I don't know the usage of other LED drivers .. Just a thought .. How about
> enabling this feature in LED class itself ...Something like:
> - Add new attribute to generic LED class (say persistent)?
> - If drivers wants to retain LED state across reboot, then enable this option
> - During unregister call check for this attribute
>
>>
>> You can refer to the following driver to find out how to add the
>> sysfs attribute:
>>
>> drivers/leds/leds-lm3533.c
>>
>> The attribute will also have to be documented, similarly to these:
>>
>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>
>> Currently I don't have a good candidate for attribute
>> name, so feel free to propose one.
>
> If you don't like generic attribute, then shall I introduce "persistent"
> attribute inside my driver. ?
> - By default this attribute is ON and if user wants he can disable this .
> - And I will have another variable (say op_support).. which I will disable in
> unload path..
> .../...
>
>>>>
>>>> The label could be composed of segments and an ordinal number as
>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>> The segments would have to be parsed by the driver to discover
>>>> all the LED's available modes.
>>>>
>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>> Could you describe the purpose of this mode, so that we could come
>>>> up with a better name?
>>>
>>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>>> can have below states :
>>> - OFF : no action
>>> - Identify: blinking state (user can use this state to identify particular
>>> component).
>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>> retained same name here.
>>> How about just "ident" ?
>>
>> Sounds good.
>>
>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>> Note that our FW is capable of isolating some of the issues and it can turn
>>> on LEDs without OS
>>> interference.
>>
>> Does it mean that the LED can be controlled from hardware?
>> If so, what would be software use cases then? The same question is
>> related to the attn and indent states.
>
> - Identify LEDs:
> We have service processor interface to set/reset this LEDs.. Similar operation
> can be done from inband interface as well (via user space tools in Linux)..
> Later management layer can make use of this.
>
> - Fault / Attention :
> FW can SET these LEDs if its capable of isolating problem.
> Similarly host/user space can SET these LEDs if then can do fine grained
> problem isolation etc.
> Host/user space can RESET these LEDs.
>
> Hence we are adding host support to all the LEDs which can be modified by host.
>
> Hope this clarifies.
Thanks for this explanation.
I am changing my mind about these LEDs. Since they can be controlled
from hardware without any system interaction, then turning them off
on driver removal makes no sense. The LEDs could be turned on again even
if the driver is unloaded.
Since the driver doesn't perform any initialization of the LEDs,
neither should it turn them off on removal.
If I understand this correctly, then the solution with variable would
do and no sysfs attribute would be required.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-16 8:51 ` Jacek Anaszewski
0 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-16 8:51 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
Hi Vasant,
On 04/16/2015 08:52 AM, Vasant Hegde wrote:
> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>> Hi Vasant,
>
> Hi Jacek,
>
> .../...
>
>>>>>>
>>>>>
>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>
>>>> Static variables are reinitialized on system reboot, aren't they?
>>>
>>> Sorry. I think comment was confusing..
>>>
>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>> case, we don't
>>> want to change the LED state during system shutdown/reboot.
>>>
>>> Hence I have introduced state variable here. So during register call, I just
>>> disable LEDs so that any further call will just return. That way we retain LED
>>> state even after unloading module.
>>>
>>> Please let me know if there is any better way to achieve this.
>>
>> Since this is not a feature of the device, but rather a use case, then
>> it cannot be hard coded in the driver this way. The solution I see is
>> introducing a sysfs attribute that would determine if we want the
>> LED to be turned off on unregistration or not.
>
> Hmmm. I thought having simple various is enough ..
>
> I don't know the usage of other LED drivers .. Just a thought .. How about
> enabling this feature in LED class itself ...Something like:
> - Add new attribute to generic LED class (say persistent)?
> - If drivers wants to retain LED state across reboot, then enable this option
> - During unregister call check for this attribute
>
>>
>> You can refer to the following driver to find out how to add the
>> sysfs attribute:
>>
>> drivers/leds/leds-lm3533.c
>>
>> The attribute will also have to be documented, similarly to these:
>>
>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>
>> Currently I don't have a good candidate for attribute
>> name, so feel free to propose one.
>
> If you don't like generic attribute, then shall I introduce "persistent"
> attribute inside my driver. ?
> - By default this attribute is ON and if user wants he can disable this .
> - And I will have another variable (say op_support).. which I will disable in
> unload path..
> .../...
>
>>>>
>>>> The label could be composed of segments and an ordinal number as
>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>> The segments would have to be parsed by the driver to discover
>>>> all the LED's available modes.
>>>>
>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>> Could you describe the purpose of this mode, so that we could come
>>>> up with a better name?
>>>
>>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>>> can have below states :
>>> - OFF : no action
>>> - Identify: blinking state (user can use this state to identify particular
>>> component).
>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>> retained same name here.
>>> How about just "ident" ?
>>
>> Sounds good.
>>
>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>> Note that our FW is capable of isolating some of the issues and it can turn
>>> on LEDs without OS
>>> interference.
>>
>> Does it mean that the LED can be controlled from hardware?
>> If so, what would be software use cases then? The same question is
>> related to the attn and indent states.
>
> - Identify LEDs:
> We have service processor interface to set/reset this LEDs.. Similar operation
> can be done from inband interface as well (via user space tools in Linux)..
> Later management layer can make use of this.
>
> - Fault / Attention :
> FW can SET these LEDs if its capable of isolating problem.
> Similarly host/user space can SET these LEDs if then can do fine grained
> problem isolation etc.
> Host/user space can RESET these LEDs.
>
> Hence we are adding host support to all the LEDs which can be modified by host.
>
> Hope this clarifies.
Thanks for this explanation.
I am changing my mind about these LEDs. Since they can be controlled
from hardware without any system interaction, then turning them off
on driver removal makes no sense. The LEDs could be turned on again even
if the driver is unloaded.
Since the driver doesn't perform any initialization of the LEDs,
neither should it turn them off on removal.
If I understand this correctly, then the solution with variable would
do and no sysfs attribute would be required.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-16 8:51 ` Jacek Anaszewski
@ 2015-04-16 10:26 ` Vasant Hegde
-1 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 10:26 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
> Hi Vasant,
>
> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>> Hi Vasant,
>>
>> Hi Jacek,
>>
>> .../...
>>
>>>>>>>
>>>>>>
>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>
>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>
>>>> Sorry. I think comment was confusing..
>>>>
>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>> case, we don't
>>>> want to change the LED state during system shutdown/reboot.
>>>>
>>>> Hence I have introduced state variable here. So during register call, I just
>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>> state even after unloading module.
>>>>
>>>> Please let me know if there is any better way to achieve this.
>>>
>>> Since this is not a feature of the device, but rather a use case, then
>>> it cannot be hard coded in the driver this way. The solution I see is
>>> introducing a sysfs attribute that would determine if we want the
>>> LED to be turned off on unregistration or not.
>>
>> Hmmm. I thought having simple various is enough ..
>>
>> I don't know the usage of other LED drivers .. Just a thought .. How about
>> enabling this feature in LED class itself ...Something like:
>> - Add new attribute to generic LED class (say persistent)?
>> - If drivers wants to retain LED state across reboot, then enable this option
>> - During unregister call check for this attribute
>>
>>>
>>> You can refer to the following driver to find out how to add the
>>> sysfs attribute:
>>>
>>> drivers/leds/leds-lm3533.c
>>>
>>> The attribute will also have to be documented, similarly to these:
>>>
>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>
>>> Currently I don't have a good candidate for attribute
>>> name, so feel free to propose one.
>>
>> If you don't like generic attribute, then shall I introduce "persistent"
>> attribute inside my driver. ?
>> - By default this attribute is ON and if user wants he can disable this .
>> - And I will have another variable (say op_support).. which I will disable in
>> unload path..
>> .../...
>>
>>>>>
>>>>> The label could be composed of segments and an ordinal number as
>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>> The segments would have to be parsed by the driver to discover
>>>>> all the LED's available modes.
>>>>>
>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>> Could you describe the purpose of this mode, so that we could come
>>>>> up with a better name?
>>>>
>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>> which
>>>> can have below states :
>>>> - OFF : no action
>>>> - Identify: blinking state (user can use this state to identify particular
>>>> component).
>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>> retained same name here.
>>>> How about just "ident" ?
>>>
>>> Sounds good.
>>>
>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>> Note that our FW is capable of isolating some of the issues and it
>>>> can turn
>>>> on LEDs without OS
>>>> interference.
>>>
>>> Does it mean that the LED can be controlled from hardware?
>>> If so, what would be software use cases then? The same question is
>>> related to the attn and indent states.
>>
>> - Identify LEDs:
>> We have service processor interface to set/reset this LEDs.. Similar operation
>> can be done from inband interface as well (via user space tools in Linux)..
>> Later management layer can make use of this.
>>
>> - Fault / Attention :
>> FW can SET these LEDs if its capable of isolating problem.
>> Similarly host/user space can SET these LEDs if then can do fine grained
>> problem isolation etc.
>> Host/user space can RESET these LEDs.
>>
>> Hence we are adding host support to all the LEDs which can be modified by host.
>>
>> Hope this clarifies.
>
> Thanks for this explanation.
>
> I am changing my mind about these LEDs. Since they can be controlled
> from hardware without any system interaction, then turning them off
> on driver removal makes no sense. The LEDs could be turned on again even
> if the driver is unloaded.
> Since the driver doesn't perform any initialization of the LEDs,
> neither should it turn them off on removal.
>
> If I understand this correctly, then the solution with variable would
> do and no sysfs attribute would be required.
>
Jacek,
Thanks. Then I will retain current method.
One question..What is the preferred way to name LED node in this case ?
<location_code>:<ATTENTION|IDENTIFY|FAULT>
OR
<location_code>
ident <- attribute under each node
fault
attention
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-16 10:26 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 10:26 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
> Hi Vasant,
>
> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>> Hi Vasant,
>>
>> Hi Jacek,
>>
>> .../...
>>
>>>>>>>
>>>>>>
>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>
>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>
>>>> Sorry. I think comment was confusing..
>>>>
>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>> case, we don't
>>>> want to change the LED state during system shutdown/reboot.
>>>>
>>>> Hence I have introduced state variable here. So during register call, I just
>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>> state even after unloading module.
>>>>
>>>> Please let me know if there is any better way to achieve this.
>>>
>>> Since this is not a feature of the device, but rather a use case, then
>>> it cannot be hard coded in the driver this way. The solution I see is
>>> introducing a sysfs attribute that would determine if we want the
>>> LED to be turned off on unregistration or not.
>>
>> Hmmm. I thought having simple various is enough ..
>>
>> I don't know the usage of other LED drivers .. Just a thought .. How about
>> enabling this feature in LED class itself ...Something like:
>> - Add new attribute to generic LED class (say persistent)?
>> - If drivers wants to retain LED state across reboot, then enable this option
>> - During unregister call check for this attribute
>>
>>>
>>> You can refer to the following driver to find out how to add the
>>> sysfs attribute:
>>>
>>> drivers/leds/leds-lm3533.c
>>>
>>> The attribute will also have to be documented, similarly to these:
>>>
>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>
>>> Currently I don't have a good candidate for attribute
>>> name, so feel free to propose one.
>>
>> If you don't like generic attribute, then shall I introduce "persistent"
>> attribute inside my driver. ?
>> - By default this attribute is ON and if user wants he can disable this .
>> - And I will have another variable (say op_support).. which I will disable in
>> unload path..
>> .../...
>>
>>>>>
>>>>> The label could be composed of segments and an ordinal number as
>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>> The segments would have to be parsed by the driver to discover
>>>>> all the LED's available modes.
>>>>>
>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>> Could you describe the purpose of this mode, so that we could come
>>>>> up with a better name?
>>>>
>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>> which
>>>> can have below states :
>>>> - OFF : no action
>>>> - Identify: blinking state (user can use this state to identify particular
>>>> component).
>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>> retained same name here.
>>>> How about just "ident" ?
>>>
>>> Sounds good.
>>>
>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>> Note that our FW is capable of isolating some of the issues and it
>>>> can turn
>>>> on LEDs without OS
>>>> interference.
>>>
>>> Does it mean that the LED can be controlled from hardware?
>>> If so, what would be software use cases then? The same question is
>>> related to the attn and indent states.
>>
>> - Identify LEDs:
>> We have service processor interface to set/reset this LEDs.. Similar operation
>> can be done from inband interface as well (via user space tools in Linux)..
>> Later management layer can make use of this.
>>
>> - Fault / Attention :
>> FW can SET these LEDs if its capable of isolating problem.
>> Similarly host/user space can SET these LEDs if then can do fine grained
>> problem isolation etc.
>> Host/user space can RESET these LEDs.
>>
>> Hence we are adding host support to all the LEDs which can be modified by host.
>>
>> Hope this clarifies.
>
> Thanks for this explanation.
>
> I am changing my mind about these LEDs. Since they can be controlled
> from hardware without any system interaction, then turning them off
> on driver removal makes no sense. The LEDs could be turned on again even
> if the driver is unloaded.
> Since the driver doesn't perform any initialization of the LEDs,
> neither should it turn them off on removal.
>
> If I understand this correctly, then the solution with variable would
> do and no sysfs attribute would be required.
>
Jacek,
Thanks. Then I will retain current method.
One question..What is the preferred way to name LED node in this case ?
<location_code>:<ATTENTION|IDENTIFY|FAULT>
OR
<location_code>
ident <- attribute under each node
fault
attention
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-16 10:26 ` Vasant Hegde
@ 2015-04-16 11:34 ` Jacek Anaszewski
-1 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-16 11:34 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
> On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>> .../...
>>>
>>>>>>>>
>>>>>>>
>>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>>
>>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>>
>>>>> Sorry. I think comment was confusing..
>>>>>
>>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>>> case, we don't
>>>>> want to change the LED state during system shutdown/reboot.
>>>>>
>>>>> Hence I have introduced state variable here. So during register call, I just
>>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>>> state even after unloading module.
>>>>>
>>>>> Please let me know if there is any better way to achieve this.
>>>>
>>>> Since this is not a feature of the device, but rather a use case, then
>>>> it cannot be hard coded in the driver this way. The solution I see is
>>>> introducing a sysfs attribute that would determine if we want the
>>>> LED to be turned off on unregistration or not.
>>>
>>> Hmmm. I thought having simple various is enough ..
>>>
>>> I don't know the usage of other LED drivers .. Just a thought .. How about
>>> enabling this feature in LED class itself ...Something like:
>>> - Add new attribute to generic LED class (say persistent)?
>>> - If drivers wants to retain LED state across reboot, then enable this option
>>> - During unregister call check for this attribute
>>>
>>>>
>>>> You can refer to the following driver to find out how to add the
>>>> sysfs attribute:
>>>>
>>>> drivers/leds/leds-lm3533.c
>>>>
>>>> The attribute will also have to be documented, similarly to these:
>>>>
>>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>>
>>>> Currently I don't have a good candidate for attribute
>>>> name, so feel free to propose one.
>>>
>>> If you don't like generic attribute, then shall I introduce "persistent"
>>> attribute inside my driver. ?
>>> - By default this attribute is ON and if user wants he can disable this .
>>> - And I will have another variable (say op_support).. which I will disable in
>>> unload path..
>>> .../...
>>>
>>>>>>
>>>>>> The label could be composed of segments and an ordinal number as
>>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>>> The segments would have to be parsed by the driver to discover
>>>>>> all the LED's available modes.
>>>>>>
>>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>>> Could you describe the purpose of this mode, so that we could come
>>>>>> up with a better name?
>>>>>
>>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>>> which
>>>>> can have below states :
>>>>> - OFF : no action
>>>>> - Identify: blinking state (user can use this state to identify particular
>>>>> component).
>>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>>> retained same name here.
>>>>> How about just "ident" ?
>>>>
>>>> Sounds good.
>>>>
>>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>>> Note that our FW is capable of isolating some of the issues and it
>>>>> can turn
>>>>> on LEDs without OS
>>>>> interference.
>>>>
>>>> Does it mean that the LED can be controlled from hardware?
>>>> If so, what would be software use cases then? The same question is
>>>> related to the attn and indent states.
>>>
>>> - Identify LEDs:
>>> We have service processor interface to set/reset this LEDs.. Similar operation
>>> can be done from inband interface as well (via user space tools in Linux)..
>>> Later management layer can make use of this.
>>>
>>> - Fault / Attention :
>>> FW can SET these LEDs if its capable of isolating problem.
>>> Similarly host/user space can SET these LEDs if then can do fine grained
>>> problem isolation etc.
>>> Host/user space can RESET these LEDs.
>>>
>>> Hence we are adding host support to all the LEDs which can be modified by host.
>>>
>>> Hope this clarifies.
>>
>> Thanks for this explanation.
>>
>> I am changing my mind about these LEDs. Since they can be controlled
>> from hardware without any system interaction, then turning them off
>> on driver removal makes no sense. The LEDs could be turned on again even
>> if the driver is unloaded.
>> Since the driver doesn't perform any initialization of the LEDs,
>> neither should it turn them off on removal.
>>
>> If I understand this correctly, then the solution with variable would
>> do and no sysfs attribute would be required.
>>
>
> Jacek,
>
> Thanks. Then I will retain current method.
>
> One question..What is the preferred way to name LED node in this case ?
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
> OR
> <location_code>
> ident <- attribute under each node
> fault
> attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
label = "powernv1:encl::ident:fault"
}
led2 {
label = "powernv2:desc:attn::fault:"
}
led2 {
label = "powernv3:desc:::fault:"
}
--
Best Regards,
Jacek Anaszewski
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-16 11:34 ` Jacek Anaszewski
0 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-16 11:34 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
> On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>> .../...
>>>
>>>>>>>>
>>>>>>>
>>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>>
>>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>>
>>>>> Sorry. I think comment was confusing..
>>>>>
>>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>>> case, we don't
>>>>> want to change the LED state during system shutdown/reboot.
>>>>>
>>>>> Hence I have introduced state variable here. So during register call, I just
>>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>>> state even after unloading module.
>>>>>
>>>>> Please let me know if there is any better way to achieve this.
>>>>
>>>> Since this is not a feature of the device, but rather a use case, then
>>>> it cannot be hard coded in the driver this way. The solution I see is
>>>> introducing a sysfs attribute that would determine if we want the
>>>> LED to be turned off on unregistration or not.
>>>
>>> Hmmm. I thought having simple various is enough ..
>>>
>>> I don't know the usage of other LED drivers .. Just a thought .. How about
>>> enabling this feature in LED class itself ...Something like:
>>> - Add new attribute to generic LED class (say persistent)?
>>> - If drivers wants to retain LED state across reboot, then enable this option
>>> - During unregister call check for this attribute
>>>
>>>>
>>>> You can refer to the following driver to find out how to add the
>>>> sysfs attribute:
>>>>
>>>> drivers/leds/leds-lm3533.c
>>>>
>>>> The attribute will also have to be documented, similarly to these:
>>>>
>>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>>
>>>> Currently I don't have a good candidate for attribute
>>>> name, so feel free to propose one.
>>>
>>> If you don't like generic attribute, then shall I introduce "persistent"
>>> attribute inside my driver. ?
>>> - By default this attribute is ON and if user wants he can disable this .
>>> - And I will have another variable (say op_support).. which I will disable in
>>> unload path..
>>> .../...
>>>
>>>>>>
>>>>>> The label could be composed of segments and an ordinal number as
>>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>>> The segments would have to be parsed by the driver to discover
>>>>>> all the LED's available modes.
>>>>>>
>>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>>> Could you describe the purpose of this mode, so that we could come
>>>>>> up with a better name?
>>>>>
>>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>>> which
>>>>> can have below states :
>>>>> - OFF : no action
>>>>> - Identify: blinking state (user can use this state to identify particular
>>>>> component).
>>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>>> retained same name here.
>>>>> How about just "ident" ?
>>>>
>>>> Sounds good.
>>>>
>>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>>> Note that our FW is capable of isolating some of the issues and it
>>>>> can turn
>>>>> on LEDs without OS
>>>>> interference.
>>>>
>>>> Does it mean that the LED can be controlled from hardware?
>>>> If so, what would be software use cases then? The same question is
>>>> related to the attn and indent states.
>>>
>>> - Identify LEDs:
>>> We have service processor interface to set/reset this LEDs.. Similar operation
>>> can be done from inband interface as well (via user space tools in Linux)..
>>> Later management layer can make use of this.
>>>
>>> - Fault / Attention :
>>> FW can SET these LEDs if its capable of isolating problem.
>>> Similarly host/user space can SET these LEDs if then can do fine grained
>>> problem isolation etc.
>>> Host/user space can RESET these LEDs.
>>>
>>> Hence we are adding host support to all the LEDs which can be modified by host.
>>>
>>> Hope this clarifies.
>>
>> Thanks for this explanation.
>>
>> I am changing my mind about these LEDs. Since they can be controlled
>> from hardware without any system interaction, then turning them off
>> on driver removal makes no sense. The LEDs could be turned on again even
>> if the driver is unloaded.
>> Since the driver doesn't perform any initialization of the LEDs,
>> neither should it turn them off on removal.
>>
>> If I understand this correctly, then the solution with variable would
>> do and no sysfs attribute would be required.
>>
>
> Jacek,
>
> Thanks. Then I will retain current method.
>
> One question..What is the preferred way to name LED node in this case ?
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
> OR
> <location_code>
> ident <- attribute under each node
> fault
> attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
label = "powernv1:encl::ident:fault"
}
led2 {
label = "powernv2:desc:attn::fault:"
}
led2 {
label = "powernv3:desc:::fault:"
}
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-16 11:34 ` Jacek Anaszewski
@ 2015-04-20 7:29 ` Jacek Anaszewski
-1 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-20 7:29 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, Linux LED Subsystem, stewart, mpe, Bryan Wu,
Richard Purdie, khandual
Resending, as there were some problems with delivering this message.
-------- Original Message --------
Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Date: Thu, 16 Apr 2015 13:34:17 +0200
From: Jacek Anaszewski <j.anaszewski@samsung.com>
To: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
CC: linuxppc-dev@lists.ozlabs.org, linux-leds@vger.kernel.org,
stewart@linux.vnet.ibm.com, mpe@ellerman.id.au, cooloney@gmail.com,
rpurdie@rpsys.net, khandual@linux.vnet.ibm.com
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
> On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>> .../...
>>>
>>>>>>>>
>>>>>>>
>>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>>
>>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>>
>>>>> Sorry. I think comment was confusing..
>>>>>
>>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>>> case, we don't
>>>>> want to change the LED state during system shutdown/reboot.
>>>>>
>>>>> Hence I have introduced state variable here. So during register call, I just
>>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>>> state even after unloading module.
>>>>>
>>>>> Please let me know if there is any better way to achieve this.
>>>>
>>>> Since this is not a feature of the device, but rather a use case, then
>>>> it cannot be hard coded in the driver this way. The solution I see is
>>>> introducing a sysfs attribute that would determine if we want the
>>>> LED to be turned off on unregistration or not.
>>>
>>> Hmmm. I thought having simple various is enough ..
>>>
>>> I don't know the usage of other LED drivers .. Just a thought .. How about
>>> enabling this feature in LED class itself ...Something like:
>>> - Add new attribute to generic LED class (say persistent)?
>>> - If drivers wants to retain LED state across reboot, then enable this option
>>> - During unregister call check for this attribute
>>>
>>>>
>>>> You can refer to the following driver to find out how to add the
>>>> sysfs attribute:
>>>>
>>>> drivers/leds/leds-lm3533.c
>>>>
>>>> The attribute will also have to be documented, similarly to these:
>>>>
>>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>>
>>>> Currently I don't have a good candidate for attribute
>>>> name, so feel free to propose one.
>>>
>>> If you don't like generic attribute, then shall I introduce "persistent"
>>> attribute inside my driver. ?
>>> - By default this attribute is ON and if user wants he can disable this .
>>> - And I will have another variable (say op_support).. which I will disable in
>>> unload path..
>>> .../...
>>>
>>>>>>
>>>>>> The label could be composed of segments and an ordinal number as
>>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>>> The segments would have to be parsed by the driver to discover
>>>>>> all the LED's available modes.
>>>>>>
>>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>>> Could you describe the purpose of this mode, so that we could come
>>>>>> up with a better name?
>>>>>
>>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>>> which
>>>>> can have below states :
>>>>> - OFF : no action
>>>>> - Identify: blinking state (user can use this state to identify particular
>>>>> component).
>>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>>> retained same name here.
>>>>> How about just "ident" ?
>>>>
>>>> Sounds good.
>>>>
>>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>>> Note that our FW is capable of isolating some of the issues and it
>>>>> can turn
>>>>> on LEDs without OS
>>>>> interference.
>>>>
>>>> Does it mean that the LED can be controlled from hardware?
>>>> If so, what would be software use cases then? The same question is
>>>> related to the attn and indent states.
>>>
>>> - Identify LEDs:
>>> We have service processor interface to set/reset this LEDs.. Similar operation
>>> can be done from inband interface as well (via user space tools in Linux)..
>>> Later management layer can make use of this.
>>>
>>> - Fault / Attention :
>>> FW can SET these LEDs if its capable of isolating problem.
>>> Similarly host/user space can SET these LEDs if then can do fine grained
>>> problem isolation etc.
>>> Host/user space can RESET these LEDs.
>>>
>>> Hence we are adding host support to all the LEDs which can be modified by host.
>>>
>>> Hope this clarifies.
>>
>> Thanks for this explanation.
>>
>> I am changing my mind about these LEDs. Since they can be controlled
>> from hardware without any system interaction, then turning them off
>> on driver removal makes no sense. The LEDs could be turned on again even
>> if the driver is unloaded.
>> Since the driver doesn't perform any initialization of the LEDs,
>> neither should it turn them off on removal.
>>
>> If I understand this correctly, then the solution with variable would
>> do and no sysfs attribute would be required.
>>
>
> Jacek,
>
> Thanks. Then I will retain current method.
>
> One question..What is the preferred way to name LED node in this case ?
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
> OR
> <location_code>
> ident <- attribute under each node
> fault
> attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
label = "powernv1:encl::ident:fault"
}
led2 {
label = "powernv2:desc:attn::fault:"
}
led2 {
label = "powernv3:desc:::fault:"
}
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-20 7:29 ` Jacek Anaszewski
0 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-20 7:29 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, Bryan Wu, Richard Purdie, linuxppc-dev,
Linux LED Subsystem, khandual
Resending, as there were some problems with delivering this message.
-------- Original Message --------
Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Date: Thu, 16 Apr 2015 13:34:17 +0200
From: Jacek Anaszewski <j.anaszewski@samsung.com>
To: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
CC: linuxppc-dev@lists.ozlabs.org, linux-leds@vger.kernel.org,
stewart@linux.vnet.ibm.com, mpe@ellerman.id.au, cooloney@gmail.com,
rpurdie@rpsys.net, khandual@linux.vnet.ibm.com
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
> On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> On 04/16/2015 08:52 AM, Vasant Hegde wrote:
>>> On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
>>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>>>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>>>>> Hi Vasant,
>>>
>>> Hi Jacek,
>>>
>>> .../...
>>>
>>>>>>>>
>>>>>>>
>>>>>>> I mean, we have to retain the state of LED across system reboot.
>>>>>>
>>>>>> Static variables are reinitialized on system reboot, aren't they?
>>>>>
>>>>> Sorry. I think comment was confusing..
>>>>>
>>>>> As I understood, classdev_unregister call resets all LEDs state. However in our
>>>>> case, we don't
>>>>> want to change the LED state during system shutdown/reboot.
>>>>>
>>>>> Hence I have introduced state variable here. So during register call, I just
>>>>> disable LEDs so that any further call will just return. That way we retain LED
>>>>> state even after unloading module.
>>>>>
>>>>> Please let me know if there is any better way to achieve this.
>>>>
>>>> Since this is not a feature of the device, but rather a use case, then
>>>> it cannot be hard coded in the driver this way. The solution I see is
>>>> introducing a sysfs attribute that would determine if we want the
>>>> LED to be turned off on unregistration or not.
>>>
>>> Hmmm. I thought having simple various is enough ..
>>>
>>> I don't know the usage of other LED drivers .. Just a thought .. How about
>>> enabling this feature in LED class itself ...Something like:
>>> - Add new attribute to generic LED class (say persistent)?
>>> - If drivers wants to retain LED state across reboot, then enable this option
>>> - During unregister call check for this attribute
>>>
>>>>
>>>> You can refer to the following driver to find out how to add the
>>>> sysfs attribute:
>>>>
>>>> drivers/leds/leds-lm3533.c
>>>>
>>>> The attribute will also have to be documented, similarly to these:
>>>>
>>>> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
>>>>
>>>> Currently I don't have a good candidate for attribute
>>>> name, so feel free to propose one.
>>>
>>> If you don't like generic attribute, then shall I introduce "persistent"
>>> attribute inside my driver. ?
>>> - By default this attribute is ON and if user wants he can disable this .
>>> - And I will have another variable (say op_support).. which I will disable in
>>> unload path..
>>> .../...
>>>
>>>>>>
>>>>>> The label could be composed of segments and an ordinal number as
>>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>>> The segments would have to be parsed by the driver to discover
>>>>>> all the LED's available modes.
>>>>>>
>>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>>> Could you describe the purpose of this mode, so that we could come
>>>>>> up with a better name?
>>>>>
>>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>>> which
>>>>> can have below states :
>>>>> - OFF : no action
>>>>> - Identify: blinking state (user can use this state to identify particular
>>>>> component).
>>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>>> retained same name here.
>>>>> How about just "ident" ?
>>>>
>>>> Sounds good.
>>>>
>>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>>> Note that our FW is capable of isolating some of the issues and it
>>>>> can turn
>>>>> on LEDs without OS
>>>>> interference.
>>>>
>>>> Does it mean that the LED can be controlled from hardware?
>>>> If so, what would be software use cases then? The same question is
>>>> related to the attn and indent states.
>>>
>>> - Identify LEDs:
>>> We have service processor interface to set/reset this LEDs.. Similar operation
>>> can be done from inband interface as well (via user space tools in Linux)..
>>> Later management layer can make use of this.
>>>
>>> - Fault / Attention :
>>> FW can SET these LEDs if its capable of isolating problem.
>>> Similarly host/user space can SET these LEDs if then can do fine grained
>>> problem isolation etc.
>>> Host/user space can RESET these LEDs.
>>>
>>> Hence we are adding host support to all the LEDs which can be modified by host.
>>>
>>> Hope this clarifies.
>>
>> Thanks for this explanation.
>>
>> I am changing my mind about these LEDs. Since they can be controlled
>> from hardware without any system interaction, then turning them off
>> on driver removal makes no sense. The LEDs could be turned on again even
>> if the driver is unloaded.
>> Since the driver doesn't perform any initialization of the LEDs,
>> neither should it turn them off on removal.
>>
>> If I understand this correctly, then the solution with variable would
>> do and no sysfs attribute would be required.
>>
>
> Jacek,
>
> Thanks. Then I will retain current method.
>
> One question..What is the preferred way to name LED node in this case ?
>
> <location_code>:<ATTENTION|IDENTIFY|FAULT>
> OR
> <location_code>
> ident <- attribute under each node
> fault
> attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
label = "powernv1:encl::ident:fault"
}
led2 {
label = "powernv2:desc:attn::fault:"
}
led2 {
label = "powernv3:desc:::fault:"
}
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 10:15 ` Vasant Hegde
@ 2015-04-20 11:45 ` Jacek Anaszewski
-1 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-20 11:45 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, stewart, mpe, cooloney, rpurdie, khandual
Hi Vasant,
I'd like to clarify some details regarding your explanation.
On 04/15/2015 12:15 PM, Vasant Hegde wrote:
[...]
>>>
>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>> fault depending on their state --- blinking = identify and solid = fault).
>>> Hence here append LED type info.
>>
>> The label could be composed of segments and an ordinal number as
>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>> The segments would have to be parsed by the driver to discover
>> all the LED's available modes.
>>
>> nitpicking: identify is a verb and is not a proper name for the LED.
>> Could you describe the purpose of this mode, so that we could come
>> up with a better name?
>
> Each component (Field Replacement Unit) will have service indicator (LEDS) which
> can have below states :
> - OFF : no action
> - Identify: blinking state (user can use this state to identify particular
> component).
> In Power Systems world we call it as "identify" indicator.. Hence I
> retained same name here.
> How about just "ident" ?
> - fault : solid state (when component goes bad, LED goes to solid state)
> Note that our FW is capable of isolating some of the issues and it can turn
> on LEDs without OS
> interference.
>
> We have one more System level LED (System Attention Indicator).. This LED has
> two states:
> - OFF : Everything is fine
> - ON : Some component has issues and needs attention.
We have three modes:
- identify - blinking
- fault - solid
- attention indicator - solid
How does LED operation differ for fault and attention modes?
Does a LED have different intensity?
I'd rather avoid creating separate LED class devices for each mode.
For blinking we could use existing timer trigger.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-20 11:45 ` Jacek Anaszewski
0 siblings, 0 replies; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-20 11:45 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
Hi Vasant,
I'd like to clarify some details regarding your explanation.
On 04/15/2015 12:15 PM, Vasant Hegde wrote:
[...]
>>>
>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>> fault depending on their state --- blinking = identify and solid = fault).
>>> Hence here append LED type info.
>>
>> The label could be composed of segments and an ordinal number as
>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>> The segments would have to be parsed by the driver to discover
>> all the LED's available modes.
>>
>> nitpicking: identify is a verb and is not a proper name for the LED.
>> Could you describe the purpose of this mode, so that we could come
>> up with a better name?
>
> Each component (Field Replacement Unit) will have service indicator (LEDS) which
> can have below states :
> - OFF : no action
> - Identify: blinking state (user can use this state to identify particular
> component).
> In Power Systems world we call it as "identify" indicator.. Hence I
> retained same name here.
> How about just "ident" ?
> - fault : solid state (when component goes bad, LED goes to solid state)
> Note that our FW is capable of isolating some of the issues and it can turn
> on LEDs without OS
> interference.
>
> We have one more System level LED (System Attention Indicator).. This LED has
> two states:
> - OFF : Everything is fine
> - ON : Some component has issues and needs attention.
We have three modes:
- identify - blinking
- fault - solid
- attention indicator - solid
How does LED operation differ for fault and attention modes?
Does a LED have different intensity?
I'd rather avoid creating separate LED class devices for each mode.
For blinking we could use existing timer trigger.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-20 11:45 ` Jacek Anaszewski
(?)
@ 2015-04-20 12:34 ` Vasant Hegde
2015-04-20 15:20 ` Jacek Anaszewski
-1 siblings, 1 reply; 65+ messages in thread
From: Vasant Hegde @ 2015-04-20 12:34 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/20/2015 05:15 PM, Jacek Anaszewski wrote:
> Hi Vasant,
>
> I'd like to clarify some details regarding your explanation.
>
> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
> [...]
>>>>
>>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>>> fault depending on their state --- blinking = identify and solid = fault).
>>>> Hence here append LED type info.
>>>
>>> The label could be composed of segments and an ordinal number as
>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>> The segments would have to be parsed by the driver to discover
>>> all the LED's available modes.
>>>
>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>> Could you describe the purpose of this mode, so that we could come
>>> up with a better name?
>>
>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>> can have below states :
>> - OFF : no action
>> - Identify: blinking state (user can use this state to identify particular
>> component).
>> In Power Systems world we call it as "identify" indicator.. Hence I
>> retained same name here.
>> How about just "ident" ?
>> - fault : solid state (when component goes bad, LED goes to solid state)
>> Note that our FW is capable of isolating some of the issues and it can turn
>> on LEDs without OS
>> interference.
>>
>> We have one more System level LED (System Attention Indicator).. This LED has
>> two states:
>> - OFF : Everything is fine
>> - ON : Some component has issues and needs attention.
>
> We have three modes:
> - identify - blinking
> - fault - solid
> - attention indicator - solid
>
> How does LED operation differ for fault and attention modes?
> Does a LED have different intensity?
Jacek,
System Attention LED is special LED and its single LED available/system. where
as identify and fault is applicable to all field replaceable units in the system..
So Typical server will have
1 System Attention LED
N Identify/fault LED (N = Field Replaceable Unit).
Apart from above two, we do have two more LEDs/Enclosure (external visible LEDs)
- Enclosure Identify
- Enclosure fault
These LEDs reflects state of all Field Replaceable Units (FRU) inside this
enclosure
If any of FRU state is ON, this will become ON
Also we can independently enable this LED!!
But from kernel side implementation point of view, I just treat this as
another LED.. as our platform code (OPAL firmware) takes care of roll up etc.
Now our LED can operate in two mode (Depending on our service model, typically
one/two socket servers are Light Path mode, whereas high end servers are Guiding
Light Mode).
1. Guiding Light
Only Identify indicator is support.. Fault is not supported
System attention indicator is used to point there is some problem in system
and need attention
2. Light Path mode
Both identify and fault indicator is supported ..
Fault is ON whenever some component is faulty
System attention indicator is used to point that FW/OS is not able to
isolate the problem and needs user to look into serviceable event (like syslog/
our agents like ESA which analyzes and reports events)
Handling LED states :
- Though physically single LED is overloaded for identify and fault, logically
(FW/OS level) we treat them as separate LED.
- We can enable both fault and identify simultaneously.
- Hardware decides physical LED state (rule : identify has priority over fault).
ex: Say location code 'X',
Identify = ON, fault = ON , state of 'X' = identify (blinking)
Identify = OFF, fault = ON, state of 'X' = fault (solid)
Identify = OFF, fault = ON, state of 'X' = identify (blinking)
Identify = OFF, fault = OFF , state of 'X' = OFF
Since we have various above combinations, I thought its best to have separate
class dev for each individual LEDs. That way we keep everything simple and let
firmware handle all complexities.
Hope this clarifies.
I just posted v3 where I addressed your comments.
https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/127702.html
Please let me know if you have any comments/suggestions.
-Vasant
>
> I'd rather avoid creating separate LED class devices for each mode.
>
> For blinking we could use existing timer trigger.
>
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-20 12:34 ` Vasant Hegde
@ 2015-04-20 15:20 ` Jacek Anaszewski
2015-04-20 15:53 ` Vasant Hegde
0 siblings, 1 reply; 65+ messages in thread
From: Jacek Anaszewski @ 2015-04-20 15:20 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/20/2015 02:34 PM, Vasant Hegde wrote:
> On 04/20/2015 05:15 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>> I'd like to clarify some details regarding your explanation.
>>
>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>> [...]
>>>>>
>>>>> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
>>>>> fault depending on their state --- blinking = identify and solid = fault).
>>>>> Hence here append LED type info.
>>>>
>>>> The label could be composed of segments and an ordinal number as
>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>> The segments would have to be parsed by the driver to discover
>>>> all the LED's available modes.
>>>>
>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>> Could you describe the purpose of this mode, so that we could come
>>>> up with a better name?
>>>
>>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>>> can have below states :
>>> - OFF : no action
>>> - Identify: blinking state (user can use this state to identify particular
>>> component).
>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>> retained same name here.
>>> How about just "ident" ?
>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>> Note that our FW is capable of isolating some of the issues and it can turn
>>> on LEDs without OS
>>> interference.
>>>
>>> We have one more System level LED (System Attention Indicator).. This LED has
>>> two states:
>>> - OFF : Everything is fine
>>> - ON : Some component has issues and needs attention.
>>
>> We have three modes:
>> - identify - blinking
>> - fault - solid
>> - attention indicator - solid
>>
>> How does LED operation differ for fault and attention modes?
>> Does a LED have different intensity?
>
> Jacek,
>
> System Attention LED is special LED and its single LED available/system. where
> as identify and fault is applicable to all field replaceable units in the system..
>
> So Typical server will have
> 1 System Attention LED
> N Identify/fault LED (N = Field Replaceable Unit).
>
> Apart from above two, we do have two more LEDs/Enclosure (external visible LEDs)
> - Enclosure Identify
> - Enclosure fault
> These LEDs reflects state of all Field Replaceable Units (FRU) inside this
> enclosure
> If any of FRU state is ON, this will become ON
> Also we can independently enable this LED!!
>
> But from kernel side implementation point of view, I just treat this as
> another LED.. as our platform code (OPAL firmware) takes care of roll up etc.
>
>
> Now our LED can operate in two mode (Depending on our service model, typically
> one/two socket servers are Light Path mode, whereas high end servers are Guiding
> Light Mode).
>
> 1. Guiding Light
> Only Identify indicator is support.. Fault is not supported
> System attention indicator is used to point there is some problem in system
> and need attention
> 2. Light Path mode
> Both identify and fault indicator is supported ..
> Fault is ON whenever some component is faulty
> System attention indicator is used to point that FW/OS is not able to
> isolate the problem and needs user to look into serviceable event (like syslog/
> our agents like ESA which analyzes and reports events)
>
>
> Handling LED states :
> - Though physically single LED is overloaded for identify and fault, logically
> (FW/OS level) we treat them as separate LED.
> - We can enable both fault and identify simultaneously.
> - Hardware decides physical LED state (rule : identify has priority over fault).
> ex: Say location code 'X',
> Identify = ON, fault = ON , state of 'X' = identify (blinking)
> Identify = OFF, fault = ON, state of 'X' = fault (solid)
> Identify = OFF, fault = ON, state of 'X' = identify (blinking)
> Identify = OFF, fault = OFF , state of 'X' = OFF
>
> Since we have various above combinations, I thought its best to have separate
> class dev for each individual LEDs. That way we keep everything simple and let
> firmware handle all complexities.
>
> Hope this clarifies.
>
> I just posted v3 where I addressed your comments.
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/127702.html
>
> Please let me know if you have any comments/suggestions.
From what I can see from the driver code the LEDs are set with:
opal_leds_set_ind(token, loc_code, led_mask, led_value, &max_led_type);
and their state can be read with:
opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type)
From the kernel point of view these are very simple operations.
All the logic you described should be handled by user space.
If you need to be able to specify the LED mode you want to set/read,
then additional 'mode' sysfs attribute should be added by the driver.
There would have to be also additional sysfs attribute 'available_modes"
provided. The ABI documentation should inform how the mode identifiers
map to the modes. I already explained how to add it, when we were
discussing about retaining led state on remove.
I'd see following use cases.
(let's assume that modes are defined as follows:
0: ident, 1: attn, 2: fault)
#cat available_modes
#0 1 2
#echo 0 > mode //set ident mode
#echo 1 > brightness //set ident state
#echo 2 > mode //set fault mode
#cat brightness //read fault state
#0
#echo 1 > attn //set attn mode
#echo 1 > brightness
This would set the LED in blinking mode, so I am wondering if we
shouldn't employ timer trigger for this to keep the LED API consistent.
Can a single LED support other mode than 'attention'? I'd like to know
if a LED in attention mode (blinking), can be set to some solid
mode?
Please let me know if such an approach would still not fit for your
requirements.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-20 15:20 ` Jacek Anaszewski
@ 2015-04-20 15:53 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-20 15:53 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/20/2015 08:50 PM, Jacek Anaszewski wrote:
> On 04/20/2015 02:34 PM, Vasant Hegde wrote:
>> On 04/20/2015 05:15 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
>>>
>>> I'd like to clarify some details regarding your explanation.
>>>
>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>>> [...]
>>>>>>
>>>>>> In Power Systems LEDs are overloaded (meaning same LED is used for
>>>>>> identify and
>>>>>> fault depending on their state --- blinking = identify and solid = fault).
>>>>>> Hence here append LED type info.
>>>>>
>>>>> The label could be composed of segments and an ordinal number as
>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1.
>>>>> The segments would have to be parsed by the driver to discover
>>>>> all the LED's available modes.
>>>>>
>>>>> nitpicking: identify is a verb and is not a proper name for the LED.
>>>>> Could you describe the purpose of this mode, so that we could come
>>>>> up with a better name?
>>>>
>>>> Each component (Field Replacement Unit) will have service indicator (LEDS)
>>>> which
>>>> can have below states :
>>>> - OFF : no action
>>>> - Identify: blinking state (user can use this state to identify particular
>>>> component).
>>>> In Power Systems world we call it as "identify" indicator.. Hence I
>>>> retained same name here.
>>>> How about just "ident" ?
>>>> - fault : solid state (when component goes bad, LED goes to solid state)
>>>> Note that our FW is capable of isolating some of the issues and it
>>>> can turn
>>>> on LEDs without OS
>>>> interference.
>>>>
>>>> We have one more System level LED (System Attention Indicator).. This LED has
>>>> two states:
>>>> - OFF : Everything is fine
>>>> - ON : Some component has issues and needs attention.
>>>
>>> We have three modes:
>>> - identify - blinking
>>> - fault - solid
>>> - attention indicator - solid
>>>
>>> How does LED operation differ for fault and attention modes?
>>> Does a LED have different intensity?
>>
>> Jacek,
>>
>> System Attention LED is special LED and its single LED available/system. where
>> as identify and fault is applicable to all field replaceable units in the
>> system..
>>
>> So Typical server will have
>> 1 System Attention LED
>> N Identify/fault LED (N = Field Replaceable Unit).
>>
>> Apart from above two, we do have two more LEDs/Enclosure (external visible LEDs)
>> - Enclosure Identify
>> - Enclosure fault
>> These LEDs reflects state of all Field Replaceable Units (FRU) inside this
>> enclosure
>> If any of FRU state is ON, this will become ON
>> Also we can independently enable this LED!!
>>
>> But from kernel side implementation point of view, I just treat this as
>> another LED.. as our platform code (OPAL firmware) takes care of roll up etc.
>>
>>
>> Now our LED can operate in two mode (Depending on our service model, typically
>> one/two socket servers are Light Path mode, whereas high end servers are Guiding
>> Light Mode).
>>
>> 1. Guiding Light
>> Only Identify indicator is support.. Fault is not supported
>> System attention indicator is used to point there is some problem in system
>> and need attention
>> 2. Light Path mode
>> Both identify and fault indicator is supported ..
>> Fault is ON whenever some component is faulty
>> System attention indicator is used to point that FW/OS is not able to
>> isolate the problem and needs user to look into serviceable event (like syslog/
>> our agents like ESA which analyzes and reports events)
>>
>>
>> Handling LED states :
>> - Though physically single LED is overloaded for identify and fault, logically
>> (FW/OS level) we treat them as separate LED.
>> - We can enable both fault and identify simultaneously.
>> - Hardware decides physical LED state (rule : identify has priority over
>> fault).
>> ex: Say location code 'X',
>> Identify = ON, fault = ON , state of 'X' = identify (blinking)
>> Identify = OFF, fault = ON, state of 'X' = fault (solid)
>> Identify = OFF, fault = ON, state of 'X' = identify (blinking)
>> Identify = OFF, fault = OFF , state of 'X' = OFF
>>
>> Since we have various above combinations, I thought its best to have separate
>> class dev for each individual LEDs. That way we keep everything simple and let
>> firmware handle all complexities.
>>
>> Hope this clarifies.
>>
>> I just posted v3 where I addressed your comments.
>> https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/127702.html
>>
>> Please let me know if you have any comments/suggestions.
Jacek,
>
> From what I can see from the driver code the LEDs are set with:
>
> opal_leds_set_ind(token, loc_code, led_mask, led_value, &max_led_type);
>
> and their state can be read with:
>
> opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type)
>
> From the kernel point of view these are very simple operations.
>
> All the logic you described should be handled by user space.
> If you need to be able to specify the LED mode you want to set/read,
> then additional 'mode' sysfs attribute should be added by the driver.
> There would have to be also additional sysfs attribute 'available_modes"
> provided. The ABI documentation should inform how the mode identifiers
> map to the modes. I already explained how to add it, when we were
> discussing about retaining led state on remove.
Sorry..My fault.. I should have elaborated mode operation...
I forgot to mention that LED mode is static... meaning platform provides this
information, but we cannot change during runtime..
Presently we have this information in Device Tree. Since this is static one (and
also LED Mode is system wide.. nothing to do individual LED), I didn't add it
in LED driver code.. .Do you think we should add that property ?
>
>
> I'd see following use cases.
>
> (let's assume that modes are defined as follows:
> 0: ident, 1: attn, 2: fault)
Modes are : Guiding Light / Light Path ... which is static and platform provides
this information.
LED types : IDENT, FAULT and ATTN .... which can be get/set/reset by OS
(kernel/userspace)
Also only 1 LED can be ATTN ...
>
> #cat available_modes
> #0 1 2
> #echo 0 > mode //set ident mode
> #echo 1 > brightness //set ident state
> #echo 2 > mode //set fault mode
> #cat brightness //read fault state
> #0
> #echo 1 > attn //set attn mode
> #echo 1 > brightness
>
> This would set the LED in blinking mode, so I am wondering if we
> shouldn't employ timer trigger for this to keep the LED API consistent.
>
> Can a single LED support other mode than 'attention'? I'd like to know
> if a LED in attention mode (blinking), can be set to some solid
> mode?
No.. Its always single attention LED/system ... which can be Set (Solid) / reset
state.
>
> Please let me know if such an approach would still not fit for your
> requirements.
>
Given above conditions, I think current approach (my v3 patchset) is simple and
works better. What you say?
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-14 15:20 ` Jacek Anaszewski
@ 2015-04-15 18:50 ` Stewart Smith
-1 siblings, 0 replies; 65+ messages in thread
From: Stewart Smith @ 2015-04-15 18:50 UTC (permalink / raw)
To: Jacek Anaszewski, Vasant Hegde
Cc: linuxppc-dev, linux-leds, mpe, cooloney, rpurdie, khandual
Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>> +static struct platform_driver powernv_led_driver = {
>> + .probe = powernv_led_probe,
>> + .remove = powernv_led_remove,
>> + .driver = {
>> + .name = "powernv-led-driver",
>> + .owner = THIS_MODULE,
>> + .of_match_table = powernv_led_match,
>
> Is somewhere DT documentation available for these leds?
https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
We've been documenting things in firmware source, and I'm open to if we
should mirror this somewhere (somewhere in linux Documentation/ ?).
If we duplicate everything, we shouldn't let things get out of date.
FWIW I won't merge code that changes device tree that skiboot emits
without associated documentation.
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-15 18:50 ` Stewart Smith
0 siblings, 0 replies; 65+ messages in thread
From: Stewart Smith @ 2015-04-15 18:50 UTC (permalink / raw)
To: Jacek Anaszewski, Vasant Hegde
Cc: cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>> +static struct platform_driver powernv_led_driver = {
>> + .probe = powernv_led_probe,
>> + .remove = powernv_led_remove,
>> + .driver = {
>> + .name = "powernv-led-driver",
>> + .owner = THIS_MODULE,
>> + .of_match_table = powernv_led_match,
>
> Is somewhere DT documentation available for these leds?
https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
We've been documenting things in firmware source, and I'm open to if we
should mirror this somewhere (somewhere in linux Documentation/ ?).
If we duplicate everything, we shouldn't let things get out of date.
FWIW I won't merge code that changes device tree that skiboot emits
without associated documentation.
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-15 18:50 ` Stewart Smith
@ 2015-04-16 5:07 ` Vasant Hegde
-1 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 5:07 UTC (permalink / raw)
To: Stewart Smith, Jacek Anaszewski
Cc: linuxppc-dev, linux-leds, mpe, cooloney, rpurdie, khandual
On 04/16/2015 12:20 AM, Stewart Smith wrote:
> Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>>> +static struct platform_driver powernv_led_driver = {
>>> + .probe = powernv_led_probe,
>>> + .remove = powernv_led_remove,
>>> + .driver = {
>>> + .name = "powernv-led-driver",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = powernv_led_match,
>>
>> Is somewhere DT documentation available for these leds?
>
> https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
>
> We've been documenting things in firmware source, and I'm open to if we
> should mirror this somewhere (somewhere in linux Documentation/ ?).
Stewart,
I'm adding Documentation/devicetree/bindings/leds/leds-powernv.txt ...which is
pretty much similar to the device tree documentation we have in OPAL side.
>
> If we duplicate everything, we shouldn't let things get out of date.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-16 5:07 ` Vasant Hegde
0 siblings, 0 replies; 65+ messages in thread
From: Vasant Hegde @ 2015-04-16 5:07 UTC (permalink / raw)
To: Stewart Smith, Jacek Anaszewski
Cc: cooloney, rpurdie, linuxppc-dev, linux-leds, khandual
On 04/16/2015 12:20 AM, Stewart Smith wrote:
> Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>>> +static struct platform_driver powernv_led_driver = {
>>> + .probe = powernv_led_probe,
>>> + .remove = powernv_led_remove,
>>> + .driver = {
>>> + .name = "powernv-led-driver",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = powernv_led_match,
>>
>> Is somewhere DT documentation available for these leds?
>
> https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
>
> We've been documenting things in firmware source, and I'm open to if we
> should mirror this somewhere (somewhere in linux Documentation/ ?).
Stewart,
I'm adding Documentation/devicetree/bindings/leds/leds-powernv.txt ...which is
pretty much similar to the device tree documentation we have in OPAL side.
>
> If we duplicate everything, we shouldn't let things get out of date.
-Vasant
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
2015-04-16 5:07 ` Vasant Hegde
@ 2015-04-21 23:03 ` Stewart Smith
-1 siblings, 0 replies; 65+ messages in thread
From: Stewart Smith @ 2015-04-21 23:03 UTC (permalink / raw)
To: mpe
Cc: Vasant Hegde, Jacek Anaszewski, linuxppc-dev, linux-leds,
cooloney, rpurdie, khandual
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> On 04/16/2015 12:20 AM, Stewart Smith wrote:
>> Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>>>> +static struct platform_driver powernv_led_driver = {
>>>> + .probe = powernv_led_probe,
>>>> + .remove = powernv_led_remove,
>>>> + .driver = {
>>>> + .name = "powernv-led-driver",
>>>> + .owner = THIS_MODULE,
>>>> + .of_match_table = powernv_led_match,
>>>
>>> Is somewhere DT documentation available for these leds?
>>
>> https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
>>
>> We've been documenting things in firmware source, and I'm open to if we
>> should mirror this somewhere (somewhere in linux Documentation/ ?).
>
> Stewart,
>
> I'm adding Documentation/devicetree/bindings/leds/leds-powernv.txt ...which is
> pretty much similar to the device tree documentation we have in OPAL side.
Michael,
I know we've kind of discussed this in the past, but perhaps we should
come up with a conistent way of doing this for OPAL related docs.
^ permalink raw reply [flat|nested] 65+ messages in thread* Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
@ 2015-04-21 23:03 ` Stewart Smith
0 siblings, 0 replies; 65+ messages in thread
From: Stewart Smith @ 2015-04-21 23:03 UTC (permalink / raw)
To: mpe
Cc: cooloney, Vasant Hegde, rpurdie, linuxppc-dev, Jacek Anaszewski,
linux-leds, khandual
Vasant Hegde <hegdevasant@linux.vnet.ibm.com> writes:
> On 04/16/2015 12:20 AM, Stewart Smith wrote:
>> Jacek Anaszewski <j.anaszewski@samsung.com> writes:
>>>> +static struct platform_driver powernv_led_driver = {
>>>> + .probe = powernv_led_probe,
>>>> + .remove = powernv_led_remove,
>>>> + .driver = {
>>>> + .name = "powernv-led-driver",
>>>> + .owner = THIS_MODULE,
>>>> + .of_match_table = powernv_led_match,
>>>
>>> Is somewhere DT documentation available for these leds?
>>
>> https://github.com/open-power/skiboot/blob/master/doc/device-tree/ibm%2Copal/led.txt
>>
>> We've been documenting things in firmware source, and I'm open to if we
>> should mirror this somewhere (somewhere in linux Documentation/ ?).
>
> Stewart,
>
> I'm adding Documentation/devicetree/bindings/leds/leds-powernv.txt ...which is
> pretty much similar to the device tree documentation we have in OPAL side.
Michael,
I know we've kind of discussed this in the past, but perhaps we should
come up with a conistent way of doing this for OPAL related docs.
^ permalink raw reply [flat|nested] 65+ messages in thread