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

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

On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
> On 04/15/2015 12:15 PM, Vasant Hegde wrote:
>> On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
>>> Hi Vasant,

Hi Jacek,

.../...

>>>>>
>>>>
>>>> I mean, we have to retain the state of LED across system reboot.
>>>
>>> Static variables are reinitialized on system reboot, aren't they?
>>
>> Sorry. I think comment was confusing..
>>
>> As I understood, classdev_unregister call resets all LEDs state. However in our
>> case, we don't
>> want to change the LED state during system shutdown/reboot.
>>
>> Hence I have introduced state variable here. So during register call, I just
>> disable LEDs so that any further call will just return. That way we retain LED
>> state even after unloading module.
>>
>> Please let me know if there is any better way to achieve this.
> 
> Since this is not a feature of the device, but rather a use case, then
> it cannot be hard coded in the driver this way. The solution I see is
> introducing a sysfs attribute that would determine if we want the
> LED to be turned off on unregistration or not.

Hmmm. I thought having simple various is enough ..

I don't know the usage of other LED drivers .. Just a thought .. How about
enabling this feature in LED class itself ...Something like:
  - Add new attribute to generic LED class (say persistent)?
  - If drivers wants to retain LED state across reboot, then enable this option
  - During unregister call check for this attribute

> 
> You can refer to the following driver to find out how to add the
> sysfs attribute:
> 
> drivers/leds/leds-lm3533.c
> 
> The attribute will also have to be documented, similarly to these:
> 
> Documentation/ABI/testing/sysfs-class-led-driver-lm3533
> 
> Currently I don't have a good candidate for attribute
> name, so feel free to propose one.

If you don't like generic attribute, then shall I introduce "persistent"
attribute inside my driver. ?
  - By default this attribute is ON and if user wants he can disable this .
  - And I will have another variable (say op_support).. which I will disable in
unload path..
.../...

>>>
>>> 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?
>>
>> Each component (Field Replacement Unit) will have service indicator (LEDS) which
>> can have below states :
>>    - OFF     : no action
>>    - Identify: blinking state (user can use this state to identify particular
>> component).
>>         In Power Systems world we call it as "identify" indicator.. Hence I
>> retained same name here.
>>         How about just "ident" ?
> 
> Sounds good.
> 
>>    - fault : solid state (when component goes bad, LED goes to solid state)
>>       Note that our FW is capable of isolating some of the issues and it can turn
>> on LEDs without OS
>>        interference.
> 
> Does it mean that the LED can be controlled from hardware?
> If so, what would be software use cases then? The same question is
> related to the attn and indent states.

- Identify LEDs:
  We have service processor interface to set/reset this LEDs.. Similar operation
can be done from inband interface as well (via user space tools in Linux)..
Later management layer can make use of this.

- Fault / Attention :
  FW can SET these LEDs if its capable of isolating problem.
  Similarly host/user space can SET these LEDs if then can do fine grained
problem isolation etc.
  Host/user space can RESET these LEDs.

Hence we are adding host support to all the LEDs which can be modified by host.

Hope this clarifies.

-Vasant