[PATCH v6 0/3] LED driver for PowerNV platform

[PATCH v6 0/3] LED driver for PowerNV platform

[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 top of v4.2-rc2.

@Jacek,
  Let me know if you want me to rebase this patchset on top of LED tree.

Previous patchset:
  v5: https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-July/130602.html
  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 v6:
  - Added loc_code and type to powernv_led_data structure instead of parsing
    them from led classdev name.
  - Fixed documentation issues.
  - Fixed mutex_destry issue
  - Replaced led_classdev_register with devm_led_classdev_register

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      |  26 ++
 arch/powerpc/include/asm/opal-api.h                |  25 +-
 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                        | 422 +++++++++++++++++++++
 8 files changed, 501 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

[PATCH v6 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states

[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..3233e6d 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, __be64 *led_mask,
+			  __be64 *led_value, __be64 *max_led_type);
+int64_t opal_leds_set_ind(uint64_t token, char *loc_code, const u64 led_mask,
+			  const u64 led_value, __be64 *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

[PATCH v6 2/3] powerpc/powernv: Create LED platform device

[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

[PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[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      |  26 ++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/Makefile                              |   1 +
 drivers/leds/leds-powernv.c                        | 422 +++++++++++++++++++++
 4 files changed, 460 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..6665569
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
@@ -0,0 +1,26 @@
+Device Tree binding for LEDs on IBM Power Systems
+-------------------------------------------------
+
+Required properties:
+- compatible : Should be "ibm,opal-v3-led".
+- led-mode   : Should be "lightpath" or "guidinglight".
+
+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.
+
+Example:
+
+leds {
+	compatible = "ibm,opal-v3-led";
+	led-mode = "lightpath";
+
+	U78C9.001.RST0027-P1-C1 {
+		led-types = "identify", "fault";
+	};
+	...
+	...
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9ad35f7..f218cc3a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -560,6 +560,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 8d6a24a..6a943d1 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -65,6 +65,7 @@ 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_KTD2692)		+= leds-ktd2692.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..c3c0b0c
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,422 @@
+/*
+ * 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;
+	char			*loc_code;	/* LED location code */
+	int			led_type;	/* OPAL_SLOT_LED_TYPE_* */
+	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 __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+
+
+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(const char *led_type_desc)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
+		if (!strcmp(led_type_map[i].desc, led_type_desc))
+			return led_type_map[i].type;
+
+	return -1;
+}
+
+/*
+ * 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 powernv_led_data *powernv_led)
+{
+	int rc, token;
+	u64 led_mask, led_value = 0;
+	__be64 max_type;
+	struct opal_msg msg;
+	struct device *dev = powernv_led->cdev.dev;
+
+	/* Prepare for the OPAL call */
+	max_type = max_led_type;
+	led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
+	if (powernv_led->value)
+		led_value = led_mask;
+
+	/* OPAL async call */
+	token = opal_async_get_token_interruptible();
+	if (token < 0) {
+		if (token != -ERESTARTSYS)
+			dev_err(dev, "%s: Couldn't get OPAL async token\n",
+				__func__);
+		return;
+	}
+
+	rc = opal_leds_set_ind(token, powernv_led->loc_code,
+			       led_mask, led_value, &max_type);
+	if (rc != OPAL_ASYNC_COMPLETION) {
+		dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
+			__func__, powernv_led->loc_code, rc);
+		goto out_token;
+	}
+
+	rc = opal_async_wait_response(token, &msg);
+	if (rc) {
+		dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
+			__func__, rc);
+
+out_token:
+	opal_async_release_token(token);
+}
+
+/*
+ * This function fetches the LED state for a given LED type for
+ * mentioned LED classdev structure.
+ */
+static enum led_brightness
+powernv_led_get(struct powernv_led_data *powernv_led)
+{
+	int rc;
+	__be64 mask, value, max_type;
+	u64 led_mask, led_value;
+	struct device *dev = powernv_led->cdev.dev;
+
+	/* Fetch all LED status */
+	mask = cpu_to_be64(0);
+	value = cpu_to_be64(0);
+	max_type = max_led_type;
+
+	rc = opal_leds_get_ind(powernv_led->loc_code,
+			       &mask, &value, &max_type);
+	if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
+		dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
+			__func__, rc);
+		return LED_OFF;
+	}
+
+	led_mask = be64_to_cpu(mask);
+	led_value = be64_to_cpu(value);
+
+	/* LED status available */
+	if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
+		dev_err(dev, "%s: LED status not available for %s\n",
+			__func__, powernv_led->cdev.name);
+		return LED_OFF;
+	}
+
+	/* LED status value */
+	if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
+		return LED_FULL;
+
+	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);
+	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)
+{
+	struct powernv_led_data *powernv_led =
+		container_of(led_cdev, struct powernv_led_data, cdev);
+
+	return powernv_led_get(powernv_led);
+}
+
+
+/*
+ * 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;
+
+	/* Make sure LED type is supported */
+	powernv_led->led_type = powernv_get_led_type(led_type_desc);
+	if (powernv_led->led_type == -1) {
+		dev_warn(dev, "%s: No support for led type : %s\n",
+			 __func__, led_type_desc);
+		return -EINVAL;
+	}
+
+	/* Location code of the LED */
+	powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);
+	if (!powernv_led->loc_code) {
+		dev_err(dev, "%s: Memory allocation failed\n", __func__);
+		return -ENOMEM;
+	}
+
+	/* Create the name for classdev */
+	powernv_led->cdev.name = devm_kasprintf(dev, 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__);
+		kfree(powernv_led->loc_code);
+		return -ENOMEM;
+	}
+
+	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 = devm_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->loc_code);
+		kfree(powernv_led->cdev.name);
+	}
+
+	return rc;
+}
+
+/* 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 = &priv->powernv_leds[i];
+		devm_led_classdev_unregister(dev, &powernv_led->cdev);
+		kfree(powernv_led->loc_code);
+		mutex_destroy(&powernv_led->lock);
+	}
+
+	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];
+		devm_led_classdev_unregister(&pdev->dev, &powernv_led->cdev);
+		flush_work(&powernv_led->work_led);
+		kfree(powernv_led->loc_code);
+		mutex_destroy(&powernv_led->lock);
+	}
+
+	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

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski]

Hi Vasant,

On 07/17/2015 12:41 PM, 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

Please don't exceed 75 character line length limit.

> 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      |  26 ++
>   drivers/leds/Kconfig                               |  11 +
>   drivers/leds/Makefile                              |   1 +
>   drivers/leds/leds-powernv.c                        | 422 +++++++++++++++++++++
>   4 files changed, 460 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..6665569
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
> @@ -0,0 +1,26 @@
> +Device Tree binding for LEDs on IBM Power Systems
> +-------------------------------------------------
> +
> +Required properties:
> +- compatible : Should be "ibm,opal-v3-led".
> +- led-mode   : Should be "lightpath" or "guidinglight".
> +
> +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.
> +
> +Example:
> +
> +leds {
> +	compatible = "ibm,opal-v3-led";
> +	led-mode = "lightpath";
> +
> +	U78C9.001.RST0027-P1-C1 {
> +		led-types = "identify", "fault";
> +	};
> +	...
> +	...
> +};
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9ad35f7..f218cc3a 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -560,6 +560,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 8d6a24a..6a943d1 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -65,6 +65,7 @@ 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_KTD2692)		+= leds-ktd2692.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..c3c0b0c
> --- /dev/null
> +++ b/drivers/leds/leds-powernv.c
> @@ -0,0 +1,422 @@
> +/*
> + * 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;

Please move this to the struct powernv_leds_priv.

> +
> +/* 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;
> +	char			*loc_code;	/* LED location code */
> +	int			led_type;	/* OPAL_SLOT_LED_TYPE_* */
> +	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 __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);

