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

[Vasant Hegde <hegdevasant@linux.vnet.ibm.com>]

On 04/20/2015 08:57 PM, Jacek Anaszewski wrote:
> Hi Vasant,
> 

Jacek,

Thanks for the review.

> Thanks for the update.
> 
> On 04/20/2015 10:01 AM, 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 registers classdev structures for all individual LEDs detected on the
> 
> s/registers/register

Fixed.

> 
>> 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. 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

.../...

>> +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>;
>> +    };
>> +    ...
>> +    ...
>> +};
>> +
>> +
>> +'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?

.../...

>> +
>> +/*
>> + * 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.

> 
> 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>

And if this approach fine,  I can fix (assuming maintainer agrees :-) ) firmware
side to change identify -> ident.


.../...


>> +        list_add_tail(&powernv_led->link, &powernv_led_list);
> 
> You don't need a list. powernv_led can be allocated with devm_kzalloc
> and it will be released on remove automatically.


Sure. .will look into it.

-Vasant