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

[Jacek Anaszewski]

Hi Vasant,

On 04/16/2015 08:52 AM, Vasant Hegde wrote:
> 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.

Thanks for this explanation.

I am changing my mind about these LEDs. Since they can be controlled
from hardware without any system interaction, then turning them off
on driver removal makes no sense. The LEDs could be turned on again even
if the driver is unloaded.
Since the driver doesn't perform any initialization of the LEDs,
neither should it turn them off on removal.

If I understand this correctly, then the solution with variable would
do and no sysfs attribute would be required.

-- 
Best Regards,
Jacek Anaszewski