The C standard prohibits initialization of global objects with 
non-constant values. Section 6.7.8 of the C99 standard states:

"All the expressions in an initializer for an object that has static 
storage duration shall be constant expressions or string literals."

Compilation succeeds when using powerpc64-linux-gcc because then
the following version of macro is used:

#define cpu_to_be64(x) (x)

and not

#define cpu_to_be64(x) swab64(x)

Please move max_led_type also to the struct powernv_leds_priv
and initialize it dynamically.

> +
> +
> +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(const char *led_type_desc)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
> +		if (!strcmp(led_type_map[i].desc, led_type_desc))
> +			return led_type_map[i].type;
> +
> +	return -1;
> +}
> +
> +/*
> + * 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 powernv_led_data *powernv_led)
> +{
> +	int rc, token;
> +	u64 led_mask, led_value = 0;
> +	__be64 max_type;
> +	struct opal_msg msg;
> +	struct device *dev = powernv_led->cdev.dev;
> +
> +	/* Prepare for the OPAL call */
> +	max_type = max_led_type;
> +	led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
> +	if (powernv_led->value)
> +		led_value = led_mask;
> +
> +	/* OPAL async call */
> +	token = opal_async_get_token_interruptible();
> +	if (token < 0) {
> +		if (token != -ERESTARTSYS)
> +			dev_err(dev, "%s: Couldn't get OPAL async token\n",
> +				__func__);
> +		return;
> +	}
> +
> +	rc = opal_leds_set_ind(token, powernv_led->loc_code,
> +			       led_mask, led_value, &max_type);
> +	if (rc != OPAL_ASYNC_COMPLETION) {
> +		dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
> +			__func__, powernv_led->loc_code, rc);
> +		goto out_token;
> +	}
> +
> +	rc = opal_async_wait_response(token, &msg);
> +	if (rc) {
> +		dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
> +			__func__, rc);
> +
> +out_token:
> +	opal_async_release_token(token);
> +}
> +
> +/*
> + * This function fetches the LED state for a given LED type for
> + * mentioned LED classdev structure.
> + */
> +static enum led_brightness
> +powernv_led_get(struct powernv_led_data *powernv_led)
> +{
> +	int rc;
> +	__be64 mask, value, max_type;
> +	u64 led_mask, led_value;
> +	struct device *dev = powernv_led->cdev.dev;
> +
> +	/* Fetch all LED status */
> +	mask = cpu_to_be64(0);
> +	value = cpu_to_be64(0);
> +	max_type = max_led_type;
> +
> +	rc = opal_leds_get_ind(powernv_led->loc_code,
> +			       &mask, &value, &max_type);
> +	if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
> +		dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
> +			__func__, rc);
> +		return LED_OFF;
> +	}
> +
> +	led_mask = be64_to_cpu(mask);
> +	led_value = be64_to_cpu(value);
> +
> +	/* LED status available */
> +	if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
> +		dev_err(dev, "%s: LED status not available for %s\n",
> +			__func__, powernv_led->cdev.name);
> +		return LED_OFF;
> +	}
> +
> +	/* LED status value */
> +	if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
> +		return LED_FULL;
> +
> +	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);
> +	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)
> +{
> +	struct powernv_led_data *powernv_led =
> +		container_of(led_cdev, struct powernv_led_data, cdev);
> +
> +	return powernv_led_get(powernv_led);
> +}
> +
> +
> +/*
> + * 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;
> +
> +	/* Make sure LED type is supported */
> +	powernv_led->led_type = powernv_get_led_type(led_type_desc);
> +	if (powernv_led->led_type == -1) {
> +		dev_warn(dev, "%s: No support for led type : %s\n",
> +			 __func__, led_type_desc);
> +		return -EINVAL;
> +	}
> +
> +	/* Location code of the LED */
> +	powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);

DT node name is available all the time, you don't need another variable
for it.

> +	if (!powernv_led->loc_code) {
> +		dev_err(dev, "%s: Memory allocation failed\n", __func__);
> +		return -ENOMEM;
> +	}
> +
> +	/* Create the name for classdev */
> +	powernv_led->cdev.name = devm_kasprintf(dev, 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__);
> +		kfree(powernv_led->loc_code);
> +		return -ENOMEM;
> +	}
> +
> +	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 = devm_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->loc_code);
> +		kfree(powernv_led->cdev.name);
> +	}
> +
> +	return rc;
> +}
> +
> +/* 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 = &priv->powernv_leds[i];
> +		devm_led_classdev_unregister(dev, &powernv_led->cdev);
> +		kfree(powernv_led->loc_code);
> +		mutex_destroy(&powernv_led->lock);
> +	}
> +
> +	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];
> +		devm_led_classdev_unregister(&pdev->dev, &powernv_led->cdev);
> +		flush_work(&powernv_led->work_led);
> +		kfree(powernv_led->loc_code);
> +		mutex_destroy(&powernv_led->lock);
> +	}
> +
> +	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");

If you want to be consistent with what you declare in the beginnig
then it should be:

MODULE_LICENSE("GPL v2")

> +MODULE_DESCRIPTION("PowerNV LED driver");
> +MODULE_AUTHOR("Vasant Hegde <hegdevasant@linux.vnet.ibm.com>");
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Vasant Hegde]

On 07/17/2015 08:55 PM, Jacek Anaszewski wrote:
> Hi Vasant,

Hi Jacek,


.../...


>> 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
> 
> Please don't exceed 75 character line length limit.

Ok. I will fix it.. But I thought 80 character is the limit.

> 
>> through OPAL calls. These calls are made available for the driver by
>> exporting from architecture specific codes.
>>

.../...

>> +
>> +#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;
> 
> Please move this to the struct powernv_leds_priv.

Hmmm.. Ok. Will update and use platform_get_drvdata to access platform_get_drvdata.

> 
>> +
>> +/* 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;
>> +    char            *loc_code;    /* LED location code */
>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>> +    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 __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
> 
> The C standard prohibits initialization of global objects with non-constant
> values. Section 6.7.8 of the C99 standard states:
> 
> "All the expressions in an initializer for an object that has static storage
> duration shall be constant expressions or string literals."
> 
> Compilation succeeds when using powerpc64-linux-gcc because then
> the following version of macro is used:
> 
> #define cpu_to_be64(x) (x)
> 
> and not
> 
> #define cpu_to_be64(x) swab64(x)
> 
> Please move max_led_type also to the struct powernv_leds_priv
> and initialize it dynamically.

