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

[Jacek Anaszewski]

On Thu, 23 Apr 2015 10:55:40 +0530
Vasant Hegde <hegdevasant at linux.vnet.ibm.com> wrote:

> 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 .

How the firmware is related to kernel? These bindings are for kernel,
not for the firmware.

DT bindings are compiled to *.dtb file which is concatenated with
zImage. During system boot device drivers are matched with DT bindings
through 'compatible' property. A driver should have single matching DT
node, i.e. no other driver can probe with the same DT node.
This implies that the node should contain only the properties required
for configuring the related device.

> 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)

I agree, it would be hard to distinguish whether by executing
`echo 0 > brightness` we want to turn off identify or fault function.

>   - point 3: IIUC after duration value expires identify indicator
> reverts.. we don't want to revert until user asks .

From what you shared, blinking has hardware acceleration on OPAL side.
At first timer trigger tries to use HW accelerated blinking by
calling blink_set op and resorts to using software fallback only if
the op fails or is not defined.

BTW timer trigger re-sets blink after timer expires, unless
LED_BLINK_ONESHOT flag is set by LED class device.

>   - point 3: if I use brightness for both identify/fault, how to
> disable these LEDs independently?

Another sysfs attribute would be required, but it would be ugly.

>   - Also how to use trigger property for each LED (if at all we want
> to use them later)?
>

After analyzing pros and cons I think that separate LED class devices
for each LED type would be most suitable solution in this case.

For 'identify' LED the operation would be:

#echo "timer" > trigger		//set 'identify' (blinking)
#cat trigger			//check "identify" state
#none [timer]			//'identify' is ON
#echo 0 > brightness		//unset 'identify
#cat trigger
#[none] timer			//'identify' is OFF

You would have to implement blink_set op
(see Documentation/leds/leds-class.txt and other LED class drivers for
reference).

For attention and fault LEDs only brightness attribute would matter.

DT bindings would look as follows:

opal-leds {
        compatible = "ibm,opal-leds";

        U78C9.001.RST0027-P1-C1:fault {
        };

        U78C9.001.RST0027-P1-C1:indent {
        };

        U78C9.001.RST0027-P2-C1:attn
        };
    }
}


-- 
Best Regards,
Jacek Anaszewski