* [PATCH v5 0/3] LED driver for PowerNV platform
@ 2015-07-01 8:50 Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states Vasant Hegde
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vasant Hegde @ 2015-07-01 8:50 UTC (permalink / raw)
To: linuxppc-dev, linux-leds
Cc: benh, stewart, khandual, j.anaszewski81, cooloney, rpurdie, arnd,
mpe, Vasant Hegde
The following series implements LED driver for PowerNV platform.
PowerNV platform has below type of LEDs:
- System attention
Indicates there is a problem with the system that needs attention.
- Identify
Helps the user locate/identify a particular FRU or resource in the
system.
- Fault
Indicates there is a problem with the FRU or resource at the
location with which the indicator is associated.
On PowerNV (Non Virtualized) platform OPAL firmware provides LED information
to host via device tree (location code and LED type). During init we check
for 'ibm,opal/leds' node in device tree to enable LED driver. And we use
OPAL API's to get/set LEDs.
Note that on PowerNV platform firmware can activate fault LED, if it can isolate
the problem. Also one can modify the LEDs using service processor interface. None
of these involes kernel. Hence we retain LED state in unload path.
Sample LED device tree output:
------------------------------
leds {
compatible = "ibm,opal-v3-led";
led-mode = "lightpath";
U78C9.001.RST0027-P1-C1 {
led-types = "identify", "fault";
};
...
...
}
Sample sysfs output:
--------------------
.
├── U78CB.001.WZS008R-A1:fault
│ ├── brightness
│ ├── device -> ../../../opal_leds
│ ├── max_brightness
│ ├── power
│ │ ├── async
│ │ ├── autosuspend_delay_ms
│ │ ├── control
│ │ ├── runtime_active_kids
│ │ ├── runtime_active_time
│ │ ├── runtime_enabled
│ │ ├── runtime_status
│ │ ├── runtime_suspended_time
│ │ └── runtime_usage
│ ├── subsystem -> ../../../../../class/leds
│ ├── trigger
│ └── uevent
├── U78CB.001.WZS008R-A1:identify
│ ├── brightness
│ ├── device -> ../../../opal_leds
│ ├── max_brightness
│ ├── power
│ │ ├── async
│ │ ├── autosuspend_delay_ms
│ │ ├── control
│ │ ├── runtime_active_kids
│ │ ├── runtime_active_time
│ │ ├── runtime_enabled
│ │ ├── runtime_status
│ │ ├── runtime_suspended_time
│ │ └── runtime_usage
│ ├── subsystem -> ../../../../../class/leds
│ ├── trigger
│ └── uevent
....
....
....
patch 1/2: PowerNV architecture specific code. This adds necessary
OPAL APIs.
patch 2/2: Create LED platform device and export OPAL symbols
patch 3/3: Actual LED driver implemenation for PowerNV platform.
Note that this version of patchset is based on upstream Linus tree
as Jacek mention its better to go via LED subsystem (commit 05a8256c).
@Jacek,
Let me know if you want me to rebase this patchset on top of LED tree.
Previous patchset:
v4: https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/128028.html
v3: https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/127702.html
v2: https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-March/126301.html
v1: https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-March/125705.html
Changes in v5:
- Rebased on top of Linus tree
- Renamed led as leds and updated documentation
- As Ben and Arnd suggested, removed phandle from documenation
- As Ben suggested removed led-loc device tree property
- As Jacek suggested, added back compatible property to documentation
Changes in v4:
- Updated macros to reflect platform.
- s/u64/__be64/g for big endian data we get from firmware
- Addressed review comments from Jacek. Major once are:
Removed list in powernv_led_data structure
s/kzalloc/devm_kzalloc/
Removed compatible property from documentation
s/powernv_led_set_queue/powernv_brightness_set/
- Removed LED specific brightness_set/get function. Instead this version
uses single function to queue all LED set/get requests. Later we use
LED name to detect LED type and value.
- Removed hardcoded LED type used in previous version. Instead we use
led-types property to form LED classdev.
Changes in v3:
- Addressed review comments from Jacek. Major once are:
Replaced spin lock and mutex and removed redundant structures
Replaced pr_* with dev_*
Moved OPAL platform sepcific part to separate patch
Moved repteated code to common function
Added device tree documentation for LEDs
Changes in v2:
- Rebased patches on top of mpe's next branch
https://git.kernel.org/cgit/linux/kernel/git/mpe/linux.git/log/?h=next
- Added System Attention Indicator support
- Removed redundant code in leds-powernv.c file
Anshuman Khandual (1):
powerpc/powernv: Add OPAL interfaces for accessing and modifying
system LED states
Vasant Hegde (2):
powerpc/powernv: Create LED platform device
leds/powernv: Add driver for PowerNV platform
.../devicetree/bindings/leds/leds-powernv.txt | 27 ++
arch/powerpc/include/asm/opal-api.h | 29 +-
arch/powerpc/include/asm/opal.h | 4 +
arch/powerpc/platforms/powernv/opal-wrappers.S | 2 +
arch/powerpc/platforms/powernv/opal.c | 12 +-
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-powernv.c | 463 +++++++++++++++++++++
8 files changed, 547 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
create mode 100644 drivers/leds/leds-powernv.c
--
Vasant
2.1.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states
2015-07-01 8:50 [PATCH v5 0/3] LED driver for PowerNV platform Vasant Hegde
@ 2015-07-01 8:50 ` Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 2/3] powerpc/powernv: Create LED platform device Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform Vasant Hegde
2 siblings, 0 replies; 10+ messages in thread
From: Vasant Hegde @ 2015-07-01 8:50 UTC (permalink / raw)
To: linuxppc-dev, linux-leds
Cc: benh, stewart, khandual, j.anaszewski81, cooloney, rpurdie, arnd,
mpe, Vasant Hegde
From: Anshuman Khandual <khandual@linux.vnet.ibm.com>
This patch registers the following two new OPAL interfaces calls
for the platform LED subsystem. With the help of these new OPAL calls,
the kernel will be able to get or set the state of various individual
LEDs on the system at any given location code which is passed through
the LED specific device tree nodes.
(1) OPAL_LEDS_GET_INDICATOR opal_leds_get_ind
(2) OPAL_LEDS_SET_INDICATOR opal_leds_set_ind
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>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/include/asm/opal-api.h | 25 ++++++++++++++++++++++++-
arch/powerpc/include/asm/opal.h | 4 ++++
arch/powerpc/platforms/powernv/opal-wrappers.S | 2 ++
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
index e9e4c52..8f8c45f 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -154,7 +154,9 @@
#define OPAL_FLASH_WRITE 111
#define OPAL_FLASH_ERASE 112
#define OPAL_PRD_MSG 113
-#define OPAL_LAST 113
+#define OPAL_LEDS_GET_INDICATOR 114
+#define OPAL_LEDS_SET_INDICATOR 115
+#define OPAL_LAST 115
/* Device tree flags */
@@ -756,6 +758,27 @@ struct opal_i2c_request {
__be64 buffer_ra; /* Buffer real address */
};
+/* LED Mode */
+#define POWERNV_LED_MODE_LIGHT_PATH "lightpath"
+#define POWERNV_LED_MODE_GUIDING_LIGHT "guidinglight"
+
+/* LED type */
+#define POWERNV_LED_TYPE_IDENTIFY "identify"
+#define POWERNV_LED_TYPE_FAULT "fault"
+#define POWERNV_LED_TYPE_ATTENTION "attention"
+
+enum OpalSlotLedType {
+ OPAL_SLOT_LED_TYPE_ID = 0, /* IDENTIFY LED */
+ OPAL_SLOT_LED_TYPE_FAULT = 1, /* FAULT LED */
+ OPAL_SLOT_LED_TYPE_ATTN = 2, /* System Attention LED */
+ OPAL_SLOT_LED_TYPE_MAX = 3
+};
+
+enum OpalSlotLedState {
+ OPAL_SLOT_LED_STATE_OFF = 0, /* LED is OFF */
+ OPAL_SLOT_LED_STATE_ON = 1 /* LED is ON */
+};
+
#endif /* __ASSEMBLY__ */
#endif /* __OPAL_API_H */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 958e941..03ef848 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -195,6 +195,10 @@ int64_t opal_ipmi_recv(uint64_t interface, struct opal_ipmi_msg *msg,
int64_t opal_i2c_request(uint64_t async_token, uint32_t bus_id,
struct opal_i2c_request *oreq);
int64_t opal_prd_msg(struct opal_prd_msg *msg);
+int64_t opal_leds_get_ind(char *loc_code, u64 *led_mask,
+ u64 *led_value, u64 *max_led_type);
+int64_t opal_leds_set_ind(uint64_t token, char *loc_code, const u64 led_mask,
+ const u64 led_value, u64 *max_led_type);
int64_t opal_flash_read(uint64_t id, uint64_t offset, uint64_t buf,
uint64_t size, uint64_t token);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S b/arch/powerpc/platforms/powernv/opal-wrappers.S
index d6a7b82..34c2734 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -297,3 +297,5 @@ OPAL_CALL(opal_flash_read, OPAL_FLASH_READ);
OPAL_CALL(opal_flash_write, OPAL_FLASH_WRITE);
OPAL_CALL(opal_flash_erase, OPAL_FLASH_ERASE);
OPAL_CALL(opal_prd_msg, OPAL_PRD_MSG);
+OPAL_CALL(opal_leds_get_ind, OPAL_LEDS_GET_INDICATOR);
+OPAL_CALL(opal_leds_set_ind, OPAL_LEDS_SET_INDICATOR);
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/3] powerpc/powernv: Create LED platform device
2015-07-01 8:50 [PATCH v5 0/3] LED driver for PowerNV platform Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states Vasant Hegde
@ 2015-07-01 8:50 ` Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform Vasant Hegde
2 siblings, 0 replies; 10+ messages in thread
From: Vasant Hegde @ 2015-07-01 8:50 UTC (permalink / raw)
To: linuxppc-dev, linux-leds
Cc: benh, stewart, khandual, j.anaszewski81, cooloney, rpurdie, arnd,
mpe, Vasant Hegde
This patch adds platform devices for leds. Also export LED related
OPAL API's so that led driver can use these APIs.
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/platforms/powernv/opal.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index f084afa..6839358 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -648,7 +648,7 @@ static void opal_init_heartbeat(void)
static int __init opal_init(void)
{
- struct device_node *np, *consoles;
+ struct device_node *np, *consoles, *leds;
int rc;
opal_node = of_find_node_by_path("/ibm,opal");
@@ -689,6 +689,13 @@ static int __init opal_init(void)
/* Setup a heatbeat thread if requested by OPAL */
opal_init_heartbeat();
+ /* Create leds platform devices */
+ leds = of_find_node_by_path("/ibm,opal/leds");
+ if (leds) {
+ of_platform_device_create(leds, "opal_leds", NULL);
+ of_node_put(leds);
+ }
+
/* Create "opal" kobject under /sys/firmware */
rc = opal_sysfs_init();
if (rc == 0) {
@@ -841,3 +848,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);
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-01 8:50 [PATCH v5 0/3] LED driver for PowerNV platform Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 2/3] powerpc/powernv: Create LED platform device Vasant Hegde
@ 2015-07-01 8:50 ` Vasant Hegde
2015-07-14 9:00 ` Jacek Anaszewski
2 siblings, 1 reply; 10+ messages in thread
From: Vasant Hegde @ 2015-07-01 8:50 UTC (permalink / raw)
To: linuxppc-dev, linux-leds
Cc: benh, stewart, khandual, j.anaszewski81, cooloney, rpurdie, arnd,
mpe, Vasant Hegde
This patch implements LED driver for PowerNV platform using the existing
generic LED class framework.
PowerNV platform has below type of LEDs:
- System attention
Indicates there is a problem with the system that needs attention.
- Identify
Helps the user locate/identify a particular FRU or resource in the
system.
- Fault
Indicates there is a problem with the FRU or resource at the
location with which the indicator is associated.
We register 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.
All the system LEDs can be found in the same regular path /sys/class/leds/.
We don't use LED colors. We use LED node and led-types property to form
LED classdev. 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 (0) for any turned off LED.
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.
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.
Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
---
.../devicetree/bindings/leds/leds-powernv.txt | 24 ++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-powernv.c | 463 +++++++++++++++++++++
4 files changed, 499 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
create mode 100644 drivers/leds/leds-powernv.c
diff --git a/Documentation/devicetree/bindings/leds/leds-powernv.txt b/Documentation/devicetree/bindings/leds/leds-powernv.txt
new file mode 100644
index 0000000..636f028
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
@@ -0,0 +1,24 @@
+Device Tree binding for LEDs on IBM Power Systems
+-------------------------------------------------
+
+The 'leds' node under '/ibm,opal' lists service indicators available in the
+system and their capabilities.
+
+leds {
+ compatible = "ibm,opal-v3-led";
+ led-mode = "lightpath";
+
+ U78C9.001.RST0027-P1-C1 {
+ led-types = "identify", "fault";
+ };
+ ...
+ ...
+};
+
+Each node under 'leds' node describes location code of FRU/Enclosure.
+
+compatible : should be : "ibm,opal-v3-led".
+
+The properties under each node:
+
+ led-types : Supported LED types (attention/identify/fault).
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4191614..4f56c7a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -505,6 +505,17 @@ config LEDS_BLINKM
This option enables support for the BlinkM RGB LED connected
through I2C. Say Y to enable support for the BlinkM LED.
+config LEDS_POWERNV
+ tristate "LED support for PowerNV Platform"
+ depends on LEDS_CLASS
+ depends on PPC_POWERNV
+ depends on OF
+ help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ To compile this driver as a module, choose 'm' here: the module
+ will be called leds-powernv.
+
config LEDS_SYSCON
bool "LED support for LEDs on system controllers"
depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index bf46093..480814a 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o
+obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
new file mode 100644
index 0000000..b5a307c
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,463 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
+ * Author: Anshuman Khandual <khandual@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.
+ */
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#include <asm/opal.h>
+
+/*
+ * By default unload path resets all the LEDs. But on PowerNV platform
+ * we want to retain LED state across reboot as these are controlled by
+ * firmware. Also service processor can modify the LEDs independent of
+ * OS. Hence avoid resetting LEDs in unload path.
+ */
+static bool led_disabled;
+
+/* Map LED type to description. */
+struct led_type_map {
+ const int type;
+ const char *desc;
+};
+static const struct led_type_map led_type_map[] = {
+ {OPAL_SLOT_LED_TYPE_ID, POWERNV_LED_TYPE_IDENTIFY},
+ {OPAL_SLOT_LED_TYPE_FAULT, POWERNV_LED_TYPE_FAULT},
+ {OPAL_SLOT_LED_TYPE_ATTN, POWERNV_LED_TYPE_ATTENTION},
+ {-1, NULL},
+};
+
+/*
+ * LED set routines have been implemented as work queue tasks scheduled
+ * on the global work queue. Individual task calls OPAL interface to set
+ * the LED state which might sleep for some time.
+ */
+struct powernv_led_data {
+ struct led_classdev cdev;
+ enum led_brightness value; /* Brightness value */
+ struct mutex lock;
+ struct work_struct work_led; /* LED update workqueue */
+};
+
+struct powernv_leds_priv {
+ int num_leds;
+ struct powernv_led_data powernv_leds[];
+};
+
+
+static inline int sizeof_powernv_leds_priv(int num_leds)
+{
+ return sizeof(struct powernv_leds_priv) +
+ (sizeof(struct powernv_led_data) * num_leds);
+}
+
+/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
+static int powernv_get_led_type(struct led_classdev *led_cdev)
+{
+ char *desc;
+ int i;
+
+ desc = strstr(led_cdev->name, ":");
+ if (!desc)
+ return -1;
+ desc++;
+ if (!desc)
+ return -1;
+
+ for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
+ if (!strcmp(led_type_map[i].desc, desc))
+ return led_type_map[i].type;
+
+ return -1;
+}
+
+/* This function gets LED location code for given LED classdev */
+static char *powernv_get_location_code(struct led_classdev *led_cdev)
+{
+ char *loc_code;
+ char *colon;
+
+ /* Location code of the LED */
+ loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
+ if (!loc_code) {
+ dev_err(led_cdev->dev,
+ "%s: Memory allocation failed\n", __func__);
+ return NULL;
+ }
+
+ colon = strstr(loc_code, ":");
+ if (!colon) {
+ kfree(loc_code);
+ return NULL;
+ }
+
+ *colon = '\0';
+ return loc_code;
+}
+
+/*
+ * This commits the state change of the requested LED through an OPAL call.
+ * This function is called from work queue task context when ever it gets
+ * scheduled. This function can sleep at opal_async_wait_response call.
+ */
+static void powernv_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ char *loc_code;
+ int rc, token, led_type;
+ u64 led_mask, led_value = 0;
+ __be64 max_led_type;
+ struct opal_msg msg;
+
+ led_type = powernv_get_led_type(led_cdev);
+ if (led_type == -1)
+ return;
+
+ loc_code = powernv_get_location_code(led_cdev);
+ if (!loc_code)
+ return;
+
+ /* 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 = led_mask;
+
+ /* OPAL async call */
+ token = opal_async_get_token_interruptible();
+ if (token < 0) {
+ if (token != -ERESTARTSYS)
+ dev_err(led_cdev->dev,
+ "%s: Couldn't get OPAL async token\n",
+ __func__);
+ goto out_loc;
+ }
+
+ rc = opal_leds_set_ind(token, loc_code,
+ led_mask, led_value, &max_led_type);
+ if (rc != OPAL_ASYNC_COMPLETION) {
+ dev_err(led_cdev->dev,
+ "%s: OPAL set LED call failed for %s [rc=%d]\n",
+ __func__, loc_code, rc);
+ goto out_token;
+ }
+
+ rc = opal_async_wait_response(token, &msg);
+ if (rc) {
+ dev_err(led_cdev->dev,
+ "%s: Failed to wait for the async response [rc=%d]\n",
+ __func__, rc);
+ goto out_token;
+ }
+
+ rc = be64_to_cpu(msg.params[1]);
+ if (rc != OPAL_SUCCESS)
+ dev_err(led_cdev->dev,
+ "%s : OAPL async call returned failed [rc=%d]\n",
+ __func__, rc);
+
+out_token:
+ opal_async_release_token(token);
+
+out_loc:
+ kfree(loc_code);
+}
+
+/*
+ * This function fetches the LED state for a given LED type for
+ * mentioned LED classdev structure.
+ */
+static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
+{
+ char *loc_code;
+ int rc, led_type;
+ __be64 led_mask, led_value, max_led_type;
+
+ led_type = powernv_get_led_type(led_cdev);
+ if (led_type == -1)
+ return LED_OFF;
+
+ loc_code = powernv_get_location_code(led_cdev);
+ if (!loc_code)
+ return LED_OFF;
+
+ /* Fetch all LED status */
+ led_mask = cpu_to_be64(0);
+ led_value = cpu_to_be64(0);
+ max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+
+ rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
+ if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
+ dev_err(led_cdev->dev,
+ "%s: OPAL get led call failed [rc=%d]\n",
+ __func__, rc);
+ goto led_fail;
+ }
+
+ led_mask = be64_to_cpu(led_mask);
+ led_value = be64_to_cpu(led_value);
+
+ /* LED status available */
+ if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
+ dev_err(led_cdev->dev,
+ "%s: LED status not available for %s\n",
+ __func__, led_cdev->name);
+ goto led_fail;
+ }
+
+ /* LED status value */
+ if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) {
+ kfree(loc_code);
+ return LED_FULL;
+ }
+
+led_fail:
+ kfree(loc_code);
+ return LED_OFF;
+}
+
+/* Execute LED set task for given led classdev */
+static void powernv_deferred_led_set(struct work_struct *work)
+{
+ struct powernv_led_data *powernv_led =
+ container_of(work, struct powernv_led_data, work_led);
+
+ mutex_lock(&powernv_led->lock);
+ powernv_led_set(&powernv_led->cdev, powernv_led->value);
+ mutex_unlock(&powernv_led->lock);
+}
+
+/*
+ * LED classdev 'brightness_get' function. This schedules work
+ * to update LED state.
+ */
+static void powernv_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct powernv_led_data *powernv_led =
+ container_of(led_cdev, struct powernv_led_data, cdev);
+
+ /* Do not modify LED in unload path */
+ if (led_disabled)
+ return;
+
+ /* Prepare the request */
+ powernv_led->value = value;
+
+ /* Schedule the new task */
+ schedule_work(&powernv_led->work_led);
+}
+
+/* LED classdev 'brightness_get' function */
+static enum led_brightness
+powernv_brightness_get(struct led_classdev *led_cdev)
+{
+ return powernv_led_get(led_cdev);
+}
+
+
+/*
+ * This function registers classdev structure for any given type of LED on
+ * a given child LED device node.
+ */
+static int powernv_led_create(struct device *dev,
+ struct powernv_led_data *powernv_led,
+ const char *led_name, const char *led_type_desc)
+{
+ int rc;
+
+ /* Create the name for classdev */
+ powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s",
+ led_name, led_type_desc);
+ if (!powernv_led->cdev.name) {
+ dev_err(dev,
+ "%s: Memory allocation failed for classdev name\n",
+ __func__);
+ return -ENOMEM;
+ }
+
+ /* Make sure LED type is supported */
+ if (powernv_get_led_type(&powernv_led->cdev) == -1) {
+ kfree(powernv_led->cdev.name);
+ return -EINVAL;
+ }
+
+ powernv_led->cdev.brightness_set = powernv_brightness_set;
+ powernv_led->cdev.brightness_get = powernv_brightness_get;
+ powernv_led->cdev.brightness = LED_OFF;
+ powernv_led->cdev.max_brightness = LED_FULL;
+
+ mutex_init(&powernv_led->lock);
+ INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set);
+
+ /* Register the classdev */
+ rc = led_classdev_register(dev, &powernv_led->cdev);
+ if (rc) {
+ dev_err(dev, "%s: Classdev registration failed for %s\n",
+ __func__, powernv_led->cdev.name);
+ kfree(powernv_led->cdev.name);
+ }
+
+ return rc;
+}
+
+/* Unregister classdev structure for any given LED */
+static void powernv_led_delete(struct powernv_led_data *powernv_led)
+{
+ led_classdev_unregister(&powernv_led->cdev);
+}
+
+/* Go through LED device tree node and register LED classdev structure */
+static int powernv_led_classdev(struct platform_device *pdev,
+ struct device_node *led_node,
+ struct powernv_leds_priv *priv, int num_leds)
+{
+ const char *cur = NULL;
+ int i, rc = -1;
+ struct property *p;
+ struct device_node *np;
+ struct powernv_led_data *powernv_led;
+ struct device *dev = &pdev->dev;
+
+ for_each_child_of_node(led_node, np) {
+ p = of_find_property(np, "led-types", NULL);
+ if (!p)
+ continue;
+
+ while ((cur = of_prop_next_string(p, cur)) != NULL) {
+ powernv_led = &priv->powernv_leds[priv->num_leds++];
+ if (priv->num_leds > num_leds) {
+ rc = -ENOMEM;
+ goto classdev_fail;
+ }
+ rc = powernv_led_create(dev,
+ powernv_led, np->name, cur);
+ if (rc)
+ goto classdev_fail;
+ } /* while end */
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return rc;
+
+classdev_fail:
+ for (i = priv->num_leds - 2; i > 0; i--)
+ powernv_led_delete(&priv->powernv_leds[i]);
+
+ return rc;
+}
+
+/*
+ * We want to populate LED device for each LED type. Hence we
+ * have to calculate count explicitly.
+ */
+static int powernv_leds_count(struct device_node *led_node)
+{
+ const char *cur = NULL;
+ int num_leds = 0;
+ struct property *p;
+ struct device_node *np;
+
+ for_each_child_of_node(led_node, np) {
+ p = of_find_property(np, "led-types", NULL);
+ if (!p)
+ continue;
+
+ while ((cur = of_prop_next_string(p, cur)) != NULL)
+ num_leds++;
+ }
+
+ return num_leds;
+}
+
+/* Platform driver probe */
+static int powernv_led_probe(struct platform_device *pdev)
+{
+ int num_leds;
+ struct device_node *led_node;
+ struct powernv_leds_priv *priv;
+
+ led_node = of_find_node_by_path("/ibm,opal/leds");
+ if (!led_node) {
+ dev_err(&pdev->dev,
+ "%s: LED parent device node not found\n", __func__);
+ return -EINVAL;
+ }
+
+ num_leds = powernv_leds_count(led_node);
+ if (num_leds <= 0) {
+ dev_err(&pdev->dev,
+ "%s: No location code found under LED node\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(&pdev->dev,
+ sizeof_powernv_leds_priv(num_leds),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ return powernv_led_classdev(pdev, led_node, priv, num_leds);
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ int i;
+ struct powernv_led_data *powernv_led;
+ struct powernv_leds_priv *priv;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ priv = platform_get_drvdata(pdev);
+
+ for (i = 0; i < priv->num_leds; i++) {
+ powernv_led = &priv->powernv_leds[i];
+ powernv_led_delete(powernv_led);
+ flush_work(&powernv_led->work_led);
+ }
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\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("Vasant Hegde <hegdevasant@linux.vnet.ibm.com>");
--
2.1.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-01 8:50 ` [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform Vasant Hegde
@ 2015-07-14 9:00 ` Jacek Anaszewski
2015-07-16 6:54 ` Vasant Hegde
0 siblings, 1 reply; 10+ messages in thread
From: Jacek Anaszewski @ 2015-07-14 9:00 UTC (permalink / raw)
To: Vasant Hegde
Cc: linuxppc-dev, linux-leds, benh, stewart, khandual, j.anaszewski81,
cooloney, rpurdie, arnd, mpe
Hi Vasant,
Thanks for the update. I think that we have still room
for improvements, please look at my comments below.
On 07/01/2015 10:50 AM, Vasant Hegde wrote:
> This patch implements LED driver for PowerNV platform using the existing
> generic LED class framework.
>
> PowerNV platform has below type of LEDs:
> - System attention
> Indicates there is a problem with the system that needs attention.
> - Identify
> Helps the user locate/identify a particular FRU or resource in the
> system.
> - Fault
> Indicates there is a problem with the FRU or resource at the
> location with which the indicator is associated.
>
> We register 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.
>
> All the system LEDs can be found in the same regular path /sys/class/leds/.
> We don't use LED colors. We use LED node and led-types property to form
> LED classdev. 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 (0) for any turned off LED.
>
> 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.
>
> 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.
>
> Signed-off-by: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> Acked-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Tested-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
> .../devicetree/bindings/leds/leds-powernv.txt | 24 ++
> drivers/leds/Kconfig | 11 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-powernv.c | 463 +++++++++++++++++++++
> 4 files changed, 499 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
> create mode 100644 drivers/leds/leds-powernv.c
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-powernv.txt b/Documentation/devicetree/bindings/leds/leds-powernv.txt
> new file mode 100644
> index 0000000..636f028
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
> @@ -0,0 +1,24 @@
> +Device Tree binding for LEDs on IBM Power Systems
> +-------------------------------------------------
> +
Please start with:
---------
Required properties:
- compatible : Should be "ibm,opal-v3-led".
Each location code of FRU/Enclosure must be expressed in the
form of a sub-node.
Required properties for the sub nodes:
- led-types : Supported LED types (attention/identify/fault) provided
in the form of string array.
---------
or something of this flavour. The example should be at the end.
> +The 'leds' node under '/ibm,opal' lists service indicators available in the
> +system and their capabilities.
> +
> +leds {
> + compatible = "ibm,opal-v3-led";
> + led-mode = "lightpath";
What about led-mode property? If it is generated by firmware I think,
that this should be mentioned somehow.
> +
> + U78C9.001.RST0027-P1-C1 {
> + led-types = "identify", "fault";
> + };
> + ...
> + ...
> +};
> +
> +Each node under 'leds' node describes location code of FRU/Enclosure.
> +
> +compatible : should be : "ibm,opal-v3-led".
Second colon was redundant here.
> +
> +The properties under each node:
> +
> + led-types : Supported LED types (attention/identify/fault).
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 4191614..4f56c7a 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -505,6 +505,17 @@ config LEDS_BLINKM
> This option enables support for the BlinkM RGB LED connected
> through I2C. Say Y to enable support for the BlinkM LED.
>
> +config LEDS_POWERNV
> + tristate "LED support for PowerNV Platform"
> + depends on LEDS_CLASS
> + depends on PPC_POWERNV
> + depends on OF
> + help
> + This option enables support for the system LEDs present on
> + PowerNV platforms. Say 'y' to enable this support in kernel.
> + To compile this driver as a module, choose 'm' here: the module
> + will be called leds-powernv.
> +
> config LEDS_SYSCON
> bool "LED support for LEDs on system controllers"
> depends on LEDS_CLASS=y
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index bf46093..480814a 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
> obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o
> +obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
> new file mode 100644
> index 0000000..b5a307c
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,463 @@
> +/*
> + * PowerNV LED Driver
> + *
> + * Copyright IBM Corp. 2015
> + *
> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> + * Author: Anshuman Khandual <khandual@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.
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#include <asm/opal.h>
> +
> +/*
> + * By default unload path resets all the LEDs. But on PowerNV platform
> + * we want to retain LED state across reboot as these are controlled by
> + * firmware. Also service processor can modify the LEDs independent of
> + * OS. Hence avoid resetting LEDs in unload path.
> + */
> +static bool led_disabled;
> +
> +/* Map LED type to description. */
> +struct led_type_map {
> + const int type;
> + const char *desc;
> +};
> +static const struct led_type_map led_type_map[] = {
> + {OPAL_SLOT_LED_TYPE_ID, POWERNV_LED_TYPE_IDENTIFY},
> + {OPAL_SLOT_LED_TYPE_FAULT, POWERNV_LED_TYPE_FAULT},
> + {OPAL_SLOT_LED_TYPE_ATTN, POWERNV_LED_TYPE_ATTENTION},
> + {-1, NULL},
> +};
> +
> +/*
> + * LED set routines have been implemented as work queue tasks scheduled
> + * on the global work queue. Individual task calls OPAL interface to set
> + * the LED state which might sleep for some time.
> + */
> +struct powernv_led_data {
> + struct led_classdev cdev;
> + enum led_brightness value; /* Brightness value */
> + struct mutex lock;
> + struct work_struct work_led; /* LED update workqueue */
> +};
> +
> +struct powernv_leds_priv {
> + int num_leds;
> + struct powernv_led_data powernv_leds[];
> +};
> +
> +
> +static inline int sizeof_powernv_leds_priv(int num_leds)
> +{
> + return sizeof(struct powernv_leds_priv) +
> + (sizeof(struct powernv_led_data) * num_leds);
> +}
> +
> +/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
> +static int powernv_get_led_type(struct led_classdev *led_cdev)
> +{
> + char *desc;
> + int i;
> +
> + desc = strstr(led_cdev->name, ":");
> + if (!desc)
> + return -1;
> + desc++;
> + if (!desc)
> + return -1;
> +
> + for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
> + if (!strcmp(led_type_map[i].desc, desc))
> + return led_type_map[i].type;
> +
> + return -1;
> +}
> +
> +/* This function gets LED location code for given LED classdev */
> +static char *powernv_get_location_code(struct led_classdev *led_cdev)
> +{
> + char *loc_code;
> + char *colon;
> +
> + /* Location code of the LED */
> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
> + if (!loc_code) {
> + dev_err(led_cdev->dev,
> + "%s: Memory allocation failed\n", __func__);
> + return NULL;
> + }
> +
> + colon = strstr(loc_code, ":");
> + if (!colon) {
> + kfree(loc_code);
> + return NULL;
> + }
> +
> + *colon = '\0';
> + return loc_code;
> +}
> +
> +/*
> + * This commits the state change of the requested LED through an OPAL call.
> + * This function is called from work queue task context when ever it gets
> + * scheduled. This function can sleep at opal_async_wait_response call.
> + */
> +static void powernv_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + char *loc_code;
> + int rc, token, led_type;
> + u64 led_mask, led_value = 0;
> + __be64 max_led_type;
> + struct opal_msg msg;
> +
> + led_type = powernv_get_led_type(led_cdev);
> + if (led_type == -1)
> + return;
Please parse the led type once upon initialization and add related
property to the struct powernv_led_data that will hold the value.
> + loc_code = powernv_get_location_code(led_cdev);
> + if (!loc_code)
> + return;
The same situation as in case of led type.
> + /* Prepare for the OPAL call */
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
This value could be also calculated only once.
> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
> + if (value)
> + led_value = led_mask;
> +
> + /* OPAL async call */
> + token = opal_async_get_token_interruptible();
> + if (token < 0) {
> + if (token != -ERESTARTSYS)
> + dev_err(led_cdev->dev,
> + "%s: Couldn't get OPAL async token\n",
> + __func__);
> + goto out_loc;
> + }
> +
> + rc = opal_leds_set_ind(token, loc_code,
> + led_mask, led_value, &max_led_type);
> + if (rc != OPAL_ASYNC_COMPLETION) {
> + dev_err(led_cdev->dev,
> + "%s: OPAL set LED call failed for %s [rc=%d]\n",
> + __func__, loc_code, rc);
> + goto out_token;
> + }
> +
> + rc = opal_async_wait_response(token, &msg);
> + if (rc) {
> + dev_err(led_cdev->dev,
> + "%s: Failed to wait for the async response [rc=%d]\n",
> + __func__, rc);
> + goto out_token;
> + }
> +
> + rc = be64_to_cpu(msg.params[1]);
> + if (rc != OPAL_SUCCESS)
> + dev_err(led_cdev->dev,
> + "%s : OAPL async call returned failed [rc=%d]\n",
> + __func__, rc);
> +
> +out_token:
> + opal_async_release_token(token);
> +
> +out_loc:
> + kfree(loc_code);
> +}
> +
> +/*
> + * This function fetches the LED state for a given LED type for
> + * mentioned LED classdev structure.
> + */
> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
> +{
> + char *loc_code;
> + int rc, led_type;
> + __be64 led_mask, led_value, max_led_type;
> +
> + led_type = powernv_get_led_type(led_cdev);
> + if (led_type == -1)
> + return LED_OFF;
> +
> + loc_code = powernv_get_location_code(led_cdev);
> + if (!loc_code)
> + return LED_OFF;
> +
> + /* Fetch all LED status */
> + led_mask = cpu_to_be64(0);
> + led_value = cpu_to_be64(0);
> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> +
> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
> + dev_err(led_cdev->dev,
> + "%s: OPAL get led call failed [rc=%d]\n",
> + __func__, rc);
> + goto led_fail;
> + }
> +
> + led_mask = be64_to_cpu(led_mask);
> + led_value = be64_to_cpu(led_value);
be64_to_cpu result should be assigned to the variable of u64/s64 type.
> + /* LED status available */
> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
> + dev_err(led_cdev->dev,
> + "%s: LED status not available for %s\n",
> + __func__, led_cdev->name);
> + goto led_fail;
> + }
> +
> + /* LED status value */
> + if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) {
> + kfree(loc_code);
> + return LED_FULL;
> + }
> +
> +led_fail:
> + kfree(loc_code);
> + return LED_OFF;
> +}
> +
> +/* Execute LED set task for given led classdev */
> +static void powernv_deferred_led_set(struct work_struct *work)
> +{
> + struct powernv_led_data *powernv_led =
> + container_of(work, struct powernv_led_data, work_led);
> +
> + mutex_lock(&powernv_led->lock);
> + powernv_led_set(&powernv_led->cdev, powernv_led->value);
> + mutex_unlock(&powernv_led->lock);
> +}
> +
> +/*
> + * LED classdev 'brightness_get' function. This schedules work
> + * to update LED state.
> + */
> +static void powernv_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct powernv_led_data *powernv_led =
> + container_of(led_cdev, struct powernv_led_data, cdev);
> +
> + /* Do not modify LED in unload path */
> + if (led_disabled)
> + return;
> +
> + /* Prepare the request */
> + powernv_led->value = value;
> +
> + /* Schedule the new task */
> + schedule_work(&powernv_led->work_led);
> +}
> +
> +/* LED classdev 'brightness_get' function */
> +static enum led_brightness
> +powernv_brightness_get(struct led_classdev *led_cdev)
> +{
> + return powernv_led_get(led_cdev);
> +}
> +
> +
> +/*
> + * This function registers classdev structure for any given type of LED on
> + * a given child LED device node.
> + */
> +static int powernv_led_create(struct device *dev,
> + struct powernv_led_data *powernv_led,
> + const char *led_name, const char *led_type_desc)
> +{
> + int rc;
> +
> + /* Create the name for classdev */
> + powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s",
> + led_name, led_type_desc);
Please use devm_kasprintf.
> + if (!powernv_led->cdev.name) {
> + dev_err(dev,
> + "%s: Memory allocation failed for classdev name\n",
> + __func__);
> + return -ENOMEM;
> + }
> +
> + /* Make sure LED type is supported */
> + if (powernv_get_led_type(&powernv_led->cdev) == -1) {
> + kfree(powernv_led->cdev.name);
> + return -EINVAL;
> + }
> +
> + powernv_led->cdev.brightness_set = powernv_brightness_set;
> + powernv_led->cdev.brightness_get = powernv_brightness_get;
> + powernv_led->cdev.brightness = LED_OFF;
> + powernv_led->cdev.max_brightness = LED_FULL;
> +
> + mutex_init(&powernv_led->lock);
> + INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set);
> +
> + /* Register the classdev */
> + rc = led_classdev_register(dev, &powernv_led->cdev);
devm_led_classdev_register is also available.
> + if (rc) {
> + dev_err(dev, "%s: Classdev registration failed for %s\n",
> + __func__, powernv_led->cdev.name);
> + kfree(powernv_led->cdev.name);
> + }
> +
> + return rc;
> +}
> +
> +/* Unregister classdev structure for any given LED */
> +static void powernv_led_delete(struct powernv_led_data *powernv_led)
> +{
> + led_classdev_unregister(&powernv_led->cdev);
> +}
This function is redundant.
> +/* Go through LED device tree node and register LED classdev structure */
> +static int powernv_led_classdev(struct platform_device *pdev,
> + struct device_node *led_node,
> + struct powernv_leds_priv *priv, int num_leds)
> +{
> + const char *cur = NULL;
> + int i, rc = -1;
> + struct property *p;
> + struct device_node *np;
> + struct powernv_led_data *powernv_led;
> + struct device *dev = &pdev->dev;
> +
> + for_each_child_of_node(led_node, np) {
> + p = of_find_property(np, "led-types", NULL);
> + if (!p)
> + continue;
> +
> + while ((cur = of_prop_next_string(p, cur)) != NULL) {
> + powernv_led = &priv->powernv_leds[priv->num_leds++];
> + if (priv->num_leds > num_leds) {
> + rc = -ENOMEM;
> + goto classdev_fail;
> + }
> + rc = powernv_led_create(dev,
> + powernv_led, np->name, cur);
> + if (rc)
> + goto classdev_fail;
> + } /* while end */
> + }
> +
> + platform_set_drvdata(pdev, priv);
> + return rc;
> +
> +classdev_fail:
> + for (i = priv->num_leds - 2; i > 0; i--)
Why do you leave element with id == 0 unreleased?
> + powernv_led_delete(&priv->powernv_leds[i]);
> +
> + return rc;
> +}
> +
> +/*
> + * We want to populate LED device for each LED type. Hence we
> + * have to calculate count explicitly.
> + */
> +static int powernv_leds_count(struct device_node *led_node)
> +{
> + const char *cur = NULL;
> + int num_leds = 0;
> + struct property *p;
> + struct device_node *np;
> +
> + for_each_child_of_node(led_node, np) {
> + p = of_find_property(np, "led-types", NULL);
> + if (!p)
> + continue;
> +
> + while ((cur = of_prop_next_string(p, cur)) != NULL)
> + num_leds++;
> + }
> +
> + return num_leds;
> +}
Does it mean that if the node exists but does't have led-types
property we are not going to register it? I assume that this is
firmware which generates the nodes, otherwise it would make
no sense to have the node, am I right?
> +/* Platform driver probe */
> +static int powernv_led_probe(struct platform_device *pdev)
> +{
> + int num_leds;
> + struct device_node *led_node;
> + struct powernv_leds_priv *priv;
> +
> + led_node = of_find_node_by_path("/ibm,opal/leds");
> + if (!led_node) {
> + dev_err(&pdev->dev,
> + "%s: LED parent device node not found\n", __func__);
> + return -EINVAL;
> + }
> +
> + num_leds = powernv_leds_count(led_node);
> + if (num_leds <= 0) {
> + dev_err(&pdev->dev,
> + "%s: No location code found under LED node\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + priv = devm_kzalloc(&pdev->dev,
> + sizeof_powernv_leds_priv(num_leds),
> + GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + return powernv_led_classdev(pdev, led_node, priv, num_leds);
> +}
> +
> +/* Platform driver remove */
> +static int powernv_led_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct powernv_led_data *powernv_led;
> + struct powernv_leds_priv *priv;
> +
> + /* Disable LED operation */
> + led_disabled = true;
> +
> + priv = platform_get_drvdata(pdev);
> +
> + for (i = 0; i < priv->num_leds; i++) {
> + powernv_led = &priv->powernv_leds[i];
> + powernv_led_delete(powernv_led);
> + flush_work(&powernv_led->work_led);
> + }
You are missing mutex_destroy here.
> +
> + dev_info(&pdev->dev, "PowerNV led module unregistered\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("Vasant Hegde <hegdevasant@linux.vnet.ibm.com>");
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-14 9:00 ` Jacek Anaszewski
@ 2015-07-16 6:54 ` Vasant Hegde
2015-07-16 8:27 ` Jacek Anaszewski
0 siblings, 1 reply; 10+ messages in thread
From: Vasant Hegde @ 2015-07-16 6:54 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, arnd, j.anaszewski81, cooloney, rpurdie, linuxppc-dev,
linux-leds, khandual
On 07/14/2015 02:30 PM, Jacek Anaszewski wrote:
> Hi Vasant,
Jacek,
>
> Thanks for the update. I think that we have still room
> for improvements, please look at my comments below.
Thanks for the detailed review.
.../...
>> @@ -0,0 +1,24 @@
>> +Device Tree binding for LEDs on IBM Power Systems
>> +-------------------------------------------------
>> +
>
> Please start with:
>
> ---------
>
> Required properties:
> - compatible : Should be "ibm,opal-v3-led".
>
> Each location code of FRU/Enclosure must be expressed in the
> form of a sub-node.
>
> Required properties for the sub nodes:
> - led-types : Supported LED types (attention/identify/fault) provided
> in the form of string array.
>
> ---------
>
> or something of this flavour. The example should be at the end.
>
Fixed.
>
>
>> +The 'leds' node under '/ibm,opal' lists service indicators available in the
>> +system and their capabilities.
>> +
>> +leds {
>> + compatible = "ibm,opal-v3-led";
>> + led-mode = "lightpath";
>
> What about led-mode property? If it is generated by firmware I think,
> that this should be mentioned somehow.
Yes.. Its generated by firmware. Added this property to documentation file.
>
>> +
>> + U78C9.001.RST0027-P1-C1 {
>> + led-types = "identify", "fault";
>> + };
>> + ...
>> + ...
>> +};
>> +
>> +Each node under 'leds' node describes location code of FRU/Enclosure.
>> +
>> +compatible : should be : "ibm,opal-v3-led".
>
> Second colon was redundant here.
>
I have added as
- compatible : "ibm,opal-v3-led".
>> +
>> +The properties under each node:
>> +
>> + led-types : Supported LED types (attention/identify/fault).
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 4191614..4f56c7a 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -505,6 +505,17 @@ config LEDS_BLINKM
>> This option enables support for the BlinkM RGB LED connected
>> through I2C. Say Y to enable support for the BlinkM LED.
>>
>> +config LEDS_POWERNV
>> + tristate "LED support for PowerNV Platform"
>> + depends on LEDS_CLASS
>> + depends on PPC_POWERNV
>> + depends on OF
>> + help
>> + This option enables support for the system LEDs present on
>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>> + To compile this driver as a module, choose 'm' here: the module
>> + will be called leds-powernv.
>> +
>> config LEDS_SYSCON
>> bool "LED support for LEDs on system controllers"
>> depends on LEDS_CLASS=y
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index bf46093..480814a 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
>> obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o
>> +obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
>>
>> # LED SPI Drivers
>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
>> new file mode 100644
>> index 0000000..b5a307c
>> --- /dev/null
>> +++ b/drivers/leds/leds-powernv.c
>> @@ -0,0 +1,463 @@
>> +/*
>> + * PowerNV LED Driver
>> + *
>> + * Copyright IBM Corp. 2015
>> + *
>> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>> + * Author: Anshuman Khandual <khandual@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.
>> + */
>> +
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +
>> +#include <asm/opal.h>
>> +
>> +/*
>> + * By default unload path resets all the LEDs. But on PowerNV platform
>> + * we want to retain LED state across reboot as these are controlled by
>> + * firmware. Also service processor can modify the LEDs independent of
>> + * OS. Hence avoid resetting LEDs in unload path.
>> + */
>> +static bool led_disabled;
>> +
>> +/* Map LED type to description. */
>> +struct led_type_map {
>> + const int type;
>> + const char *desc;
>> +};
>> +static const struct led_type_map led_type_map[] = {
>> + {OPAL_SLOT_LED_TYPE_ID, POWERNV_LED_TYPE_IDENTIFY},
>> + {OPAL_SLOT_LED_TYPE_FAULT, POWERNV_LED_TYPE_FAULT},
>> + {OPAL_SLOT_LED_TYPE_ATTN, POWERNV_LED_TYPE_ATTENTION},
>> + {-1, NULL},
>> +};
>> +
>> +/*
>> + * LED set routines have been implemented as work queue tasks scheduled
>> + * on the global work queue. Individual task calls OPAL interface to set
>> + * the LED state which might sleep for some time.
>> + */
>> +struct powernv_led_data {
>> + struct led_classdev cdev;
>> + enum led_brightness value; /* Brightness value */
>> + struct mutex lock;
>> + struct work_struct work_led; /* LED update workqueue */
>> +};
>> +
>> +struct powernv_leds_priv {
>> + int num_leds;
>> + struct powernv_led_data powernv_leds[];
>> +};
>> +
>> +
>> +static inline int sizeof_powernv_leds_priv(int num_leds)
>> +{
>> + return sizeof(struct powernv_leds_priv) +
>> + (sizeof(struct powernv_led_data) * num_leds);
>> +}
>> +
>> +/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
>> +static int powernv_get_led_type(struct led_classdev *led_cdev)
>> +{
>> + char *desc;
>> + int i;
>> +
>> + desc = strstr(led_cdev->name, ":");
>> + if (!desc)
>> + return -1;
>> + desc++;
>> + if (!desc)
>> + return -1;
>> +
>> + for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>> + if (!strcmp(led_type_map[i].desc, desc))
>> + return led_type_map[i].type;
>> +
>> + return -1;
>> +}
>> +
>> +/* This function gets LED location code for given LED classdev */
>> +static char *powernv_get_location_code(struct led_classdev *led_cdev)
>> +{
>> + char *loc_code;
>> + char *colon;
>> +
>> + /* Location code of the LED */
>> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
>> + if (!loc_code) {
>> + dev_err(led_cdev->dev,
>> + "%s: Memory allocation failed\n", __func__);
>> + return NULL;
>> + }
>> +
>> + colon = strstr(loc_code, ":");
>> + if (!colon) {
>> + kfree(loc_code);
>> + return NULL;
>> + }
>> +
>> + *colon = '\0';
>> + return loc_code;
>> +}
>> +
>> +/*
>> + * This commits the state change of the requested LED through an OPAL call.
>> + * This function is called from work queue task context when ever it gets
>> + * scheduled. This function can sleep at opal_async_wait_response call.
>> + */
>> +static void powernv_led_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + char *loc_code;
>> + int rc, token, led_type;
>> + u64 led_mask, led_value = 0;
>> + __be64 max_led_type;
>> + struct opal_msg msg;
>> +
>> + led_type = powernv_get_led_type(led_cdev);
>> + if (led_type == -1)
>> + return;
>
> Please parse the led type once upon initialization and add related
> property to the struct powernv_led_data that will hold the value.
I thought we can get location code and type using class dev name itself. Hence I
didn't add these two properties to structure..
Do you want me to add them to structure itself?
>
>> + loc_code = powernv_get_location_code(led_cdev);
>> + if (!loc_code)
>> + return;
>
> The same situation as in case of led type.
>
>> + /* Prepare for the OPAL call */
>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>
> This value could be also calculated only once.
Yeah. May be I can move this to powernv_leds_priv structure.
>
>> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
>> + if (value)
>> + led_value = led_mask;
>> +
>> + /* OPAL async call */
>> + token = opal_async_get_token_interruptible();
>> + if (token < 0) {
>> + if (token != -ERESTARTSYS)
>> + dev_err(led_cdev->dev,
>> + "%s: Couldn't get OPAL async token\n",
>> + __func__);
>> + goto out_loc;
>> + }
>> +
>> + rc = opal_leds_set_ind(token, loc_code,
>> + led_mask, led_value, &max_led_type);
>> + if (rc != OPAL_ASYNC_COMPLETION) {
>> + dev_err(led_cdev->dev,
>> + "%s: OPAL set LED call failed for %s [rc=%d]\n",
>> + __func__, loc_code, rc);
>> + goto out_token;
>> + }
>> +
>> + rc = opal_async_wait_response(token, &msg);
>> + if (rc) {
>> + dev_err(led_cdev->dev,
>> + "%s: Failed to wait for the async response [rc=%d]\n",
>> + __func__, rc);
>> + goto out_token;
>> + }
>> +
>> + rc = be64_to_cpu(msg.params[1]);
>> + if (rc != OPAL_SUCCESS)
>> + dev_err(led_cdev->dev,
>> + "%s : OAPL async call returned failed [rc=%d]\n",
>> + __func__, rc);
>> +
>> +out_token:
>> + opal_async_release_token(token);
>> +
>> +out_loc:
>> + kfree(loc_code);
>> +}
>> +
>> +/*
>> + * This function fetches the LED state for a given LED type for
>> + * mentioned LED classdev structure.
>> + */
>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
>> +{
>> + char *loc_code;
>> + int rc, led_type;
>> + __be64 led_mask, led_value, max_led_type;
>> +
>> + led_type = powernv_get_led_type(led_cdev);
>> + if (led_type == -1)
>> + return LED_OFF;
>> +
>> + loc_code = powernv_get_location_code(led_cdev);
>> + if (!loc_code)
>> + return LED_OFF;
>> +
>> + /* Fetch all LED status */
>> + led_mask = cpu_to_be64(0);
>> + led_value = cpu_to_be64(0);
>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>> +
>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>> + dev_err(led_cdev->dev,
>> + "%s: OPAL get led call failed [rc=%d]\n",
>> + __func__, rc);
>> + goto led_fail;
>> + }
>> +
>> + led_mask = be64_to_cpu(led_mask);
>> + led_value = be64_to_cpu(led_value);
>
> be64_to_cpu result should be assigned to the variable of u64/s64 type.
PowerNV platform is capable of running both big/little endian mode.. But
presently our firmware is big endian. These variable contains big endian values.
Hence I have created as __be64 .. (This is the convention we follow in other
places as well).
>
>> + /* LED status available */
>> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
>> + dev_err(led_cdev->dev,
>> + "%s: LED status not available for %s\n",
>> + __func__, led_cdev->name);
>> + goto led_fail;
>> + }
>> +
>> + /* LED status value */
>> + if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) {
>> + kfree(loc_code);
>> + return LED_FULL;
>> + }
>> +
>> +led_fail:
>> + kfree(loc_code);
>> + return LED_OFF;
>> +}
>> +
>> +/* Execute LED set task for given led classdev */
>> +static void powernv_deferred_led_set(struct work_struct *work)
>> +{
>> + struct powernv_led_data *powernv_led =
>> + container_of(work, struct powernv_led_data, work_led);
>> +
>> + mutex_lock(&powernv_led->lock);
>> + powernv_led_set(&powernv_led->cdev, powernv_led->value);
>> + mutex_unlock(&powernv_led->lock);
>> +}
>> +
>> +/*
>> + * LED classdev 'brightness_get' function. This schedules work
>> + * to update LED state.
>> + */
>> +static void powernv_brightness_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + struct powernv_led_data *powernv_led =
>> + container_of(led_cdev, struct powernv_led_data, cdev);
>> +
>> + /* Do not modify LED in unload path */
>> + if (led_disabled)
>> + return;
>> +
>> + /* Prepare the request */
>> + powernv_led->value = value;
>> +
>> + /* Schedule the new task */
>> + schedule_work(&powernv_led->work_led);
>> +}
>> +
>> +/* LED classdev 'brightness_get' function */
>> +static enum led_brightness
>> +powernv_brightness_get(struct led_classdev *led_cdev)
>> +{
>> + return powernv_led_get(led_cdev);
>> +}
>> +
>> +
>> +/*
>> + * This function registers classdev structure for any given type of LED on
>> + * a given child LED device node.
>> + */
>> +static int powernv_led_create(struct device *dev,
>> + struct powernv_led_data *powernv_led,
>> + const char *led_name, const char *led_type_desc)
>> +{
>> + int rc;
>> +
>> + /* Create the name for classdev */
>> + powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s",
>> + led_name, led_type_desc);
>
> Please use devm_kasprintf.
Done.
>
>> + if (!powernv_led->cdev.name) {
>> + dev_err(dev,
>> + "%s: Memory allocation failed for classdev name\n",
>> + __func__);
>> + return -ENOMEM;
>> + }
>> +
>> + /* Make sure LED type is supported */
>> + if (powernv_get_led_type(&powernv_led->cdev) == -1) {
>> + kfree(powernv_led->cdev.name);
>> + return -EINVAL;
>> + }
>> +
>> + powernv_led->cdev.brightness_set = powernv_brightness_set;
>> + powernv_led->cdev.brightness_get = powernv_brightness_get;
>> + powernv_led->cdev.brightness = LED_OFF;
>> + powernv_led->cdev.max_brightness = LED_FULL;
>> +
>> + mutex_init(&powernv_led->lock);
>> + INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set);
>> +
>> + /* Register the classdev */
>> + rc = led_classdev_register(dev, &powernv_led->cdev);
>
> devm_led_classdev_register is also available.
Looks like most of the existing drivers are using led_classdev_register function..
Which one is preferred here?
>
>> + if (rc) {
>> + dev_err(dev, "%s: Classdev registration failed for %s\n",
>> + __func__, powernv_led->cdev.name);
>> + kfree(powernv_led->cdev.name);
>> + }
>> +
>> + return rc;
>> +}
>> +
>> +/* Unregister classdev structure for any given LED */
>> +static void powernv_led_delete(struct powernv_led_data *powernv_led)
>> +{
>> + led_classdev_unregister(&powernv_led->cdev);
>> +}
>
> This function is redundant.
Like powernv_led_create, I just added this function ... hoping this will improve
code readability.
Will remove this function.
>
>> +/* Go through LED device tree node and register LED classdev structure */
>> +static int powernv_led_classdev(struct platform_device *pdev,
>> + struct device_node *led_node,
>> + struct powernv_leds_priv *priv, int num_leds)
>> +{
>> + const char *cur = NULL;
>> + int i, rc = -1;
>> + struct property *p;
>> + struct device_node *np;
>> + struct powernv_led_data *powernv_led;
>> + struct device *dev = &pdev->dev;
>> +
>> + for_each_child_of_node(led_node, np) {
>> + p = of_find_property(np, "led-types", NULL);
>> + if (!p)
>> + continue;
>> +
>> + while ((cur = of_prop_next_string(p, cur)) != NULL) {
>> + powernv_led = &priv->powernv_leds[priv->num_leds++];
>> + if (priv->num_leds > num_leds) {
>> + rc = -ENOMEM;
>> + goto classdev_fail;
>> + }
>> + rc = powernv_led_create(dev,
>> + powernv_led, np->name, cur);
>> + if (rc)
>> + goto classdev_fail;
>> + } /* while end */
>> + }
>> +
>> + platform_set_drvdata(pdev, priv);
>> + return rc;
>> +
>> +classdev_fail:
>> + for (i = priv->num_leds - 2; i > 0; i--)
>
> Why do you leave element with id == 0 unreleased?
It was my mistake. Fixed.
>
>> + powernv_led_delete(&priv->powernv_leds[i]);
>> +
>> + return rc;
>> +}
>> +
>> +/*
>> + * We want to populate LED device for each LED type. Hence we
>> + * have to calculate count explicitly.
>> + */
>> +static int powernv_leds_count(struct device_node *led_node)
>> +{
>> + const char *cur = NULL;
>> + int num_leds = 0;
>> + struct property *p;
>> + struct device_node *np;
>> +
>> + for_each_child_of_node(led_node, np) {
>> + p = of_find_property(np, "led-types", NULL);
>> + if (!p)
>> + continue;
>> +
>> + while ((cur = of_prop_next_string(p, cur)) != NULL)
>> + num_leds++;
>> + }
>> +
>> + return num_leds;
>> +}
>
> Does it mean that if the node exists but does't have led-types
> property we are not going to register it?
Yes.. No point in registering location code ..which doesn't have led-types
property.
> I assume that this is
> firmware which generates the nodes, otherwise it would make
> no sense to have the node, am I right?
That correct. Our firmware generates this property.
>
>> +/* Platform driver probe */
>> +static int powernv_led_probe(struct platform_device *pdev)
>> +{
>> + int num_leds;
>> + struct device_node *led_node;
>> + struct powernv_leds_priv *priv;
>> +
>> + led_node = of_find_node_by_path("/ibm,opal/leds");
>> + if (!led_node) {
>> + dev_err(&pdev->dev,
>> + "%s: LED parent device node not found\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + num_leds = powernv_leds_count(led_node);
>> + if (num_leds <= 0) {
>> + dev_err(&pdev->dev,
>> + "%s: No location code found under LED node\n",
>> + __func__);
>> + return -EINVAL;
>> + }
>> +
>> + priv = devm_kzalloc(&pdev->dev,
>> + sizeof_powernv_leds_priv(num_leds),
>> + GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + return powernv_led_classdev(pdev, led_node, priv, num_leds);
>> +}
>> +
>> +/* Platform driver remove */
>> +static int powernv_led_remove(struct platform_device *pdev)
>> +{
>> + int i;
>> + struct powernv_led_data *powernv_led;
>> + struct powernv_leds_priv *priv;
>> +
>> + /* Disable LED operation */
>> + led_disabled = true;
>> +
>> + priv = platform_get_drvdata(pdev);
>> +
>> + for (i = 0; i < priv->num_leds; i++) {
>> + powernv_led = &priv->powernv_leds[i];
>> + powernv_led_delete(powernv_led);
>> + flush_work(&powernv_led->work_led);
>> + }
>
> You are missing mutex_destroy here.
Oh yeah.. I missed it. Fixed.
-Vasant
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-16 6:54 ` Vasant Hegde
@ 2015-07-16 8:27 ` Jacek Anaszewski
2015-07-16 8:47 ` Michael Ellerman
2015-07-17 5:52 ` Vasant Hegde
0 siblings, 2 replies; 10+ messages in thread
From: Jacek Anaszewski @ 2015-07-16 8:27 UTC (permalink / raw)
To: Vasant Hegde
Cc: stewart, arnd, j.anaszewski81, cooloney, rpurdie, linuxppc-dev,
linux-leds, khandual
Hi Vasan,
On 07/16/2015 08:54 AM, Vasant Hegde wrote:
> On 07/14/2015 02:30 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>
> Jacek,
>
>>
>> Thanks for the update. I think that we have still room
>> for improvements, please look at my comments below.
>
> Thanks for the detailed review.
You're welcome.
> .../...
>
>>> @@ -0,0 +1,24 @@
>>> +Device Tree binding for LEDs on IBM Power Systems
>>> +-------------------------------------------------
>>> +
>>
>> Please start with:
>>
>> ---------
>>
>> Required properties:
>> - compatible : Should be "ibm,opal-v3-led".
>>
>> Each location code of FRU/Enclosure must be expressed in the
>> form of a sub-node.
>>
>> Required properties for the sub nodes:
>> - led-types : Supported LED types (attention/identify/fault) provided
>> in the form of string array.
>>
>> ---------
>>
>> or something of this flavour. The example should be at the end.
>>
>
> Fixed.
>
>>
>>
>>> +The 'leds' node under '/ibm,opal' lists service indicators available in the
>>> +system and their capabilities.
>>> +
>>> +leds {
>>> + compatible = "ibm,opal-v3-led";
>>> + led-mode = "lightpath";
>>
>> What about led-mode property? If it is generated by firmware I think,
>> that this should be mentioned somehow.
>
> Yes.. Its generated by firmware. Added this property to documentation file.
>
>>
>>> +
>>> + U78C9.001.RST0027-P1-C1 {
>>> + led-types = "identify", "fault";
>>> + };
>>> + ...
>>> + ...
>>> +};
>>> +
>>> +Each node under 'leds' node describes location code of FRU/Enclosure.
>>> +
>>> +compatible : should be : "ibm,opal-v3-led".
>>
>> Second colon was redundant here.
>>
>
> I have added as
> - compatible : "ibm,opal-v3-led".
Please retain "Should be :".
>
>>> +
>>> +The properties under each node:
>>> +
>>> + led-types : Supported LED types (attention/identify/fault).
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 4191614..4f56c7a 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -505,6 +505,17 @@ config LEDS_BLINKM
>>> This option enables support for the BlinkM RGB LED connected
>>> through I2C. Say Y to enable support for the BlinkM LED.
>>>
>>> +config LEDS_POWERNV
>>> + tristate "LED support for PowerNV Platform"
>>> + depends on LEDS_CLASS
>>> + depends on PPC_POWERNV
>>> + depends on OF
>>> + help
>>> + This option enables support for the system LEDs present on
>>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>>> + To compile this driver as a module, choose 'm' here: the module
>>> + will be called leds-powernv.
>>> +
>>> config LEDS_SYSCON
>>> bool "LED support for LEDs on system controllers"
>>> depends on LEDS_CLASS=y
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index bf46093..480814a 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
>>> obj-$(CONFIG_LEDS_PM8941_WLED) += leds-pm8941-wled.o
>>> +obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
>>>
>>> # LED SPI Drivers
>>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>>> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
>>> new file mode 100644
>>> index 0000000..b5a307c
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-powernv.c
>>> @@ -0,0 +1,463 @@
>>> +/*
>>> + * PowerNV LED Driver
>>> + *
>>> + * Copyright IBM Corp. 2015
>>> + *
>>> + * Author: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
>>> + * Author: Anshuman Khandual <khandual@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.
>>> + */
>>> +
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/types.h>
>>> +
>>> +#include <asm/opal.h>
>>> +
>>> +/*
>>> + * By default unload path resets all the LEDs. But on PowerNV platform
>>> + * we want to retain LED state across reboot as these are controlled by
>>> + * firmware. Also service processor can modify the LEDs independent of
>>> + * OS. Hence avoid resetting LEDs in unload path.
>>> + */
>>> +static bool led_disabled;
>>> +
>>> +/* Map LED type to description. */
>>> +struct led_type_map {
>>> + const int type;
>>> + const char *desc;
>>> +};
>>> +static const struct led_type_map led_type_map[] = {
>>> + {OPAL_SLOT_LED_TYPE_ID, POWERNV_LED_TYPE_IDENTIFY},
>>> + {OPAL_SLOT_LED_TYPE_FAULT, POWERNV_LED_TYPE_FAULT},
>>> + {OPAL_SLOT_LED_TYPE_ATTN, POWERNV_LED_TYPE_ATTENTION},
>>> + {-1, NULL},
>>> +};
>>> +
>>> +/*
>>> + * LED set routines have been implemented as work queue tasks scheduled
>>> + * on the global work queue. Individual task calls OPAL interface to set
>>> + * the LED state which might sleep for some time.
>>> + */
>>> +struct powernv_led_data {
>>> + struct led_classdev cdev;
>>> + enum led_brightness value; /* Brightness value */
>>> + struct mutex lock;
>>> + struct work_struct work_led; /* LED update workqueue */
>>> +};
>>> +
>>> +struct powernv_leds_priv {
>>> + int num_leds;
>>> + struct powernv_led_data powernv_leds[];
>>> +};
>>> +
>>> +
>>> +static inline int sizeof_powernv_leds_priv(int num_leds)
>>> +{
>>> + return sizeof(struct powernv_leds_priv) +
>>> + (sizeof(struct powernv_led_data) * num_leds);
>>> +}
>>> +
>>> +/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
>>> +static int powernv_get_led_type(struct led_classdev *led_cdev)
>>> +{
>>> + char *desc;
>>> + int i;
>>> +
>>> + desc = strstr(led_cdev->name, ":");
>>> + if (!desc)
>>> + return -1;
>>> + desc++;
>>> + if (!desc)
>>> + return -1;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>>> + if (!strcmp(led_type_map[i].desc, desc))
>>> + return led_type_map[i].type;
>>> +
>>> + return -1;
>>> +}
>>> +
>>> +/* This function gets LED location code for given LED classdev */
>>> +static char *powernv_get_location_code(struct led_classdev *led_cdev)
>>> +{
>>> + char *loc_code;
>>> + char *colon;
>>> +
>>> + /* Location code of the LED */
>>> + loc_code = kasprintf(GFP_KERNEL, "%s", led_cdev->name);
>>> + if (!loc_code) {
>>> + dev_err(led_cdev->dev,
>>> + "%s: Memory allocation failed\n", __func__);
>>> + return NULL;
>>> + }
>>> +
>>> + colon = strstr(loc_code, ":");
>>> + if (!colon) {
>>> + kfree(loc_code);
>>> + return NULL;
>>> + }
>>> +
>>> + *colon = '\0';
>>> + return loc_code;
>>> +}
>>> +
>>> +/*
>>> + * This commits the state change of the requested LED through an OPAL call.
>>> + * This function is called from work queue task context when ever it gets
>>> + * scheduled. This function can sleep at opal_async_wait_response call.
>>> + */
>>> +static void powernv_led_set(struct led_classdev *led_cdev,
>>> + enum led_brightness value)
>>> +{
>>> + char *loc_code;
>>> + int rc, token, led_type;
>>> + u64 led_mask, led_value = 0;
>>> + __be64 max_led_type;
>>> + struct opal_msg msg;
>>> +
>>> + led_type = powernv_get_led_type(led_cdev);
>>> + if (led_type == -1)
>>> + return;
>>
>> Please parse the led type once upon initialization and add related
>> property to the struct powernv_led_data that will hold the value.
>
> I thought we can get location code and type using class dev name itself. Hence I
> didn't add these two properties to structure..
This way you are doing extra work for parsing the name each time
the brightness is set.
> Do you want me to add them to structure itself?
Yes, please add them.
>>
>>> + loc_code = powernv_get_location_code(led_cdev);
>>> + if (!loc_code)
>>> + return;
>>
>> The same situation as in case of led type.
>>
>>> + /* Prepare for the OPAL call */
>>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>
>> This value could be also calculated only once.
>
> Yeah. May be I can move this to powernv_leds_priv structure.
>
>>
>>> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
>>> + if (value)
>>> + led_value = led_mask;
>>> +
>>> + /* OPAL async call */
>>> + token = opal_async_get_token_interruptible();
>>> + if (token < 0) {
>>> + if (token != -ERESTARTSYS)
>>> + dev_err(led_cdev->dev,
>>> + "%s: Couldn't get OPAL async token\n",
>>> + __func__);
>>> + goto out_loc;
>>> + }
>>> +
>>> + rc = opal_leds_set_ind(token, loc_code,
>>> + led_mask, led_value, &max_led_type);
>>> + if (rc != OPAL_ASYNC_COMPLETION) {
>>> + dev_err(led_cdev->dev,
>>> + "%s: OPAL set LED call failed for %s [rc=%d]\n",
>>> + __func__, loc_code, rc);
>>> + goto out_token;
>>> + }
>>> +
>>> + rc = opal_async_wait_response(token, &msg);
>>> + if (rc) {
>>> + dev_err(led_cdev->dev,
>>> + "%s: Failed to wait for the async response [rc=%d]\n",
>>> + __func__, rc);
>>> + goto out_token;
>>> + }
>>> +
>>> + rc = be64_to_cpu(msg.params[1]);
>>> + if (rc != OPAL_SUCCESS)
>>> + dev_err(led_cdev->dev,
>>> + "%s : OAPL async call returned failed [rc=%d]\n",
>>> + __func__, rc);
>>> +
>>> +out_token:
>>> + opal_async_release_token(token);
>>> +
>>> +out_loc:
>>> + kfree(loc_code);
>>> +}
>>> +
>>> +/*
>>> + * This function fetches the LED state for a given LED type for
>>> + * mentioned LED classdev structure.
>>> + */
>>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
>>> +{
>>> + char *loc_code;
>>> + int rc, led_type;
>>> + __be64 led_mask, led_value, max_led_type;
>>> +
>>> + led_type = powernv_get_led_type(led_cdev);
>>> + if (led_type == -1)
>>> + return LED_OFF;
>>> +
>>> + loc_code = powernv_get_location_code(led_cdev);
>>> + if (!loc_code)
>>> + return LED_OFF;
>>> +
>>> + /* Fetch all LED status */
>>> + led_mask = cpu_to_be64(0);
>>> + led_value = cpu_to_be64(0);
>>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>> +
>>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
>>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>> + dev_err(led_cdev->dev,
>>> + "%s: OPAL get led call failed [rc=%d]\n",
>>> + __func__, rc);
>>> + goto led_fail;
>>> + }
>>> +
>>> + led_mask = be64_to_cpu(led_mask);
>>> + led_value = be64_to_cpu(led_value);
>>
>> be64_to_cpu result should be assigned to the variable of u64/s64 type.
>
> PowerNV platform is capable of running both big/little endian mode.. But
> presently our firmware is big endian. These variable contains big endian values.
> Hence I have created as __be64 .. (This is the convention we follow in other
> places as well).
It is correct that the argument is of __be64 type, but be64_to_cpu
returns u64 type, whereas you assign it to __be64.
>>
>>> + /* LED status available */
>>> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
>>> + dev_err(led_cdev->dev,
>>> + "%s: LED status not available for %s\n",
>>> + __func__, led_cdev->name);
>>> + goto led_fail;
>>> + }
>>> +
>>> + /* LED status value */
>>> + if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) {
>>> + kfree(loc_code);
>>> + return LED_FULL;
>>> + }
>>> +
>>> +led_fail:
>>> + kfree(loc_code);
>>> + return LED_OFF;
>>> +}
>>> +
>>> +/* Execute LED set task for given led classdev */
>>> +static void powernv_deferred_led_set(struct work_struct *work)
>>> +{
>>> + struct powernv_led_data *powernv_led =
>>> + container_of(work, struct powernv_led_data, work_led);
>>> +
>>> + mutex_lock(&powernv_led->lock);
>>> + powernv_led_set(&powernv_led->cdev, powernv_led->value);
>>> + mutex_unlock(&powernv_led->lock);
>>> +}
>>> +
>>> +/*
>>> + * LED classdev 'brightness_get' function. This schedules work
>>> + * to update LED state.
>>> + */
>>> +static void powernv_brightness_set(struct led_classdev *led_cdev,
>>> + enum led_brightness value)
>>> +{
>>> + struct powernv_led_data *powernv_led =
>>> + container_of(led_cdev, struct powernv_led_data, cdev);
>>> +
>>> + /* Do not modify LED in unload path */
>>> + if (led_disabled)
>>> + return;
>>> +
>>> + /* Prepare the request */
>>> + powernv_led->value = value;
>>> +
>>> + /* Schedule the new task */
>>> + schedule_work(&powernv_led->work_led);
>>> +}
>>> +
>>> +/* LED classdev 'brightness_get' function */
>>> +static enum led_brightness
>>> +powernv_brightness_get(struct led_classdev *led_cdev)
>>> +{
>>> + return powernv_led_get(led_cdev);
>>> +}
>>> +
>>> +
>>> +/*
>>> + * This function registers classdev structure for any given type of LED on
>>> + * a given child LED device node.
>>> + */
>>> +static int powernv_led_create(struct device *dev,
>>> + struct powernv_led_data *powernv_led,
>>> + const char *led_name, const char *led_type_desc)
>>> +{
>>> + int rc;
>>> +
>>> + /* Create the name for classdev */
>>> + powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s",
>>> + led_name, led_type_desc);
>>
>> Please use devm_kasprintf.
>
> Done.
>
>>
>>> + if (!powernv_led->cdev.name) {
>>> + dev_err(dev,
>>> + "%s: Memory allocation failed for classdev name\n",
>>> + __func__);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + /* Make sure LED type is supported */
>>> + if (powernv_get_led_type(&powernv_led->cdev) == -1) {
>>> + kfree(powernv_led->cdev.name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + powernv_led->cdev.brightness_set = powernv_brightness_set;
>>> + powernv_led->cdev.brightness_get = powernv_brightness_get;
>>> + powernv_led->cdev.brightness = LED_OFF;
>>> + powernv_led->cdev.max_brightness = LED_FULL;
>>> +
>>> + mutex_init(&powernv_led->lock);
>>> + INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set);
>>> +
>>> + /* Register the classdev */
>>> + rc = led_classdev_register(dev, &powernv_led->cdev);
>>
>> devm_led_classdev_register is also available.
>
> Looks like most of the existing drivers are using led_classdev_register function..
> Which one is preferred here?
It is quite new API, but it is now preferable.
>>
>>> + if (rc) {
>>> + dev_err(dev, "%s: Classdev registration failed for %s\n",
>>> + __func__, powernv_led->cdev.name);
>>> + kfree(powernv_led->cdev.name);
>>> + }
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +/* Unregister classdev structure for any given LED */
>>> +static void powernv_led_delete(struct powernv_led_data *powernv_led)
>>> +{
>>> + led_classdev_unregister(&powernv_led->cdev);
>>> +}
>>
>> This function is redundant.
>
> Like powernv_led_create, I just added this function ... hoping this will improve
> code readability.
> Will remove this function.
>>
>>> +/* Go through LED device tree node and register LED classdev structure */
>>> +static int powernv_led_classdev(struct platform_device *pdev,
>>> + struct device_node *led_node,
>>> + struct powernv_leds_priv *priv, int num_leds)
>>> +{
>>> + const char *cur = NULL;
>>> + int i, rc = -1;
>>> + struct property *p;
>>> + struct device_node *np;
>>> + struct powernv_led_data *powernv_led;
>>> + struct device *dev = &pdev->dev;
>>> +
>>> + for_each_child_of_node(led_node, np) {
>>> + p = of_find_property(np, "led-types", NULL);
>>> + if (!p)
>>> + continue;
>>> +
>>> + while ((cur = of_prop_next_string(p, cur)) != NULL) {
>>> + powernv_led = &priv->powernv_leds[priv->num_leds++];
>>> + if (priv->num_leds > num_leds) {
>>> + rc = -ENOMEM;
>>> + goto classdev_fail;
>>> + }
>>> + rc = powernv_led_create(dev,
>>> + powernv_led, np->name, cur);
>>> + if (rc)
>>> + goto classdev_fail;
>>> + } /* while end */
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, priv);
>>> + return rc;
>>> +
>>> +classdev_fail:
>>> + for (i = priv->num_leds - 2; i > 0; i--)
>>
>> Why do you leave element with id == 0 unreleased?
>
> It was my mistake. Fixed.
>
>>
>>> + powernv_led_delete(&priv->powernv_leds[i]);
>>> +
>>> + return rc;
>>> +}
>>> +
>>> +/*
>>> + * We want to populate LED device for each LED type. Hence we
>>> + * have to calculate count explicitly.
>>> + */
>>> +static int powernv_leds_count(struct device_node *led_node)
>>> +{
>>> + const char *cur = NULL;
>>> + int num_leds = 0;
>>> + struct property *p;
>>> + struct device_node *np;
>>> +
>>> + for_each_child_of_node(led_node, np) {
>>> + p = of_find_property(np, "led-types", NULL);
>>> + if (!p)
>>> + continue;
>>> +
>>> + while ((cur = of_prop_next_string(p, cur)) != NULL)
>>> + num_leds++;
>>> + }
>>> +
>>> + return num_leds;
>>> +}
>>
>> Does it mean that if the node exists but does't have led-types
>> property we are not going to register it?
>
> Yes.. No point in registering location code ..which doesn't have led-types
> property.
>
>
>> I assume that this is
>> firmware which generates the nodes, otherwise it would make
>> no sense to have the node, am I right?
>
> That correct. Our firmware generates this property.
Thanks for the clarification.
>>
>>> +/* Platform driver probe */
>>> +static int powernv_led_probe(struct platform_device *pdev)
>>> +{
>>> + int num_leds;
>>> + struct device_node *led_node;
>>> + struct powernv_leds_priv *priv;
>>> +
>>> + led_node = of_find_node_by_path("/ibm,opal/leds");
>>> + if (!led_node) {
>>> + dev_err(&pdev->dev,
>>> + "%s: LED parent device node not found\n", __func__);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + num_leds = powernv_leds_count(led_node);
>>> + if (num_leds <= 0) {
>>> + dev_err(&pdev->dev,
>>> + "%s: No location code found under LED node\n",
>>> + __func__);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + priv = devm_kzalloc(&pdev->dev,
>>> + sizeof_powernv_leds_priv(num_leds),
>>> + GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + return powernv_led_classdev(pdev, led_node, priv, num_leds);
>>> +}
>>> +
>>> +/* Platform driver remove */
>>> +static int powernv_led_remove(struct platform_device *pdev)
>>> +{
>>> + int i;
>>> + struct powernv_led_data *powernv_led;
>>> + struct powernv_leds_priv *priv;
>>> +
>>> + /* Disable LED operation */
>>> + led_disabled = true;
>>> +
>>> + priv = platform_get_drvdata(pdev);
>>> +
>>> + for (i = 0; i < priv->num_leds; i++) {
>>> + powernv_led = &priv->powernv_leds[i];
>>> + powernv_led_delete(powernv_led);
>>> + flush_work(&powernv_led->work_led);
>>> + }
>>
>> You are missing mutex_destroy here.
>
> Oh yeah.. I missed it. Fixed.
>
> -Vasant
>
>
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-16 8:27 ` Jacek Anaszewski
@ 2015-07-16 8:47 ` Michael Ellerman
2015-07-17 5:53 ` Vasant Hegde
2015-07-17 5:52 ` Vasant Hegde
1 sibling, 1 reply; 10+ messages in thread
From: Michael Ellerman @ 2015-07-16 8:47 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Vasant Hegde, stewart, arnd, j.anaszewski81, cooloney, rpurdie,
linuxppc-dev, linux-leds, khandual
On Thu, 2015-07-16 at 10:27 +0200, Jacek Anaszewski wrote:
> On 07/16/2015 08:54 AM, Vasant Hegde wrote:
> >>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
> >>> +{
> >>> + char *loc_code;
> >>> + int rc, led_type;
> >>> + __be64 led_mask, led_value, max_led_type;
> >>> +
> >>> + led_type = powernv_get_led_type(led_cdev);
> >>> + if (led_type == -1)
> >>> + return LED_OFF;
> >>> +
> >>> + loc_code = powernv_get_location_code(led_cdev);
> >>> + if (!loc_code)
> >>> + return LED_OFF;
> >>> +
> >>> + /* Fetch all LED status */
> >>> + led_mask = cpu_to_be64(0);
> >>> + led_value = cpu_to_be64(0);
> >>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> >>> +
> >>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
> >>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
> >>> + dev_err(led_cdev->dev,
> >>> + "%s: OPAL get led call failed [rc=%d]\n",
> >>> + __func__, rc);
> >>> + goto led_fail;
> >>> + }
> >>> +
> >>> + led_mask = be64_to_cpu(led_mask);
> >>> + led_value = be64_to_cpu(led_value);
> >>
> >> be64_to_cpu result should be assigned to the variable of u64/s64 type.
> >
> > PowerNV platform is capable of running both big/little endian mode.. But
> > presently our firmware is big endian. These variable contains big endian values.
> > Hence I have created as __be64 .. (This is the convention we follow in other
> > places as well).
>
> It is correct that the argument is of __be64 type, but be64_to_cpu
> returns u64 type, whereas you assign it to __be64.
Yeah that's wrong. You are using led_mask etc as __be64 when you pass them to
firmware, which is correct, but then you're also using them as the lvalue of
be64_to_cpu() which returns a u64.
Sparse should warn you about that if you use it, please do.
$ apt-get install sparse
$ cd kernel
$ make C=2 CF=-D__CHECK_ENDIAN__
Whether the kernel or OPAL is running big or little endian is irrelevant to all
of this. The OPAL API defines that parameters to OPAL calls are big endian, and
that's all that matters:
https://github.com/open-power/skiboot/blob/master/doc/opal-spec.txt#L142
Thanks for the review Jacek.
cheers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-16 8:27 ` Jacek Anaszewski
2015-07-16 8:47 ` Michael Ellerman
@ 2015-07-17 5:52 ` Vasant Hegde
1 sibling, 0 replies; 10+ messages in thread
From: Vasant Hegde @ 2015-07-17 5:52 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: stewart, arnd, j.anaszewski81, cooloney, rpurdie, linuxppc-dev,
linux-leds, khandual
On 07/16/2015 01:57 PM, Jacek Anaszewski wrote:
> Hi Vasan,
>
Hello Jacek,
.../...
>>
>> I have added as
>> - compatible : "ibm,opal-v3-led".
>
> Please retain "Should be :".
>
Done.
.../...
>>>
>>> Please parse the led type once upon initialization and add related
>>> property to the struct powernv_led_data that will hold the value.
>>
>> I thought we can get location code and type using class dev name itself. Hence I
>> didn't add these two properties to structure..
>
> This way you are doing extra work for parsing the name each time
> the brightness is set.
Agreed. I have added them to structure now.
>
>> Do you want me to add them to structure itself?
>
> Yes, please add them.
Done.
>
>>>
>>>> + loc_code = powernv_get_location_code(led_cdev);
>>>> + if (!loc_code)
>>>> + return;
>>>
>>> The same situation as in case of led type.
>>>
>>>> + /* Prepare for the OPAL call */
>>>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>
>>> This value could be also calculated only once.
>>
>> Yeah. May be I can move this to powernv_leds_priv structure.
>>
>>>
>>>> + led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
>>>> + if (value)
>>>> + led_value = led_mask;
>>>> +
>>>> + /* OPAL async call */
>>>> + token = opal_async_get_token_interruptible();
>>>> + if (token < 0) {
>>>> + if (token != -ERESTARTSYS)
>>>> + dev_err(led_cdev->dev,
>>>> + "%s: Couldn't get OPAL async token\n",
>>>> + __func__);
>>>> + goto out_loc;
>>>> + }
>>>> +
>>>> + rc = opal_leds_set_ind(token, loc_code,
>>>> + led_mask, led_value, &max_led_type);
>>>> + if (rc != OPAL_ASYNC_COMPLETION) {
>>>> + dev_err(led_cdev->dev,
>>>> + "%s: OPAL set LED call failed for %s [rc=%d]\n",
>>>> + __func__, loc_code, rc);
>>>> + goto out_token;
>>>> + }
>>>> +
>>>> + rc = opal_async_wait_response(token, &msg);
>>>> + if (rc) {
>>>> + dev_err(led_cdev->dev,
>>>> + "%s: Failed to wait for the async response [rc=%d]\n",
>>>> + __func__, rc);
>>>> + goto out_token;
>>>> + }
>>>> +
>>>> + rc = be64_to_cpu(msg.params[1]);
>>>> + if (rc != OPAL_SUCCESS)
>>>> + dev_err(led_cdev->dev,
>>>> + "%s : OAPL async call returned failed [rc=%d]\n",
>>>> + __func__, rc);
>>>> +
>>>> +out_token:
>>>> + opal_async_release_token(token);
>>>> +
>>>> +out_loc:
>>>> + kfree(loc_code);
>>>> +}
>>>> +
>>>> +/*
>>>> + * This function fetches the LED state for a given LED type for
>>>> + * mentioned LED classdev structure.
>>>> + */
>>>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
>>>> +{
>>>> + char *loc_code;
>>>> + int rc, led_type;
>>>> + __be64 led_mask, led_value, max_led_type;
>>>> +
>>>> + led_type = powernv_get_led_type(led_cdev);
>>>> + if (led_type == -1)
>>>> + return LED_OFF;
>>>> +
>>>> + loc_code = powernv_get_location_code(led_cdev);
>>>> + if (!loc_code)
>>>> + return LED_OFF;
>>>> +
>>>> + /* Fetch all LED status */
>>>> + led_mask = cpu_to_be64(0);
>>>> + led_value = cpu_to_be64(0);
>>>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>> +
>>>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
>>>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>>> + dev_err(led_cdev->dev,
>>>> + "%s: OPAL get led call failed [rc=%d]\n",
>>>> + __func__, rc);
>>>> + goto led_fail;
>>>> + }
>>>> +
>>>> + led_mask = be64_to_cpu(led_mask);
>>>> + led_value = be64_to_cpu(led_value);
>>>
>>> be64_to_cpu result should be assigned to the variable of u64/s64 type.
>>
>> PowerNV platform is capable of running both big/little endian mode.. But
>> presently our firmware is big endian. These variable contains big endian values.
>> Hence I have created as __be64 .. (This is the convention we follow in other
>> places as well).
>
> It is correct that the argument is of __be64 type, but be64_to_cpu
> returns u64 type, whereas you assign it to __be64.
>
Got it .. Fixed.
>>>
>>>> + /* LED status available */
>>>> + if (!((led_mask >> led_type) & OPAL_SLOT_LED_STATE_ON)) {
>>>> + dev_err(led_cdev->dev,
>>>> + "%s: LED status not available for %s\n",
>>>> + __func__, led_cdev->name);
>>>> + goto led_fail;
>>>> + }
>>>> +
>>>> + /* LED status value */
>>>> + if ((led_value >> led_type) & OPAL_SLOT_LED_STATE_ON) {
>>>> + kfree(loc_code);
>>>> + return LED_FULL;
>>>> + }
>>>> +
>>>> +led_fail:
>>>> + kfree(loc_code);
>>>> + return LED_OFF;
>>>> +}
>>>> +
>>>> +/* Execute LED set task for given led classdev */
>>>> +static void powernv_deferred_led_set(struct work_struct *work)
>>>> +{
>>>> + struct powernv_led_data *powernv_led =
>>>> + container_of(work, struct powernv_led_data, work_led);
>>>> +
>>>> + mutex_lock(&powernv_led->lock);
>>>> + powernv_led_set(&powernv_led->cdev, powernv_led->value);
>>>> + mutex_unlock(&powernv_led->lock);
>>>> +}
>>>> +
>>>> +/*
>>>> + * LED classdev 'brightness_get' function. This schedules work
>>>> + * to update LED state.
>>>> + */
>>>> +static void powernv_brightness_set(struct led_classdev *led_cdev,
>>>> + enum led_brightness value)
>>>> +{
>>>> + struct powernv_led_data *powernv_led =
>>>> + container_of(led_cdev, struct powernv_led_data, cdev);
>>>> +
>>>> + /* Do not modify LED in unload path */
>>>> + if (led_disabled)
>>>> + return;
>>>> +
>>>> + /* Prepare the request */
>>>> + powernv_led->value = value;
>>>> +
>>>> + /* Schedule the new task */
>>>> + schedule_work(&powernv_led->work_led);
>>>> +}
>>>> +
>>>> +/* LED classdev 'brightness_get' function */
>>>> +static enum led_brightness
>>>> +powernv_brightness_get(struct led_classdev *led_cdev)
>>>> +{
>>>> + return powernv_led_get(led_cdev);
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * This function registers classdev structure for any given type of LED on
>>>> + * a given child LED device node.
>>>> + */
>>>> +static int powernv_led_create(struct device *dev,
>>>> + struct powernv_led_data *powernv_led,
>>>> + const char *led_name, const char *led_type_desc)
>>>> +{
>>>> + int rc;
>>>> +
>>>> + /* Create the name for classdev */
>>>> + powernv_led->cdev.name = kasprintf(GFP_KERNEL, "%s:%s",
>>>> + led_name, led_type_desc);
>>>
>>> Please use devm_kasprintf.
>>
>> Done.
>>
>>>
>>>> + if (!powernv_led->cdev.name) {
>>>> + dev_err(dev,
>>>> + "%s: Memory allocation failed for classdev name\n",
>>>> + __func__);
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + /* Make sure LED type is supported */
>>>> + if (powernv_get_led_type(&powernv_led->cdev) == -1) {
>>>> + kfree(powernv_led->cdev.name);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + powernv_led->cdev.brightness_set = powernv_brightness_set;
>>>> + powernv_led->cdev.brightness_get = powernv_brightness_get;
>>>> + powernv_led->cdev.brightness = LED_OFF;
>>>> + powernv_led->cdev.max_brightness = LED_FULL;
>>>> +
>>>> + mutex_init(&powernv_led->lock);
>>>> + INIT_WORK(&powernv_led->work_led, powernv_deferred_led_set);
>>>> +
>>>> + /* Register the classdev */
>>>> + rc = led_classdev_register(dev, &powernv_led->cdev);
>>>
>>> devm_led_classdev_register is also available.
>>
>> Looks like most of the existing drivers are using led_classdev_register
>> function..
>> Which one is preferred here?
>
> It is quite new API, but it is now preferable.
Ok..Let me use new API.
-Vasant
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
2015-07-16 8:47 ` Michael Ellerman
@ 2015-07-17 5:53 ` Vasant Hegde
0 siblings, 0 replies; 10+ messages in thread
From: Vasant Hegde @ 2015-07-17 5:53 UTC (permalink / raw)
To: Michael Ellerman, Jacek Anaszewski
Cc: stewart, arnd, j.anaszewski81, cooloney, rpurdie, linuxppc-dev,
linux-leds, khandual
On 07/16/2015 02:17 PM, Michael Ellerman wrote:
> On Thu, 2015-07-16 at 10:27 +0200, Jacek Anaszewski wrote:
>> On 07/16/2015 08:54 AM, Vasant Hegde wrote:
>>>>> +static enum led_brightness powernv_led_get(struct led_classdev *led_cdev)
>>>>> +{
>>>>> + char *loc_code;
>>>>> + int rc, led_type;
>>>>> + __be64 led_mask, led_value, max_led_type;
>>>>> +
>>>>> + led_type = powernv_get_led_type(led_cdev);
>>>>> + if (led_type == -1)
>>>>> + return LED_OFF;
>>>>> +
>>>>> + loc_code = powernv_get_location_code(led_cdev);
>>>>> + if (!loc_code)
>>>>> + return LED_OFF;
>>>>> +
>>>>> + /* Fetch all LED status */
>>>>> + led_mask = cpu_to_be64(0);
>>>>> + led_value = cpu_to_be64(0);
>>>>> + max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>>> +
>>>>> + rc = opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type);
>>>>> + if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>>>> + dev_err(led_cdev->dev,
>>>>> + "%s: OPAL get led call failed [rc=%d]\n",
>>>>> + __func__, rc);
>>>>> + goto led_fail;
>>>>> + }
>>>>> +
>>>>> + led_mask = be64_to_cpu(led_mask);
>>>>> + led_value = be64_to_cpu(led_value);
>>>>
>>>> be64_to_cpu result should be assigned to the variable of u64/s64 type.
>>>
>>> PowerNV platform is capable of running both big/little endian mode.. But
>>> presently our firmware is big endian. These variable contains big endian values.
>>> Hence I have created as __be64 .. (This is the convention we follow in other
>>> places as well).
>>
>> It is correct that the argument is of __be64 type, but be64_to_cpu
>> returns u64 type, whereas you assign it to __be64.
>
> Yeah that's wrong. You are using led_mask etc as __be64 when you pass them to
> firmware, which is correct, but then you're also using them as the lvalue of
> be64_to_cpu() which returns a u64.
>
Yep. Got it.
> Sparse should warn you about that if you use it, please do.
>
> $ apt-get install sparse
> $ cd kernel
> $ make C=2 CF=-D__CHECK_ENDIAN__
>
Thanks!
-Vasant
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-17 5:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-01 8:50 [PATCH v5 0/3] LED driver for PowerNV platform Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 2/3] powerpc/powernv: Create LED platform device Vasant Hegde
2015-07-01 8:50 ` [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform Vasant Hegde
2015-07-14 9:00 ` Jacek Anaszewski
2015-07-16 6:54 ` Vasant Hegde
2015-07-16 8:27 ` Jacek Anaszewski
2015-07-16 8:47 ` Michael Ellerman
2015-07-17 5:53 ` Vasant Hegde
2015-07-17 5:52 ` Vasant Hegde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).