Yeah.. I should have added this to structure itself. Will fix.

> 
>> +
>> +
>> +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(const char *led_type_desc)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>> +        if (!strcmp(led_type_map[i].desc, led_type_desc))
>> +            return led_type_map[i].type;
>> +
>> +    return -1;
>> +}
>> +
>> +/*
>> + * 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 powernv_led_data *powernv_led)
>> +{
>> +    int rc, token;
>> +    u64 led_mask, led_value = 0;
>> +    __be64 max_type;
>> +    struct opal_msg msg;
>> +    struct device *dev = powernv_led->cdev.dev;
>> +
>> +    /* Prepare for the OPAL call */
>> +    max_type = max_led_type;
>> +    led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
>> +    if (powernv_led->value)
>> +        led_value = led_mask;
>> +
>> +    /* OPAL async call */
>> +    token = opal_async_get_token_interruptible();
>> +    if (token < 0) {
>> +        if (token != -ERESTARTSYS)
>> +            dev_err(dev, "%s: Couldn't get OPAL async token\n",
>> +                __func__);
>> +        return;
>> +    }
>> +
>> +    rc = opal_leds_set_ind(token, powernv_led->loc_code,
>> +                   led_mask, led_value, &max_type);
>> +    if (rc != OPAL_ASYNC_COMPLETION) {
>> +        dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
>> +            __func__, powernv_led->loc_code, rc);
>> +        goto out_token;
>> +    }
>> +
>> +    rc = opal_async_wait_response(token, &msg);
>> +    if (rc) {
>> +        dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
>> +            __func__, rc);
>> +
>> +out_token:
>> +    opal_async_release_token(token);
>> +}
>> +
>> +/*
>> + * This function fetches the LED state for a given LED type for
>> + * mentioned LED classdev structure.
>> + */
>> +static enum led_brightness
>> +powernv_led_get(struct powernv_led_data *powernv_led)
>> +{
>> +    int rc;
>> +    __be64 mask, value, max_type;
>> +    u64 led_mask, led_value;
>> +    struct device *dev = powernv_led->cdev.dev;
>> +
>> +    /* Fetch all LED status */
>> +    mask = cpu_to_be64(0);
>> +    value = cpu_to_be64(0);
>> +    max_type = max_led_type;
>> +
>> +    rc = opal_leds_get_ind(powernv_led->loc_code,
>> +                   &mask, &value, &max_type);
>> +    if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>> +        dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
>> +            __func__, rc);
>> +        return LED_OFF;
>> +    }
>> +
>> +    led_mask = be64_to_cpu(mask);
>> +    led_value = be64_to_cpu(value);
>> +
>> +    /* LED status available */
>> +    if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
>> +        dev_err(dev, "%s: LED status not available for %s\n",
>> +            __func__, powernv_led->cdev.name);
>> +        return LED_OFF;
>> +    }
>> +
>> +    /* LED status value */
>> +    if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
>> +        return LED_FULL;
>> +
>> +    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);
>> +    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)
>> +{
>> +    struct powernv_led_data *powernv_led =
>> +        container_of(led_cdev, struct powernv_led_data, cdev);
>> +
>> +    return powernv_led_get(powernv_led);
>> +}
>> +
>> +
>> +/*
>> + * 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;
>> +
>> +    /* Make sure LED type is supported */
>> +    powernv_led->led_type = powernv_get_led_type(led_type_desc);
>> +    if (powernv_led->led_type == -1) {
>> +        dev_warn(dev, "%s: No support for led type : %s\n",
>> +             __func__, led_type_desc);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Location code of the LED */
>> +    powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);
> 
> DT node name is available all the time, you don't need another variable
> for it.

Yes.. But then we have to traverse through  DT node get location code in
powernv_led_get() and powernv_led_set() function. something like
	for_each_child_of_node(led_node, np) {
		if (!strstr(np->name, powernv_led->cdev.name) {
			/* Process get/set LED */
			break;
		}
	}
	
Hence I thought of adding that to structure itself.

-Vasant

			
> 
>> +    if (!powernv_led->loc_code) {
>> +        dev_err(dev, "%s: Memory allocation failed\n", __func__);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    /* Create the name for classdev */
>> +    powernv_led->cdev.name = devm_kasprintf(dev, 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__);
>> +        kfree(powernv_led->loc_code);
>> +        return -ENOMEM;
>> +    }
>> +
>> +    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 = devm_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->loc_code);
>> +        kfree(powernv_led->cdev.name);
>> +    }
>> +
>> +    return rc;
>> +}
>> +
>> +/* 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 = &priv->powernv_leds[i];
>> +        devm_led_classdev_unregister(dev, &powernv_led->cdev);
>> +        kfree(powernv_led->loc_code);
>> +        mutex_destroy(&powernv_led->lock);
>> +    }
>> +
>> +    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];
>> +        devm_led_classdev_unregister(&pdev->dev, &powernv_led->cdev);
>> +        flush_work(&powernv_led->work_led);
>> +        kfree(powernv_led->loc_code);
>> +        mutex_destroy(&powernv_led->lock);
>> +    }
>> +
>> +    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");
> 
> If you want to be consistent with what you declare in the beginnig
> then it should be:
> 
> MODULE_LICENSE("GPL v2")

Fixed.

Thanks!
-Vasant

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski]

Hi Vasant,

I've revised your patch and found few more issues.
Please refer to my comments below.


On 17.07.2015 18:20, Vasant Hegde wrote:
> On 07/17/2015 08:55 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>
> Hi Jacek,
>
>
> .../...
>
>
>>> 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
>>
>> Please don't exceed 75 character line length limit.
>
> Ok. I will fix it.. But I thought 80 character is the limit.

checkpatch.pl reports this.

>>
>>> through OPAL calls. These calls are made available for the driver by
>>> exporting from architecture specific codes.
>>>
>
> .../...
>
>>> +
>>> +#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;
>>
>> Please move this to the struct powernv_leds_priv.
>
> Hmmm.. Ok. Will update and use platform_get_drvdata to access platform_get_drvdata.
>>
>>> +
>>> +/* 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;
>>> +    char            *loc_code;    /* LED location code */
>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>> +    enum led_brightness    value;        /* Brightness value */
>>> +    struct mutex        lock;

