Resending, as there were some problems with delivering this message.
-------- Original Message --------
Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Date: Thu, 16 Apr 2015 13:34:17 +0200
From: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
To: Vasant Hegde <hegdevasant@xxxxxxxxxxxxxxxxxx>
CC: linuxppc-dev@xxxxxxxxxxxxxxxx, linux-leds@xxxxxxxxxxxxxxx,
stewart@xxxxxxxxxxxxxxxxxx, mpe@xxxxxxxxxxxxxx, cooloney@xxxxxxxxx,
rpurdie@xxxxxxxxx, khandual@xxxxxxxxxxxxxxxxxx
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
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.
Jacek,
Thanks. Then I will retain current method.
One question..What is the preferred way to name LED node in this case ?
<location_code>:<ATTENTION|IDENTIFY|FAULT>
OR
<location_code>
ident <- attribute under each node
fault
attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
label = "powernv1:encl::ident:fault"
}
led2 {
label = "powernv2:desc:attn::fault:"
}
led2 {
label = "powernv3:desc:::fault:"
}
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
