Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform

[Jacek Anaszewski <j.anaszewski@samsung.com>]

Hi Vasant,

On 04/15/2015 08:26 AM, Vasant Hegde wrote:
> On 04/14/2015 08:50 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>
> Hi Jacek,
>
>>>
>>> This patch implements LED driver for PowerNV platform using the existing
>>> generic LED class framework. It registers classdev structures for all
>>> individual LEDs detected on the system through LED specific device tree
>>> nodes. Device tree nodes specify what all kind of LEDs present on the
>>> same location code. It registers LED classdev structure for each of them.
>>>
>>> The platform level implementation of LED get and set state has been
>>> achieved through OPAL calls. These calls are made available for the
>>> driver by exporting from architecture specific codes.
>>>
>>> As per the LED class framework, the 'brightness_set' function should not
>>> sleep. Hence these functions have been implemented through global work
>>> queue tasks which might sleep on OPAL async call completion.
>>
>> Wouldn't it be easier to implement synchronization on the OPAL side?
>
> We had thought about this.. But OPAL intern depends on service processor to
> enable/disable indicator. So we can't make this as synchronous one.

I've revised this one more time and I think that use of spin locks is an
overkill in this case. It would suffice to define three work queues -
one per led and one common mutex.

powernv_led_set and powernv_led_get would have to be called under the
mutex. Please refer to the existing LED class drivers of the LED
controllers with many sub-LEDs to control: e.g.:
drivers/leds/leds-lm355x.c

>
> .../...
>
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/opal.c
>>> b/arch/powerpc/platforms/powernv/opal.c
>>> index 142a08a..fbfd9c1 100644
>>> --- a/arch/powerpc/platforms/powernv/opal.c
>>> +++ b/arch/powerpc/platforms/powernv/opal.c
>>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn)
>>>
>>>    static int __init opal_init(void)
>>>    {
>>> -    struct device_node *np, *consoles;
>>> +    struct device_node *np, *consoles, *led;
>>>        int rc;
>>>
>>>        opal_node = of_find_node_by_path("/ibm,opal");
>>> @@ -772,6 +772,13 @@ static int __init opal_init(void)
>>>        /* Create i2c platform devices */
>>>        opal_i2c_create_devs();
>>>
>>> +    /* Create led platform devices */
>>> +    led = of_find_node_by_path("/ibm,opal/led");
>>> +    if (led) {
>>> +        of_platform_device_create(led, "opal_led", NULL);
>>> +        of_node_put(led);
>>> +    }
>>> +
>>>        /* Find all OPAL interrupts and request them */
>>>        opal_irq_init(opal_node);
>>>
>>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write);
>>>    EXPORT_SYMBOL_GPL(opal_tpo_read);
>>>    EXPORT_SYMBOL_GPL(opal_tpo_write);
>>>    EXPORT_SYMBOL_GPL(opal_i2c_request);
>>> +/* Export these symbols for PowerNV LED class driver */
>>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind);
>>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind);
>>
>> Please split the above part to the separate patch and put it in the
>> series before this one.
>
> Sure. Will split it in next version.
>
>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index 25b320d..a93223c 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -508,6 +508,15 @@ config LEDS_BLINKM
>>>          This option enables support for the BlinkM RGB LED connected
>>>          through I2C. Say Y to enable support for the BlinkM LED.
>>>
>>> +config LEDS_POWERNV
>>> +    tristate "LED support for PowerNV Platform"
>>> +    depends on LEDS_CLASS
>>> +    depends on PPC_POWERNV
>>
>> OF dependency is missing here.
>
> Agree. Will fix.
>
>>
>>> +    help
>>> +      This option enables support for the system LEDs present on
>>> +      PowerNV platforms. Say 'y' to enable this support in kernel.
>>> +      Say 'm' enable this support as module.
>>
>> Please change the last line to:
>>
>> To compile this driver as a module, choose M here: the module will
>> be called leds-powernv.
>>
>
> Sure. Will fix.
>
>>
>>> +
>>>    config LEDS_SYSCON
>>>        bool "LED support for LEDs on system controllers"
>>>        depends on LEDS_CLASS=y
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index cbba921..604ffc9 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM)        += leds-blinkm.o
>>>    obj-$(CONFIG_LEDS_SYSCON)        += leds-syscon.o
>>>    obj-$(CONFIG_LEDS_VERSATILE)        += leds-versatile.o
>>>    obj-$(CONFIG_LEDS_MENF21BMC)        += leds-menf21bmc.o
>>> +obj-$(CONFIG_LEDS_POWERNV)        += leds-powernv.o
>>>
>>>    # LED SPI Drivers
>>>    obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o
>>> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
>>> new file mode 100644
>>> index 0000000..0c9f958
>>> --- /dev/null
>>> +++ b/drivers/leds/leds-powernv.c
>>> @@ -0,0 +1,620 @@
>>> +/*
>>> + * PowerNV LED Driver
>>> + *
>>> + * Copyright IBM Corp. 2015
>>> + *
>>> + * Author: Anshuman Khandual <khandual@linux.vnet.ibm.com>
>>> + * Author: Vasant Hegde <hegdevasant@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.
>>> + */
>>> +
>>> +#define PREFIX        "POWERNV_LED"
>>> +#define pr_fmt(fmt)    PREFIX ": " fmt
>>
>> Wouldn't you mind using dev_* prefixed logging?
>> Unless you have a good reason not to do it.
>>
>
> I don't see any specific reason to use pr_fmt. Will convert this to dev_*.
>
>
>>> +
>>> +#include <linux/platform_device.h>
>>> +#include <linux/leds.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/list.h>
>>
>> Please keep alphabetical order.
>
> Sure.
>
>>
>>> +
>>> +#include <asm/opal.h>
>>> +
>>> +#define LED_STR_ATTENTION    ":ATTENTION"
>>> +#define LED_STR_IDENT        ":IDENTIFY"
>>> +#define LED_STR_FAULT        ":FAULT"
>>
>> Namespacing prefix is required here. LED is reserved for
>> LED subsystem global macros. How about POWERNV_LED_* ?
>
> Yep... Its my fault.. Makes sense to have POWERNV_*.