You're unnecessarily adding mutex for each LED class device.
The shared resource to protect is here powernv led interface,
so one mutex will suffice.


>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>> +};
>>> +
>>> +struct powernv_leds_priv {
>>> +    int num_leds;
>>> +    struct powernv_led_data powernv_leds[];
>>> +};

powernv_led_data doesn't have to be retained in the array. You access
the array elements only upon creation of LED class devices. When using
managed resource allocation you don't need to bother with freeing
resources, so you don't need to keep reference to the data.

I propose to drop struct powernv_leds_priv and instead introduce
a structure that would aggregate common driver data like mutex,
led_disable and max_led_type.

>>> +
>>> +static __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>
>> The C standard prohibits initialization of global objects with non-constant
>> values. Section 6.7.8 of the C99 standard states:
>>
>> "All the expressions in an initializer for an object that has static storage
>> duration shall be constant expressions or string literals."
>>
>> Compilation succeeds when using powerpc64-linux-gcc because then
>> the following version of macro is used:
>>
>> #define cpu_to_be64(x) (x)
>>
>> and not
>>
>> #define cpu_to_be64(x) swab64(x)
>>
>> Please move max_led_type also to the struct powernv_leds_priv
>> and initialize it dynamically.
>
> Yeah.. I should have added this to structure itself. Will fix.
>
>>
>>> +
>>> +
>>> +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(const char *led_type_desc)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>>> +        if (!strcmp(led_type_map[i].desc, led_type_desc))
>>> +            return led_type_map[i].type;
>>> +
>>> +    return -1;
>>> +}
>>> +
>>> +/*
>>> + * 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 powernv_led_data *powernv_led)
>>> +{
>>> +    int rc, token;
>>> +    u64 led_mask, led_value = 0;
>>> +    __be64 max_type;
>>> +    struct opal_msg msg;
>>> +    struct device *dev = powernv_led->cdev.dev;
>>> +
>>> +    /* Prepare for the OPAL call */
>>> +    max_type = max_led_type;
>>> +    led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
>>> +    if (powernv_led->value)
>>> +        led_value = led_mask;
>>> +
>>> +    /* OPAL async call */
>>> +    token = opal_async_get_token_interruptible();
>>> +    if (token < 0) {
>>> +        if (token != -ERESTARTSYS)
>>> +            dev_err(dev, "%s: Couldn't get OPAL async token\n",
>>> +                __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    rc = opal_leds_set_ind(token, powernv_led->loc_code,
>>> +                   led_mask, led_value, &max_type);
>>> +    if (rc != OPAL_ASYNC_COMPLETION) {
>>> +        dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
>>> +            __func__, powernv_led->loc_code, rc);
>>> +        goto out_token;
>>> +    }
>>> +
>>> +    rc = opal_async_wait_response(token, &msg);
>>> +    if (rc) {
>>> +        dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
>>> +            __func__, rc);
>>> +
>>> +out_token:
>>> +    opal_async_release_token(token);
>>> +}
>>> +
>>> +/*
>>> + * This function fetches the LED state for a given LED type for
>>> + * mentioned LED classdev structure.
>>> + */
>>> +static enum led_brightness
>>> +powernv_led_get(struct powernv_led_data *powernv_led)
>>> +{
>>> +    int rc;
>>> +    __be64 mask, value, max_type;
>>> +    u64 led_mask, led_value;
>>> +    struct device *dev = powernv_led->cdev.dev;
>>> +
>>> +    /* Fetch all LED status */
>>> +    mask = cpu_to_be64(0);
>>> +    value = cpu_to_be64(0);
>>> +    max_type = max_led_type;
>>> +
>>> +    rc = opal_leds_get_ind(powernv_led->loc_code,
>>> +                   &mask, &value, &max_type);
>>> +    if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>> +        dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
>>> +            __func__, rc);
>>> +        return LED_OFF;
>>> +    }
>>> +
>>> +    led_mask = be64_to_cpu(mask);
>>> +    led_value = be64_to_cpu(value);
>>> +
>>> +    /* LED status available */
>>> +    if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
>>> +        dev_err(dev, "%s: LED status not available for %s\n",
>>> +            __func__, powernv_led->cdev.name);
>>> +        return LED_OFF;
>>> +    }
>>> +
>>> +    /* LED status value */
>>> +    if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
>>> +        return LED_FULL;
>>> +
>>> +    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);
>>> +    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)
>>> +{
>>> +    struct powernv_led_data *powernv_led =
>>> +        container_of(led_cdev, struct powernv_led_data, cdev);
>>> +
>>> +    return powernv_led_get(powernv_led);
>>> +}
>>> +
>>> +
>>> +/*
>>> + * 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;
>>> +
>>> +    /* Make sure LED type is supported */
>>> +    powernv_led->led_type = powernv_get_led_type(led_type_desc);
>>> +    if (powernv_led->led_type == -1) {
>>> +        dev_warn(dev, "%s: No support for led type : %s\n",
>>> +             __func__, led_type_desc);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    /* Location code of the LED */
>>> +    powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);
>>
>> DT node name is available all the time, you don't need another variable
>> for it.
>
> Yes.. But then we have to traverse through  DT node get location code in
> powernv_led_get() and powernv_led_set() function. something like
> 	for_each_child_of_node(led_node, np) {
> 		if (!strstr(np->name, powernv_led->cdev.name) {
> 			/* Process get/set LED */
> 			break;
> 		}
> 	}
> 	
> Hence I thought of adding that to structure itself.

I meant that you can store the pointer to the DT node name, which
is a location code name and you don't need to allocate new memory
for the string.

> 			
>>
>>> +    if (!powernv_led->loc_code) {
>>> +        dev_err(dev, "%s: Memory allocation failed\n", __func__);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    /* Create the name for classdev */
>>> +    powernv_led->cdev.name = devm_kasprintf(dev, 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__);
>>> +        kfree(powernv_led->loc_code);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    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 = devm_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->loc_code);
>>> +        kfree(powernv_led->cdev.name);

You don't need to explicitly free the memory allocated with
devm_kasprintf if this will result in probe failure - all allocations
committed with managed resource allocation will be unrolled then.

>>> +    }
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +/* 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 = &priv->powernv_leds[i];
>>> +        devm_led_classdev_unregister(dev, &powernv_led->cdev);

Upon driver detach all LED class devices registered with devm prefixed
API will be unregistered automatically.

>>> +        kfree(powernv_led->loc_code);
>>> +        mutex_destroy(&powernv_led->lock);
>>> +    }
>>> +
>>> +    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;
>>> +    }

You won't need to count the number of LEDs to register, just allocate
memory for each LED during parsing with managed resource allocation
API.

>>> +
>>> +    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);
>>> +

Here you will only need mutex_destroy.

Please also drop work queue, as it is going to be supported
by the LED core, when the patch set [1] will be merged.

>>> +    for (i = 0; i < priv->num_leds; i++) {
>>> +        powernv_led = &priv->powernv_leds[i];
>>> +        devm_led_classdev_unregister(&pdev->dev, &powernv_led->cdev);
>>> +        flush_work(&powernv_led->work_led);
>>> +        kfree(powernv_led->loc_code);
>>> +        mutex_destroy(&powernv_led->lock);
>>> +    }
>>> +
>>> +    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");
>>
>> If you want to be consistent with what you declare in the beginnig
>> then it should be:
>>
>> MODULE_LICENSE("GPL v2")
>
> Fixed.
>
> Thanks!
> -Vasant
>
>

[1] https://lkml.org/lkml/2015/7/17/151

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski]

On 19.07.2015 23:40, Jacek Anaszewski wrote:
[...]
>>>> +/* 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;
>>>> +    }
>
> You won't need to count the number of LEDs to register, just allocate
> memory for each LED during parsing with managed resource allocation
> API.

You can refer to drivers/leds/leds-bcm6328.c in order to gain
full picture on how this can look like.
struct powernv_led_data would have to carry the pointer to the
new common struct.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Vasant Hegde]

On 07/20/2015 11:46 AM, Jacek Anaszewski wrote:
> On 19.07.2015 23:40, Jacek Anaszewski wrote:
> [...]
>>>>> +/* 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;
>>>>> +    }
>>
>> You won't need to count the number of LEDs to register, just allocate
>> memory for each LED during parsing with managed resource allocation
>> API.

Jacek,

> 
> You can refer to drivers/leds/leds-bcm6328.c in order to gain
> full picture on how this can look like.
> struct powernv_led_data would have to carry the pointer to the
> new common struct.
> 

Sure.. Will though this code and will fix powernv code..

Thanks
-Vasant

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Vasant Hegde]

On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
> Hi Vasant,
> 

Jacek,

> I've revised your patch and found few more issues.
> Please refer to my comments below.

Thanks.

.../...

>>>
>>> Please don't exceed 75 character line length limit.
>>
>> Ok. I will fix it.. But I thought 80 character is the limit.
> 
> checkpatch.pl reports this.

Ah! I was running checkpatch.pl against source. Let me fix this.
.../...

>>>> +/*
>>>> + * 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;
>>>> +    char            *loc_code;    /* LED location code */
>>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>>> +    enum led_brightness    value;        /* Brightness value */
>>>> +    struct mutex        lock;
> 
> You're unnecessarily adding mutex for each LED class device.
> The shared resource to protect is here powernv led interface,
> so one mutex will suffice.


