From: Jacek Anaszewski <j.anaszewski@samsung•com>
To: Vasant Hegde <hegdevasant@linux•vnet.ibm.com>
Cc: stewart@linux•vnet.ibm.com, cooloney@gmail•com,
rpurdie@rpsys•net, linuxppc-dev@lists•ozlabs.org,
linux-leds@vger•kernel.org, khandual@linux•vnet.ibm.com
Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Date: Thu, 16 Apr 2015 13:34:17 +0200 [thread overview]
Message-ID: <552F9E39.4050801@samsung.com> (raw)
In-Reply-To: <552F8E48.6070903@linux.vnet.ibm.com>
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
next prev parent reply other threads:[~2015-04-16 11:34 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-20 11:02 [PATCH v2 0/2] LED interface for PowerNV platform Vasant Hegde
2015-03-20 11:03 ` [PATCH v2 1/2] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states Vasant Hegde
2015-03-20 11:04 ` [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform Vasant Hegde
2015-03-25 5:21 ` Benjamin Herrenschmidt
2015-04-14 5:40 ` Vasant Hegde
2015-04-14 15:20 ` Jacek Anaszewski
2015-04-15 6:26 ` Vasant Hegde
[not found] ` <552E2480.9060102@samsung.com>
2015-04-15 10:15 ` Vasant Hegde
[not found] ` <552E63D2.4070209@samsung.com>
2015-04-16 6:52 ` Vasant Hegde
2015-04-16 8:51 ` Jacek Anaszewski
2015-04-16 10:26 ` Vasant Hegde
2015-04-16 11:34 ` Jacek Anaszewski [this message]
2015-04-20 7:29 ` Jacek Anaszewski
2015-04-20 11:45 ` Jacek Anaszewski
2015-04-20 12:34 ` Vasant Hegde
2015-04-20 15:20 ` Jacek Anaszewski
2015-04-20 15:53 ` Vasant Hegde
2015-04-15 18:50 ` Stewart Smith
2015-04-16 5:07 ` Vasant Hegde
2015-04-21 23:03 ` Stewart Smith
-- strict thread matches above, loose matches on Subject: below --
2015-04-22 21:45 Jacek Anaszewski
2015-04-23 5:25 ` Vasant Hegde
2015-04-23 14:13 ` Jacek Anaszewski
2015-04-24 4:18 ` Stewart Smith
2015-04-24 10:16 ` Jacek Anaszewski
2015-04-28 6:59 ` Stewart Smith
2015-04-28 9:10 ` Jacek Anaszewski
2015-04-24 5:30 ` Vasant Hegde
2015-04-24 10:15 ` Jacek Anaszewski
2015-04-26 22:08 ` Benjamin Herrenschmidt
2015-04-27 11:15 ` Jacek Anaszewski
2015-04-26 22:07 ` Benjamin Herrenschmidt
2015-04-27 7:24 ` Jacek Anaszewski
2015-04-27 9:53 ` Benjamin Herrenschmidt
2015-04-27 11:15 ` Jacek Anaszewski
2015-04-27 13:47 ` Vasant Hegde
2015-04-28 11:06 ` Vasant Hegde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=552F9E39.4050801@samsung.com \
--to=j.anaszewski@samsung$(echo .)com \
--cc=cooloney@gmail$(echo .)com \
--cc=hegdevasant@linux$(echo .)vnet.ibm.com \
--cc=khandual@linux$(echo .)vnet.ibm.com \
--cc=linux-leds@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=rpurdie@rpsys$(echo .)net \
--cc=stewart@linux$(echo .)vnet.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox