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

[Vasant Hegde]

On 04/23/2015 03:15 AM, Jacek Anaszewski wrote:
>> On 21.04.2015 07:50, Vasant Hegde wrote:
>> On 04/20/2015 08:57 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,
>>>
>>

Jacek,

.../...

>>>>
>>>> All the system LEDs can be found in the same regular
>>>> path /sys/class/leds/. We don't use LED colors. Hence our LEDs have
>>>> names in this format.
>>>>
>>>>           <location_code>:<ATTENTION|IDENT|FAULT>
>>>
>>> I think that powernv prefix should be also present in the beginning.
>>> What would be the format of <location_code> ? Does it identify a LED
>>> uniquely.
>>
>> Location code is unique and its guaranteed that we don't clash here.
>> Also this is platform specific LEDs and I don't see any value by
>> prefixing "powernv". Hence I think its fine with current format.
>>
>> Sample location code : U78C9.001.RST0027-P1-C1
>>    Here "U78C9.001.RST0027" represents the system model
>>               P1 - Planar 1
>>               C1 - Card 1
> 
> I consulted ePAPR standard and all the characters used for location code
> are allowed. Nonetheless AFAIK no existing bindings use uppercase for
> node name.
> 

These are PowerNV platform specific properties..


> 
> We will need DT maintainer's ack here too, so cc DT list.
> Please CC devicetree at vger.kernel.org next time, especially for
> DT documentation patch.
> 

Sure. Next time I will CC DT mailing list.

@Ben,
  Any comments?

>> .../...
>>
>>>> +led {
>>>> +    compatible = "ibm,opal-v3-led";
>>>> +    phandle = <0x1000006b>;
>>>> +    linux,phandle = <0x1000006b>;
>>>> +    led-mode = "lightpath";
>>>> +
>>>> +    U78C9.001.RST0027-P1-C1 {
>>>
>>> DT node names should be in lowercase form.
>>> Is U78C9.001.RST0027-P1-C1 a location code?
>>
>> Yes ... Node presents the location code.. Platform provides these
>> location code in upper case. Hence our OPAL firmware exposes them in
>> upper case.
>>
>>>
>>>> +        led-types = "identify", "fault";
>>>> +        led-loc = "descendent";
>>>> +        phandle = <0x1000006f>;
>>>> +        linux,phandle = <0x1000006f>;
> 
> I think that all these properties will not be needed.

These properties are coming from our firmware which is static.

phandle property is default .. which is required to uniquely identify node
inside  DT.
led-loc property is not used today. If its not going to be used forever then I
can request
FW folks to remove it later.


> 
>>>> +    };
>>>> +    ...
>>>> +    ...
>>>> +};
>>>> +
>>>> +
>>>> +'compatible' property describes LEDs compatibility.
>>>
>>> compatible doesn't need to be mentioned here.
>>
>> Removed.
>>
>>>
>>>> +'led-mode' property describes service indicator mode
>>>> (lightpath/guidinglight).
>>>
>>> You don't parse this in the driver? What is its purpose?
>>
>> As mentioned in other thread, currently our driver doesn't parse
>> this. (As mode is provided by platform and its static).
>>
>> Do you want me to elaborate this property here?
> 
> I don't think so. At least for now :)

Ok.

> 
>> .../...
>>
>>>> +
>>>> +/*
>>>> + * 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;
>>>
>>> I think that we have to make it configurable from the Device Tree
>>> level. There could be a 'retain-state-removed' property introduced.
>>
>> Hmmm.. In my case, LED behavior is not going to change. Hence I don't
>> think we need this property.
> 
> Can you guarantee that the next version of the platform will have also
> the same requirements? What if it would be made available to choose
> that the LEDs are software-only controlled?

This design exists for ages.. IMO if we change this to software control (or
dynamic property which can be changed during runtime) then it completely defeats
the LED purpose. Hence I don't think it will change anytime.

> 
> If these are unjustified doubts, I'm ok with leaving the static
> variable.
> 
>>>
>>> There is also another implication - you define LED_CORE_SUSPENDRESUME
>>> flag. With current approach it wouldn't turn the led off on suspend.
>>> Please either drop the flag or assure correct behaviour on suspend.
>>
>> I will drop this.
>>
>>> You can also make this configurable like retain-state-suspended
>>> property in Documentation/devicetree/bindings/leds/leds-gpio.txt.
>>>
>>
>> .../...
>>
>>>> +
>>>> +    if (powernv_get_loc_code(led_type, loc_code) != 0)
>>>> +        goto out_loc;
>>>> +
>>>> +    /* Prepare for the OPAL call */
>>>> +    max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>>>> +    led_mask = OPAL_SLOT_LED_STATE_ON << led_type;
>>>> +    if (value)
>>>> +        led_value = OPAL_SLOT_LED_STATE_ON << led_type;
>>>
>>>          led_value = led_mask;
>>
>> Fixed.
>>
>> .../...
>>
>>>> +
>>>> +/* Schedule work to update LED state */
>>>> +static void powernv_led_set_queue(struct led_classdev *led_cdev,
>>>> +                  enum led_brightness value, u64 led_type)
>>>
>>> It doesn't set any queue. It should be powernv_brightness_set.
>>
>> Make sense. Fixed.
>>
>> .../...
>>
>>>> +    /* Create the name for classdev */
>>>> +    switch (led_type) {
>>>> +    case OPAL_SLOT_LED_TYPE_ATTN:
>>>> +        powernv_led->cdev.name = kasprintf(GFP_KERNEL,
>>>> +                           "%s%s", node->name,
>>>> +                           POWERNV_LED_ATTN);
>>>
>>> LED name should be taken from of_node name or label property
>>> directly. Please refer to the other LED class drivers.
>>
>> Hmmm.. Our  current FW provides led-types property which tells
>> available type for given location code..
>> like
>>    led-types = "identify" "fault"..
>>
>> Can I use this ? So the names will be
>>     <location_code>:<identify|fault|attention>
> 
> I adressed this in the other message.
> 
>> And if this approach fine,  I can fix (assuming maintainer agrees :-)
>> ) firmware side to change identify -> ident.
> 
> Let's leave it. It seems to be platform specific.

Sure.

Thanks!
-Vasant