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@xxxxxxxxxxxxxxxxxx>
+ * Author: Vasant Hegde <hegdevasant@xxxxxxxxxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#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
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