Ok. Let me move that to common structure.

> 
> 
>>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>>> +};
>>>> +
>>>> +struct powernv_leds_priv {
>>>> +    int num_leds;
>>>> +    struct powernv_led_data powernv_leds[];
>>>> +};
> 
> powernv_led_data doesn't have to be retained in the array. You access
> the array elements only upon creation of LED class devices. When using
> managed resource allocation you don't need to bother with freeing
> resources, so you don't need to keep reference to the data.
> 
> I propose to drop struct powernv_leds_priv and instead introduce
> a structure that would aggregate common driver data like mutex,
> led_disable and max_led_type.

I still think we need two structures.. One for common driver data like mutex,
led_disable etc and other one for led data itself .. like
	struct powernv_led_data {
		struct led_classdev     cdev;
		char                    *loc_code;     <-- pointer to DT node
		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
		enum led_brightness     value;          /* Brightness value */
	};

	struct powernv_led_common {
		bool led_disable;
		int 	max_led_type;
		struct mutex            lock;
	};

> 
>>>> +
>>>> +static __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>
>>> The C standard prohibits initialization of global objects with non-constant
>>> values. Section 6.7.8 of the C99 standard states:
>>>
>>> "All the expressions in an initializer for an object that has static storage
>>> duration shall be constant expressions or string literals."
>>>
>>> Compilation succeeds when using powerpc64-linux-gcc because then
>>> the following version of macro is used:
>>>
>>> #define cpu_to_be64(x) (x)
>>>
>>> and not
>>>
>>> #define cpu_to_be64(x) swab64(x)
>>>
>>> Please move max_led_type also to the struct powernv_leds_priv
>>> and initialize it dynamically.
>>
>> Yeah.. I should have added this to structure itself. Will fix.
>>
>>>
>>>> +
>>>> +
>>>> +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(const char *led_type_desc)
>>>> +{
>>>> +    int i;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>>>> +        if (!strcmp(led_type_map[i].desc, led_type_desc))
>>>> +            return led_type_map[i].type;
>>>> +
>>>> +    return -1;
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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 powernv_led_data *powernv_led)
>>>> +{
>>>> +    int rc, token;
>>>> +    u64 led_mask, led_value = 0;
>>>> +    __be64 max_type;
>>>> +    struct opal_msg msg;
>>>> +    struct device *dev = powernv_led->cdev.dev;
>>>> +
>>>> +    /* Prepare for the OPAL call */
>>>> +    max_type = max_led_type;
>>>> +    led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
>>>> +    if (powernv_led->value)
>>>> +        led_value = led_mask;
>>>> +
>>>> +    /* OPAL async call */
>>>> +    token = opal_async_get_token_interruptible();
>>>> +    if (token < 0) {
>>>> +        if (token != -ERESTARTSYS)
>>>> +            dev_err(dev, "%s: Couldn't get OPAL async token\n",
>>>> +                __func__);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    rc = opal_leds_set_ind(token, powernv_led->loc_code,
>>>> +                   led_mask, led_value, &max_type);
>>>> +    if (rc != OPAL_ASYNC_COMPLETION) {
>>>> +        dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
>>>> +            __func__, powernv_led->loc_code, rc);
>>>> +        goto out_token;
>>>> +    }
>>>> +
>>>> +    rc = opal_async_wait_response(token, &msg);
>>>> +    if (rc) {
>>>> +        dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
>>>> +            __func__, rc);
>>>> +
>>>> +out_token:
>>>> +    opal_async_release_token(token);
>>>> +}
>>>> +
>>>> +/*
>>>> + * This function fetches the LED state for a given LED type for
>>>> + * mentioned LED classdev structure.
>>>> + */
>>>> +static enum led_brightness
>>>> +powernv_led_get(struct powernv_led_data *powernv_led)
>>>> +{
>>>> +    int rc;
>>>> +    __be64 mask, value, max_type;
>>>> +    u64 led_mask, led_value;
>>>> +    struct device *dev = powernv_led->cdev.dev;
>>>> +
>>>> +    /* Fetch all LED status */
>>>> +    mask = cpu_to_be64(0);
>>>> +    value = cpu_to_be64(0);
>>>> +    max_type = max_led_type;
>>>> +
>>>> +    rc = opal_leds_get_ind(powernv_led->loc_code,
>>>> +                   &mask, &value, &max_type);
>>>> +    if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>>> +        dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
>>>> +            __func__, rc);
>>>> +        return LED_OFF;
>>>> +    }
>>>> +
>>>> +    led_mask = be64_to_cpu(mask);
>>>> +    led_value = be64_to_cpu(value);
>>>> +
>>>> +    /* LED status available */
>>>> +    if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
>>>> +        dev_err(dev, "%s: LED status not available for %s\n",
>>>> +            __func__, powernv_led->cdev.name);
>>>> +        return LED_OFF;
>>>> +    }
>>>> +
>>>> +    /* LED status value */
>>>> +    if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
>>>> +        return LED_FULL;
>>>> +
>>>> +    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);
>>>> +    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)
>>>> +{
>>>> +    struct powernv_led_data *powernv_led =
>>>> +        container_of(led_cdev, struct powernv_led_data, cdev);
>>>> +
>>>> +    return powernv_led_get(powernv_led);
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * 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;
>>>> +
>>>> +    /* Make sure LED type is supported */
>>>> +    powernv_led->led_type = powernv_get_led_type(led_type_desc);
>>>> +    if (powernv_led->led_type == -1) {
>>>> +        dev_warn(dev, "%s: No support for led type : %s\n",
>>>> +             __func__, led_type_desc);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    /* Location code of the LED */
>>>> +    powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);
>>>
>>> DT node name is available all the time, you don't need another variable
>>> for it.
>>
>> Yes.. But then we have to traverse through  DT node get location code in
>> powernv_led_get() and powernv_led_set() function. something like
>>     for_each_child_of_node(led_node, np) {
>>         if (!strstr(np->name, powernv_led->cdev.name) {
>>             /* Process get/set LED */
>>             break;
>>         }
>>     }
>>     
>> Hence I thought of adding that to structure itself.
> 
> I meant that you can store the pointer to the DT node name, which
> is a location code name and you don't need to allocate new memory
> for the string.

Oh yeah.. Will do that.

> 
>>            
>>>
>>>> +    if (!powernv_led->loc_code) {
>>>> +        dev_err(dev, "%s: Memory allocation failed\n", __func__);
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    /* Create the name for classdev */
>>>> +    powernv_led->cdev.name = devm_kasprintf(dev, 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__);
>>>> +        kfree(powernv_led->loc_code);
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +
>>>> +    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 = devm_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->loc_code);
>>>> +        kfree(powernv_led->cdev.name);
> 
> You don't need to explicitly free the memory allocated with
> devm_kasprintf if this will result in probe failure - all allocations
> committed with managed resource allocation will be unrolled then.

Ok. Fixed.

> 
>>>> +    }
>>>> +
>>>> +    return rc;
>>>> +}
>>>> +
>>>> +/* 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 = &priv->powernv_leds[i];
>>>> +        devm_led_classdev_unregister(dev, &powernv_led->cdev);
> 
> Upon driver detach all LED class devices registered with devm prefixed
> API will be unregistered automatically.

Ok.

> 
>>>> +        kfree(powernv_led->loc_code);
>>>> +        mutex_destroy(&powernv_led->lock);
>>>> +    }
>>>> +
>>>> +    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;
>>>> +    }
> 
> You won't need to count the number of LEDs to register, just allocate
> memory for each LED during parsing with managed resource allocation
> API.


Hmmm. Ok.

> 
>>>> +
>>>> +    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);
>>>> +
> 
> Here you will only need mutex_destroy.
> 
> Please also drop work queue, as it is going to be supported
> by the LED core, when the patch set [1] will be merged.

Is this hosted somewhere? So that I can use that to test my code.
I looked into LED tree and couldn't find this patchset.

-Vasant

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Vasant Hegde]

On 07/21/2015 11:24 AM, Vasant Hegde wrote:
> On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
> 
> Jacek,
> 
>> I've revised your patch and found few more issues.
>> Please refer to my comments below.
> 
> Thanks.
> 
> .../...
> 
>>>>
>>>> Please don't exceed 75 character line length limit.
>>>
>>> Ok. I will fix it.. But I thought 80 character is the limit.
>>
>> checkpatch.pl reports this.
> 
> Ah! I was running checkpatch.pl against source. Let me fix this.
> .../...
> 
>>>>> +/*
>>>>> + * 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;
>>>>> +    char            *loc_code;    /* LED location code */
>>>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>>>> +    enum led_brightness    value;        /* Brightness value */
>>>>> +    struct mutex        lock;
>>
>> You're unnecessarily adding mutex for each LED class device.
>> The shared resource to protect is here powernv led interface,
>> so one mutex will suffice.
> 
> 
> Ok. Let me move that to common structure.
> 
>>
>>
>>>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>>>> +};
>>>>> +
>>>>> +struct powernv_leds_priv {
>>>>> +    int num_leds;
>>>>> +    struct powernv_led_data powernv_leds[];
>>>>> +};
>>
>> powernv_led_data doesn't have to be retained in the array. You access
>> the array elements only upon creation of LED class devices. When using
>> managed resource allocation you don't need to bother with freeing
>> resources, so you don't need to keep reference to the data.
>>
>> I propose to drop struct powernv_leds_priv and instead introduce
>> a structure that would aggregate common driver data like mutex,
>> led_disable and max_led_type.
> 
> I still think we need two structures.. One for common driver data like mutex,
> led_disable etc and other one for led data itself .. like
> 	struct powernv_led_data {
> 		struct led_classdev     cdev;
> 		char                    *loc_code;     <-- pointer to DT node
> 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
> 		enum led_brightness     value;          /* Brightness value */
> 	};
> 
> 	struct powernv_led_common {
> 		bool led_disable;
> 		int 	max_led_type;
> 		struct mutex            lock;
> 	};