You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION.

>>
>>> +
>>> +/*
>>> + * LED operation state
>>> + *
>>> + * led_classdev_unregister resets the brightness values. However
>>> + * we want to retain the LED state across boot.
>>
>> What boot are you thinking of here?
>>
>>> Hence disable
>>> + * LED operation before calling led_classdev_unregister.
>>> + */
>>
>> This comment is unclear for me, as this is static initialization,
>> unrelated to to any function call.
>>
>
> I mean, we have to retain the state of LED across system reboot.

Static variables are reinitialized on system reboot, aren't they?

>>> +static bool led_disabled = false;
>>
>> Static variables are initialized to 0 by default.
>
> Fixed.
>
> .../...
>
>>> +/*
>>> + * powernv_led_compact_work_list
>>> + *
>>> + * This function goes through the entire list of scheduled powernv_led_work
>>> + * nodes and removes the nodes which have already processed one set LED
>>> + * state request from request list and has been marked for deletion. This is
>>> + * essential for cleaning the list before adding new elements into it. This
>>> + * also tracks the total number of pending tasks. Once it reaches the
>>> + * threshold the function will throttle till all the scheduled tasks completes
>>> + * execution during which the user space thread will block and will be
>>> + * prevented from queuing up more LED state change requests.
>>> + */
>>
>> Did you test the driver with led-triggers?
>
> I have tested set/reset LEDs .. not triggers as our user space dont' use that.

At this moment it doesn't, but it can use them in the future. Besides,
I think that you wouldn't like to see someone testing your driver with
led-triggers and having it crashed? :)

Testing LED subsystem drivers with led-triggers is useful also because
some triggers (e.g. timer) call brightness_set op from the interrupt
context. It allows to detect some non-obvious issues.

>>> +
>>> +    switch (led_type) {
>>> +    case OPAL_SLOT_LED_TYPE_ATTN:
>>> +        loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0';
>>> +        break;
>>> +    case OPAL_SLOT_LED_TYPE_ID:
>>> +        loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0';
>>> +        break;
>>> +    case OPAL_SLOT_LED_TYPE_FAULT:
>>> +        loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0';
>>> +        break;
>>> +    default:    /* Unknown LED type */
>>> +        goto out_loc;
>>> +    }
>>
>> Above sequence is repeated in the function below - it could be wrapped
>> with a new function.
>
> Agree. Will fix.
>
> .../...
>
>>> +
>>> +/*
>>> + * power_led_classdev
>>> + *
>>> + * This function registers classdev structure for any given type of LED on
>>> + * a given child LED device node.
>>> + */
>>> +static int power_led_classdev(struct platform_device *pdev,
>>> +                  struct device_node *cled, u64 led_type)
>>> +{
>>> +    int rc;
>>> +    unsigned long flags;
>>> +    struct powernv_led *cpled;
>>> +
>>> +    cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL);
>>> +    if (!cpled) {
>>> +        pr_err("Memory allocation failed at %s\n", __func__);
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    /* Create the name for classdev */
>>> +    switch (led_type) {
>>> +    case OPAL_SLOT_LED_TYPE_ATTN:
>>> +        cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s",
>>> +                         cled->name, LED_STR_ATTENTION);
>>
>> We have a 'label' DT property for naming LED Flash class devices.
>> Please refer to Documentation/devicetree/bindings/leds/common.txt.
>>
>
> In Power Systems LEDs are overloaded (meaning same LED is used for identify and
> fault depending on their state  ---  blinking = identify and solid = fault).
> Hence here append LED type info.

The label could be composed of segments and an ordinal number as
labels have to be unique, e.g. attn_ident_0, attn_ident_1.
The segments would have to be parsed by the driver to discover
all the LED's available modes.

nitpicking: identify is a verb and is not a proper name for the LED.
Could you describe the purpose of this mode, so that we could come
up with a better name?


>
> .../...
>
>>> +
>>> +static struct platform_driver powernv_led_driver = {
>>> +    .probe    = powernv_led_probe,
>>> +    .remove = powernv_led_remove,
>>> +    .driver = {
>>> +        .name = "powernv-led-driver",
>>> +        .owner = THIS_MODULE,
>>> +        .of_match_table = powernv_led_match,
>>
>> Is somewhere DT documentation available for these leds?
>
> These are PowerNV platform specific properties. I don't think its
> specified/documented in kernel side.
> These are documented in OPAL (firmware) side.

Every driver using DT bindings needs DT documentation.
Moreover the one exists for powerenv platform:

Documentation/devicetree/bindings/hwmon/ibmpowernv.txt

Bindings for these LEDs should be added there probably.

-- 
Best Regards,
Jacek Anaszewski