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

2015-07-23 Thread Jacek Anaszewski 

Vasant,

On 23.07.2015 10:08, Vasant Hegde wrote:

On 07/23/2015 01:25 PM, Jacek Anaszewski wrote:

Hi Vasant,



Jacek,

.../...


+/* PowerNV LED data */
+struct powernv_led_data {
+struct led_classdevcdev;
+char*loc_code;/* LED location code */
+intled_type;/* OPAL_SLOT_LED_TYPE_* */
+enum led_brightnessvalue;/* Brightness value */


You don't need 'value' as brightness value is already stored in
cdev.brightness.



Agree. I'll remove.


+/*
+ * 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;
+
+ return powernv_led_set(powernv_led);


Isn't mutex_lock/unlock missing here?


Yes. I realized this after sending out the patch. I will fix this.
.../...


+
+max_led_type = devm_kzalloc(dev, sizeof(*max_led_type), GFP_KERNEL);
+if (!max_led_type)
+return -ENOMEM;
+
+mutex_init(lock);
+*max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+
+platform_set_drvdata(pdev, lock);


Setting only lock as drvdata looks odd to me and I haven't noticed
anyone doing that.
I'd prefer to put lock, led_disabled and max_led_type in a common
struct and set it as drvdata. I know that I accepted this design, but
I didn't take into account these details.


Yeah. Even I looked into existing code and I don't see anyone using like this.
Since it's void * and we just need lock, I added like this.

If I break this into two structure, then I've to use platform_get_drvdata() call
in multiple functions like powernv_brightness_set() to get max_let_type etc. Is
that fine?


You could add wrappers for grouping instructions required to
retrieve given properties of the common struct.
Alternatively you could add a pointer to the common struct
in the struct powernv_led_data. You can play with these approaches
and choose the one which looks better.

--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2015-07-23 Thread Vasant Hegde 
On 07/23/2015 01:25 PM, Jacek Anaszewski wrote:
 Hi Vasant,
 

Jacek,

.../...

 +/* PowerNV LED data */
 +struct powernv_led_data {
 +struct led_classdevcdev;
 +char*loc_code;/* LED location code */
 +intled_type;/* OPAL_SLOT_LED_TYPE_* */
 +enum led_brightnessvalue;/* Brightness value */
 
 You don't need 'value' as brightness value is already stored in
 cdev.brightness.
 

Agree. I'll remove.

 +/*
 + * 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;
 +
 + return powernv_led_set(powernv_led);
 
 Isn't mutex_lock/unlock missing here?

Yes. I realized this after sending out the patch. I will fix this.
.../...

 +
 +max_led_type = devm_kzalloc(dev, sizeof(*max_led_type), GFP_KERNEL);
 +if (!max_led_type)
 +return -ENOMEM;
 +
 +mutex_init(lock);
 +*max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
 +
 +platform_set_drvdata(pdev, lock);
 
 Setting only lock as drvdata looks odd to me and I haven't noticed
 anyone doing that.
 I'd prefer to put lock, led_disabled and max_led_type in a common
 struct and set it as drvdata. I know that I accepted this design, but
 I didn't take into account these details.

Yeah. Even I looked into existing code and I don't see anyone using like this.
Since it's void * and we just need lock, I added like this.

If I break this into two structure, then I've to use platform_get_drvdata() call
in multiple functions like powernv_brightness_set() to get max_let_type etc. Is
that fine?

-Vasant

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2015-07-23 Thread Jacek Anaszewski 

Hi Vasant,

Thanks for the update.

On 22.07.2015 16:52, Vasant Hegde wrote:

This patch implements LED driver for PowerNV platform using the existing
generic LED class framework.

PowerNV platform has below type of LEDs:
   - System attention
   Indicates there is a problem with the system that needs attention.
   - Identify
   Helps the user locate/identify a particular FRU or resource in the
   system.
   - Fault
   Indicates there is a problem with the FRU or resource at the
   location with which the indicator is associated.

We register classdev structures for all individual LEDs detected on the
system through LED specific device tree nodes. Device tree nodes specify
what all kind of LEDs present on the same location code. It registers
LED classdev structure for each of them.

All the system LEDs can be found in the same regular path /sys/class/leds/.
We don't use LED colors. We use LED node and led-types property to form
LED classdev. Our LEDs have names in this format.

 location_code:attention|identify|fault

Any positive brightness value would turn on the LED and a zero value would
turn off the LED. The driver will return LED_FULL (255) for any turned on
LED and LED_OFF (0) for any turned off LED.

As per the LED class framework, the 'brightness_set' function should not
sleep. Hence these functions have been implemented through global work
queue tasks which might sleep on OPAL async call completion.

The platform level implementation of LED get and set state has been
achieved through OPAL calls. These calls are made available for the
driver by exporting from architecture specific codes.

Signed-off-by: Vasant Hegde hegdevas...@linux.vnet.ibm.com
Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
Acked-by: Stewart Smith stew...@linux.vnet.ibm.com
Tested-by: Stewart Smith stew...@linux.vnet.ibm.com
---
  .../devicetree/bindings/leds/leds-powernv.txt  |  26 ++
  drivers/leds/Kconfig   |  11 +
  drivers/leds/Makefile  |   1 +
  drivers/leds/leds-powernv.c| 342 +
  4 files changed, 380 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 000..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 000..9e70291
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,342 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Vasant Hegde hegdevas...@linux.vnet.ibm.com
+ * Author: Anshuman Khandual

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

2015-07-22 Thread 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 hegdevas...@linux.vnet.ibm.com
Signed-off-by: Anshuman Khandual khand...@linux.vnet.ibm.com
Acked-by: Stewart Smith stew...@linux.vnet.ibm.com
Tested-by: Stewart Smith stew...@linux.vnet.ibm.com
---
 .../devicetree/bindings/leds/leds-powernv.txt  |  26 ++
 drivers/leds/Kconfig   |  11 +
 drivers/leds/Makefile  |   1 +
 drivers/leds/leds-powernv.c| 342 +
 4 files changed, 380 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 000..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 000..9e70291
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,342 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Vasant Hegde hegdevas...@linux.vnet.ibm.com
+ * Author: Anshuman Khandual khand...@linux.vnet.ibm.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under