Jacek,

Alternatively I can club both into single structure like below

 	struct powernv_led_data {
 		struct led_classdev     cdev;
 		char                    *loc_code;     <-- pointer to DT node
 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
 		enum led_brightness     value;          /* Brightness value */

 		int 		*max_led_type;
 		struct mutex            *lock;
 	};

	static  bool 	led_disable;


In this case I've to keep led_disable outside the structure as I need to access
this variable in powernv_led_remove().

One remaining issue with these approach (where we don't have array of
powernv_led ) is,
powernv_let_set() function can sleep. Current code (v6) calls flush_work before
unloading
module. That way we are sure work is complete before unloading module.
With new approach, I'm not sure how I can make sure work is completed before
loading module. Does new workqueue approach has sleep functionality? Is there
way to make sure work is completed before
unloading module?

-Vasant

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski]

Hi Vasant,

On 21.07.2015 07:54, Vasant Hegde wrote:
> On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>
> Jacek,
>
>> I've revised your patch and found few more issues.
>> Please refer to my comments below.
>
> Thanks.
>
> .../...
>
>>>>
>>>> Please don't exceed 75 character line length limit.
>>>
>>> Ok. I will fix it.. But I thought 80 character is the limit.
>>
>> checkpatch.pl reports this.
>
> Ah! I was running checkpatch.pl against source. Let me fix this.
> .../...
>
>>>>> +/*
>>>>> + * 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;
>>>>> +    char            *loc_code;    /* LED location code */
>>>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>>>> +    enum led_brightness    value;        /* Brightness value */
>>>>> +    struct mutex        lock;
>>
>> You're unnecessarily adding mutex for each LED class device.
>> The shared resource to protect is here powernv led interface,
>> so one mutex will suffice.
>
>
> Ok. Let me move that to common structure.
>
>>
>>
>>>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>>>> +};
>>>>> +
>>>>> +struct powernv_leds_priv {
>>>>> +    int num_leds;
>>>>> +    struct powernv_led_data powernv_leds[];
>>>>> +};
>>
>> powernv_led_data doesn't have to be retained in the array. You access
>> the array elements only upon creation of LED class devices. When using
>> managed resource allocation you don't need to bother with freeing
>> resources, so you don't need to keep reference to the data.
>>
>> I propose to drop struct powernv_leds_priv and instead introduce
>> a structure that would aggregate common driver data like mutex,
>> led_disable and max_led_type.
>
> I still think we need two structures.. One for common driver data like mutex,
> led_disable etc and other one for led data itself .. like
> 	struct powernv_led_data {
> 		struct led_classdev     cdev;
> 		char                    *loc_code;     <-- pointer to DT node
> 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
> 		enum led_brightness     value;          /* Brightness value */
> 	};
>
> 	struct powernv_led_common {
> 		bool led_disable;
> 		int 	max_led_type;
> 		struct mutex            lock;
> 	};

