On 04/23/2015 03:15 AM, Jacek Anaszewski wrote:
> Hi Vasant,
>
Hi Jacek,
.../...
>>
>>>
>>> From what I can see from the driver code the LEDs are set with:
>>>
>>> opal_leds_set_ind(token, loc_code, led_mask, led_value,
>>> &max_led_type);
>>>
>>> and their state can be read with:
>>>
>>> opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type)
>>>
>>> From the kernel point of view these are very simple operations.
>>>
>>> All the logic you described should be handled by user space.
>>> If you need to be able to specify the LED mode you want to set/read,
>>> then additional 'mode' sysfs attribute should be added by the driver.
>>> There would have to be also additional sysfs attribute
>>> 'available_modes" provided. The ABI documentation should inform how
>>> the mode identifiers map to the modes. I already explained how to
>>> add it, when we were discussing about retaining led state on remove.
>>
>> Sorry..My fault.. I should have elaborated mode operation...
>
> What I was thinking about here was actually LED type, not mode in terms
> of Guiding Light/Light Path. However, please look at the newest
> approach in the end of this message.
>
No problem.
>> I forgot to mention that LED mode is static... meaning platform
>> provides this information, but we cannot change during runtime..
>>
>> Presently we have this information in Device Tree. Since this is
>> static one (and also LED Mode is system wide.. nothing to do
>> individual LED), I didn't add it in LED driver code.. .Do you think
>> we should add that property ?
>
> The property shouldn't be documented at all if it isn't to be used.
Ok . I will remove this.
>
>>>
>>>
>>> I'd see following use cases.
>>>
>>> (let's assume that modes are defined as follows:
>>> 0: ident, 1: attn, 2: fault)
>>
>> Modes are : Guiding Light / Light Path ... which is static and
>> platform provides this information.
>>
>> LED types : IDENT, FAULT and ATTN .... which can be get/set/reset by
>> OS (kernel/userspace)
>>
>> Also only 1 LED can be ATTN ...
>>
>>> #cat available_modes
>>> #0 1 2
>>> #echo 0 > mode //set ident mode
>>> #echo 1 > brightness //set ident state
>>> #echo 2 > mode //set fault mode
>>> #cat brightness //read fault state
>>> #0
>>> #echo 1 > attn //set attn mode
>>> #echo 1 > brightness
>>>
>>> This would set the LED in blinking mode, so I am wondering if we
>>> shouldn't employ timer trigger for this to keep the LED API
>>> consistent.
>>>
>>> Can a single LED support other mode than 'attention'? I'd like to
>>> know if a LED in attention mode (blinking), can be set to some solid
>>> mode?
>>
>> No.. Its always single attention LED/system ... which can be Set
>> (Solid) / reset state.
>
> I confused it with ident.
No problem. We have many hardware specific jargon's which is enough to confuse
anyone :-)
>
>>>
>>> Please let me know if such an approach would still not fit for your
>>> requirements.
>>>
>>
>> Given above conditions, I think current approach (my v3 patchset) is
>> simple and works better. What you say?
>
> Yes, but we still have naming and blinking issues to solve.
>
> Please look at this draft design of device tree node:
>
> opal-leds {
> compatible = "ibm,opal-v3-led";
>
> U78C9.001.RST0027-P1-C1:attn {
> };
>
> U78C9.001.RST0027-P2-C1:identify:fault
> };
>
> U78C9.001.RST0027-P3-C2:identify:fault {
> };
> };
> };
>
> The LED nodes could be empty as the name would convey all the
> required information. The implications would be as follows:
>
These device tree comes from out firmware ... which is immutable .
We can use LED node name + led-type property for naming...which is what I do
currently (v4.. which I haven't posted)
> 1. Each LED would have one corresponding LED class device.
>
> 2. Operations on attn and fault LED types:
> turn on:
> echo 255 > brightness
> turn off:
> echo 0 > brightness
> get status
> cat brightness
>
> 3. Operations on identify LED:
> turn on:
> echo "timer" > trigger
> (blink_set op would have to be implemented in the
> driver)
> turn off:
> echo 0 > brightness
> get status:
> support for this would have to be added to the LED
> subsystem core
I see few issues here.
- Overloading same LED device with multiple opeartion complicates things .. as
these operations can be done independently (say user is allowed to enable both
identify and fault simultaneously)
- point 3: IIUC after duration value expires identify indicator reverts.. we
don't want to revert until user asks .
- point 3: if I use brightness for both identify/fault, how to disable these
LEDs independently?
- Also how to use trigger property for each LED (if at all we want to use them
later)?
>
> 4. Since 'identify' is the platform specific name it could be preserved
>
Ok sure.
>
> Does it work for you?
>
Thanks
Vasant
--
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
