On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
> Hi Vasant,
Hi Jacek,
>>
>> This patch implements LED driver for PowerNV platform using the existing
>> generic LED class framework. It registers classdev structures for all
>> individual LEDs detected on the system through LED specific device tree
>> nodes. Device tree nodes specify what all kind of LEDs present on the
>> same location code. It registers LED classdev structure for each of them.
>>
>> The platform level implementation of LED get and set state has been
>> achieved through OPAL calls. These calls are made available for the
>> driver by exporting from architecture specific codes.
>>
>> As per the LED class framework, the 'brightness_set' function should not
>> sleep. Hence these functions have been implemented through global work
>> queue tasks which might sleep on OPAL async call completion.
>
> Wouldn't it be easier to implement synchronization on the OPAL side?
We had thought about this.. But OPAL intern depends on service processor to
enable/disable indicator. So we can't make this as synchronous one.
.../...
>>
>> diff --git a/arch/powerpc/platforms/powernv/opal.c
>> b/arch/powerpc/platforms/powernv/opal.c
>> index 142a08a..fbfd9c1 100644
>> --- a/arch/powerpc/platforms/powernv/opal.c
>> +++ b/arch/powerpc/platforms/powernv/opal.c
>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>>
>> static int __init opal_init(void)
>> {
>> - struct device_node *np, *consoles;
>> + struct device_node *np, *consoles, *led;
>> int rc;
>>
>> opal_node = of_find_node_by_path("/ibm,opal");
>> @@ -772,6 +772,13 @@ static int __init opal_init(void)
>> /* Create i2c platform devices */
>> opal_i2c_create_devs();
>>
>> + /* Create led platform devices */
>> + led = of_find_node_by_path("/ibm,opal/led");
>> + if (led) {
>> + of_platform_device_create(led, "opal_led", NULL);
>> + of_node_put(led);
>> + }
>> +
>> /* Find all OPAL interrupts and request them */
>> opal_irq_init(opal_node);
>>
>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
>> EXPORT_SYMBOL_GPL(opal_tpo_read);
>> EXPORT_SYMBOL_GPL(opal_tpo_write);
>> EXPORT_SYMBOL_GPL(opal_i2c_request);
>> +/* Export these symbols for PowerNV LED class driver */
>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
>
> Please split the above part to the separate patch and put it in the
> series before this one.
Sure. Will split it in next version.
>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 25b320d..a93223c 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -508,6 +508,15 @@ config LEDS_BLINKM
>> This option enables support for the BlinkM RGB LED connected
>> through I2C. Say Y to enable support for the BlinkM LED.
>>
>> +config LEDS_POWERNV
>> + tristate "LED support for PowerNV Platform"
>> + depends on LEDS_CLASS
>> + depends on PPC_POWERNV
>
> OF dependency is missing here.
Agree. Will fix.
>
>> + help
>> + This option enables support for the system LEDs present on
>> + PowerNV platforms. Say 'y' to enable this support in kernel.
>> + Say 'm' enable this support as module.
>
> Please change the last line to:
>
> To compile this driver as a module, choose M here: the module will
> be called leds-powernv.
>
Sure. Will fix.
>
>> +
>> config LEDS_SYSCON
>> bool "LED support for LEDs on system controllers"
>> depends on LEDS_CLASS=y
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index cbba921..604ffc9 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o
>> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o
>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
>> +obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
>>
>> # LED SPI Drivers
>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
>> new file mode 100644
>> index 0000000..0c9f958
>> --- /dev/null
>> +++ b/drivers/leds/leds-powernv.c
>> @@ -0,0 +1,620 @@
>> +/*
>> + * PowerNV LED Driver
>> + *
>> + * Copyright IBM Corp. 2015
>> + *
>> + * Author: Anshuman Khandual <khandual@xxxxxxxxxxxxxxxxxx>
>> + * Author: Vasant Hegde <hegdevasant@xxxxxxxxxxxxxxxxxx>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +#define PREFIX "POWERNV_LED"
>> +#define pr_fmt(fmt) PREFIX ": " fmt
>
> Wouldn't you mind using dev_* prefixed logging?
> Unless you have a good reason not to do it.
>
I don't see any specific reason to use pr_fmt. Will convert this to dev_*.
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/slab.h>
>> +#include <linux/list.h>
>
> Please keep alphabetical order.
Sure.
>
>> +
>> +#include <asm/opal.h>
>> +
>> +#define LED_STR_ATTENTION ":ATTENTION"
>> +#define LED_STR_IDENT ":IDENTIFY"
>> +#define LED_STR_FAULT ":FAULT"
>
> Namespacing prefix is required here. LED is reserved for
> LED subsystem global macros. How about POWERNV_LED_* ?
Yep... Its my fault.. Makes sense to have POWERNV_*.
>
>> +
>> +/*
>> + * LED operation state
>> + *
>> + * led_classdev_unregister resets the brightness values. However
>> + * we want to retain the LED state across boot.
>
> What boot are you thinking of here?
>
>> Hence disable
>> + * LED operation before calling led_classdev_unregister.
>> + */
>
> This comment is unclear for me, as this is static initialization,
> unrelated to to any function call.
>
I mean, we have to retain the state of LED across system reboot.
>> +static bool led_disabled = false;
>
> Static variables are initialized to 0 by default.
Fixed.
.../...
>> +/*
>> + * powernv_led_compact_work_list
>> + *
>> + * This function goes through the entire list of scheduled powernv_led_work
>> + * nodes and removes the nodes which have already processed one set LED
>> + * state request from request list and has been marked for deletion. This is
>> + * essential for cleaning the list before adding new elements into it. This
>> + * also tracks the total number of pending tasks. Once it reaches the
>> + * threshold the function will throttle till all the scheduled tasks completes
>> + * execution during which the user space thread will block and will be
>> + * prevented from queuing up more LED state change requests.
>> + */
>
> Did you test the driver with led-triggers?
I have tested set/reset LEDs .. not triggers as our user space dont' use that.
>> +
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_ID:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>> + break;
>> + case OPAL_SLOT_LED_TYPE_FAULT:
>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>> + break;
>> + default: /* Unknown LED type */
>> + goto out_loc;
>> + }
>
> Above sequence is repeated in the function below - it could be wrapped
> with a new function.
Agree. Will fix.
.../...
>> +
>> +/*
>> + * power_led_classdev
>> + *
>> + * This function registers classdev structure for any given type of LED on
>> + * a given child LED device node.
>> + */
>> +static int power_led_classdev(struct platform_device *pdev,
>> + struct device_node *cled, u64 led_type)
>> +{
>> + int rc;
>> + unsigned long flags;
>> + struct powernv_led *cpled;
>> +
>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>> + if (!cpled) {
>> + pr_err("Memory allocation failed at %s\n", __func__);
>> + return -ENOMEM;
>> + }
>> +
>> + /* Create the name for classdev */
>> + switch (led_type) {
>> + case OPAL_SLOT_LED_TYPE_ATTN:
>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>> + cled->name, LED_STR_ATTENTION);
>
> We have a 'label' DT property for naming LED Flash class devices.
> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>
In Power Systems LEDs are overloaded (meaning same LED is used for identify and
fault depending on their state --- blinking = identify and solid = fault).
Hence here append LED type info.
.../...
>> +
>> +static struct platform_driver powernv_led_driver = {
>> + .probe = powernv_led_probe,
>> + .remove = powernv_led_remove,
>> + .driver = {
>> + .name = "powernv-led-driver",
>> + .owner = THIS_MODULE,
>> + .of_match_table = powernv_led_match,
>
> Is somewhere DT documentation available for these leds?
These are PowerNV platform specific properties. I don't think its
specified/documented in kernel side.
These are documented in OPAL (firmware) side.
Thanks for valuable review. Will post v3 ASAP.
-Vasant
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