This is exactly what I proposed. I didn't mention dropping
struct powernv_led_data.

>>
>>>>> +
>>>>> +static __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>>
>>>> The C standard prohibits initialization of global objects with non-constant
>>>> values. Section 6.7.8 of the C99 standard states:
>>>>
>>>> "All the expressions in an initializer for an object that has static storage
>>>> duration shall be constant expressions or string literals."
>>>>
>>>> Compilation succeeds when using powerpc64-linux-gcc because then
>>>> the following version of macro is used:
>>>>
>>>> #define cpu_to_be64(x) (x)
>>>>
>>>> and not
>>>>
>>>> #define cpu_to_be64(x) swab64(x)
>>>>
>>>> Please move max_led_type also to the struct powernv_leds_priv
>>>> and initialize it dynamically.
>>>
>>> Yeah.. I should have added this to structure itself. Will fix.
>>>
>>>>
>>>>> +
>>>>> +
>>>>> +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(const char *led_type_desc)
>>>>> +{
>>>>> +    int i;
>>>>> +
>>>>> +    for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
>>>>> +        if (!strcmp(led_type_map[i].desc, led_type_desc))
>>>>> +            return led_type_map[i].type;
>>>>> +
>>>>> +    return -1;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * 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 powernv_led_data *powernv_led)
>>>>> +{
>>>>> +    int rc, token;
>>>>> +    u64 led_mask, led_value = 0;
>>>>> +    __be64 max_type;
>>>>> +    struct opal_msg msg;
>>>>> +    struct device *dev = powernv_led->cdev.dev;
>>>>> +
>>>>> +    /* Prepare for the OPAL call */
>>>>> +    max_type = max_led_type;
>>>>> +    led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
>>>>> +    if (powernv_led->value)
>>>>> +        led_value = led_mask;
>>>>> +
>>>>> +    /* OPAL async call */
>>>>> +    token = opal_async_get_token_interruptible();
>>>>> +    if (token < 0) {
>>>>> +        if (token != -ERESTARTSYS)
>>>>> +            dev_err(dev, "%s: Couldn't get OPAL async token\n",
>>>>> +                __func__);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    rc = opal_leds_set_ind(token, powernv_led->loc_code,
>>>>> +                   led_mask, led_value, &max_type);
>>>>> +    if (rc != OPAL_ASYNC_COMPLETION) {
>>>>> +        dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
>>>>> +            __func__, powernv_led->loc_code, rc);
>>>>> +        goto out_token;
>>>>> +    }
>>>>> +
>>>>> +    rc = opal_async_wait_response(token, &msg);
>>>>> +    if (rc) {
>>>>> +        dev_err(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(dev, "%s : OAPL async call returned failed [rc=%d]\n",
>>>>> +            __func__, rc);
>>>>> +
>>>>> +out_token:
>>>>> +    opal_async_release_token(token);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * This function fetches the LED state for a given LED type for
>>>>> + * mentioned LED classdev structure.
>>>>> + */
>>>>> +static enum led_brightness
>>>>> +powernv_led_get(struct powernv_led_data *powernv_led)
>>>>> +{
>>>>> +    int rc;
>>>>> +    __be64 mask, value, max_type;
>>>>> +    u64 led_mask, led_value;
>>>>> +    struct device *dev = powernv_led->cdev.dev;
>>>>> +
>>>>> +    /* Fetch all LED status */
>>>>> +    mask = cpu_to_be64(0);
>>>>> +    value = cpu_to_be64(0);
>>>>> +    max_type = max_led_type;
>>>>> +
>>>>> +    rc = opal_leds_get_ind(powernv_led->loc_code,
>>>>> +                   &mask, &value, &max_type);
>>>>> +    if (rc != OPAL_SUCCESS && rc != OPAL_PARTIAL) {
>>>>> +        dev_err(dev, "%s: OPAL get led call failed [rc=%d]\n",
>>>>> +            __func__, rc);
>>>>> +        return LED_OFF;
>>>>> +    }
>>>>> +
>>>>> +    led_mask = be64_to_cpu(mask);
>>>>> +    led_value = be64_to_cpu(value);
>>>>> +
>>>>> +    /* LED status available */
>>>>> +    if (!((led_mask >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)) {
>>>>> +        dev_err(dev, "%s: LED status not available for %s\n",
>>>>> +            __func__, powernv_led->cdev.name);
>>>>> +        return LED_OFF;
>>>>> +    }
>>>>> +
>>>>> +    /* LED status value */
>>>>> +    if ((led_value >> powernv_led->led_type) & OPAL_SLOT_LED_STATE_ON)
>>>>> +        return LED_FULL;
>>>>> +
>>>>> +    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);
>>>>> +    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)
>>>>> +{
>>>>> +    struct powernv_led_data *powernv_led =
>>>>> +        container_of(led_cdev, struct powernv_led_data, cdev);
>>>>> +
>>>>> +    return powernv_led_get(powernv_led);
>>>>> +}
>>>>> +
>>>>> +
>>>>> +/*
>>>>> + * 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;
>>>>> +
>>>>> +    /* Make sure LED type is supported */
>>>>> +    powernv_led->led_type = powernv_get_led_type(led_type_desc);
>>>>> +    if (powernv_led->led_type == -1) {
>>>>> +        dev_warn(dev, "%s: No support for led type : %s\n",
>>>>> +             __func__, led_type_desc);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    /* Location code of the LED */
>>>>> +    powernv_led->loc_code = kasprintf(GFP_KERNEL, "%s", led_name);
>>>>
>>>> DT node name is available all the time, you don't need another variable
>>>> for it.
>>>
>>> Yes.. But then we have to traverse through  DT node get location code in
>>> powernv_led_get() and powernv_led_set() function. something like
>>>      for_each_child_of_node(led_node, np) {
>>>          if (!strstr(np->name, powernv_led->cdev.name) {
>>>              /* Process get/set LED */
>>>              break;
>>>          }
>>>      }
>>>
>>> Hence I thought of adding that to structure itself.
>>
>> I meant that you can store the pointer to the DT node name, which
>> is a location code name and you don't need to allocate new memory
>> for the string.
>
> Oh yeah.. Will do that.
>
>>
>>>
>>>>
>>>>> +    if (!powernv_led->loc_code) {
>>>>> +        dev_err(dev, "%s: Memory allocation failed\n", __func__);
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    /* Create the name for classdev */
>>>>> +    powernv_led->cdev.name = devm_kasprintf(dev, 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__);
>>>>> +        kfree(powernv_led->loc_code);
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +
>>>>> +    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 = devm_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->loc_code);
>>>>> +        kfree(powernv_led->cdev.name);
>>
>> You don't need to explicitly free the memory allocated with
>> devm_kasprintf if this will result in probe failure - all allocations
>> committed with managed resource allocation will be unrolled then.
>
> Ok. Fixed.
>
>>
>>>>> +    }
>>>>> +
>>>>> +    return rc;
>>>>> +}
>>>>> +
>>>>> +/* 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 = &priv->powernv_leds[i];
>>>>> +        devm_led_classdev_unregister(dev, &powernv_led->cdev);
>>
>> Upon driver detach all LED class devices registered with devm prefixed
>> API will be unregistered automatically.
>
> Ok.
>
>>
>>>>> +        kfree(powernv_led->loc_code);
>>>>> +        mutex_destroy(&powernv_led->lock);
>>>>> +    }
>>>>> +
>>>>> +    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;
>>>>> +    }
>>
>> You won't need to count the number of LEDs to register, just allocate
>> memory for each LED during parsing with managed resource allocation
>> API.
>
>
> Hmmm. Ok.
>
>>
>>>>> +
>>>>> +    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);
>>>>> +
>>
>> Here you will only need mutex_destroy.
>>
>> Please also drop work queue, as it is going to be supported
>> by the LED core, when the patch set [1] will be merged.
>
> Is this hosted somewhere? So that I can use that to test my code.
> I looked into LED tree and couldn't find this patchset.

It is not merged yet. Please apply LED core patches from
that patch set to test it. If LED subsystem driver doesn't
set LED_BRIGHTNESS_FAST flag the core runs brightness_set
op in a work queue task.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski]

Vasant,

On 21.07.2015 08:55, Vasant Hegde wrote:
> On 07/21/2015 11:24 AM, Vasant Hegde wrote:
>> On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
>>> Hi Vasant,
>>>
>>
>> Jacek,
>>
>>> I've revised your patch and found few more issues.
>>> Please refer to my comments below.
>>
>> Thanks.
>>
>> .../...
>>
>>>>>
>>>>> Please don't exceed 75 character line length limit.
>>>>
>>>> Ok. I will fix it.. But I thought 80 character is the limit.
>>>
>>> checkpatch.pl reports this.
>>
>> Ah! I was running checkpatch.pl against source. Let me fix this.
>> .../...
>>
>>>>>> +/*
>>>>>> + * 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;
>>>>>> +    char            *loc_code;    /* LED location code */
>>>>>> +    int            led_type;    /* OPAL_SLOT_LED_TYPE_* */
>>>>>> +    enum led_brightness    value;        /* Brightness value */
>>>>>> +    struct mutex        lock;
>>>
>>> You're unnecessarily adding mutex for each LED class device.
>>> The shared resource to protect is here powernv led interface,
>>> so one mutex will suffice.
>>
>>
>> Ok. Let me move that to common structure.
>>
>>>
>>>
>>>>>> +    struct work_struct    work_led;    /* LED update workqueue */
>>>>>> +};
>>>>>> +
>>>>>> +struct powernv_leds_priv {
>>>>>> +    int num_leds;
>>>>>> +    struct powernv_led_data powernv_leds[];
>>>>>> +};
>>>
>>> powernv_led_data doesn't have to be retained in the array. You access
>>> the array elements only upon creation of LED class devices. When using
>>> managed resource allocation you don't need to bother with freeing
>>> resources, so you don't need to keep reference to the data.
>>>
>>> I propose to drop struct powernv_leds_priv and instead introduce
>>> a structure that would aggregate common driver data like mutex,
>>> led_disable and max_led_type.
>>
>> I still think we need two structures.. One for common driver data like mutex,
>> led_disable etc and other one for led data itself .. like
>> 	struct powernv_led_data {
>> 		struct led_classdev     cdev;
>> 		char                    *loc_code;     <-- pointer to DT node
>> 		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
>> 		enum led_brightness     value;          /* Brightness value */
>> 	};
>>
>> 	struct powernv_led_common {
>> 		bool led_disable;
>> 		int 	max_led_type;
>> 		struct mutex            lock;
>> 	};
>
> Jacek,
>
> Alternatively I can club both into single structure like below
>
>   	struct powernv_led_data {
>   		struct led_classdev     cdev;
>   		char                    *loc_code;     <-- pointer to DT node
>   		int                     led_type;       /* OPAL_SLOT_LED_TYPE_* */
>   		enum led_brightness     value;          /* Brightness value */
>
>   		int 		*max_led_type;
>   		struct mutex            *lock;
>   	};
>
> 	static  bool 	led_disable;
>
>
> In this case I've to keep led_disable outside the structure as I need to access
> this variable in powernv_led_remove().

OK, this looks good to me.

>
> One remaining issue with these approach (where we don't have array of
> powernv_led ) is,
> powernv_let_set() function can sleep. Current code (v6) calls flush_work before
> unloading
> module. That way we are sure work is complete before unloading module.
> With new approach, I'm not sure how I can make sure work is completed before
> loading module.

led_classdev_unregister calls cancel_work_sync so brightness_set op
will not be called after driver detach.

 >  Does new workqueue approach has sleep functionality? Is there
> way to make sure work is completed before
> unloading module?

It is approached from the other side - we are making sure that
work will not be executed after driver detach.

-- 
Best Regards,
Jacek Anaszewski