Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support

From	Bjorn Andersson <bjorn.andersson@linaro.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support
Date	2016-03-29 17:00 +0200
Message-ID	<ri3hE-7IL-7@gated-at.bofh.it> (permalink)
References	<rfWWZ-8bL-3@gated-at.bofh.it> <rfWWZ-8bL-15@gated-at.bofh.it> <rhbTX-3Lh-1@gated-at.bofh.it> <ri2OB-7x2-5@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Tue 29 Mar 07:20 PDT 2016, Srinivas Kandagatla wrote:

> Thanks Bjorn,
> 
> On 27/03/16 06:50, Bjorn Andersson wrote:
> >On Wed 23 Mar 12:48 PDT 2016, Srinivas Kandagatla wrote:
[..]
> >>+
> >>+			led@5 {
> >>+				label = "dragonboard-600c:yellow:wlan";
> >>+				gpios = <&pm8921_mpps 7 GPIO_ACTIVE_HIGH>;
> >>+				linux,default-trigger = "wlan";
> >
> >This should either be "phy0rx", "phy0tx", "phy0assoc" or "phy0radio". TX
> >does not seem to work, so this should be debugged; "assoc" is probably
> >the one that makes most sense.
> 
> 
> Am ok, to change, did you get activity leds works with any of these strings
> with WLAN or BT?
> 

I did test them; rx, assoc and radio seems to work. Tx does not tickle
the led, but should probably.

I didn't find the spec for the baord, so I'm not sure what the LEDs
should indicate. But it feels like radio or assoc makes most sense, and
follows what we can do with the BT led.

> phy0rx/tx seems to be bit more generic and atleast the name looks bit
> non-specific to wlan.
> 
> These names should be documented somewhere, Its very difficult to find which
> names to use unless you read the code.
> 
> It would be nice to just provide a phandle to the device which led-trigger
> should use, which makes it clear and explicit.
> 

I agree, I was surprised by their naming. The phandle solution would
have been nice, except that you have no way from sysfs to pick which
node to follow.

> 
> 
> >
> >>+				default-state = "off";
> >>+			};
> >>+
> >>+			led@6 {
> >>+				label = "dragonboard-600c:blue:bt";
> >>+				gpios = <&pm8921_mpps 8 GPIO_ACTIVE_HIGH>;
> >>+				linux,default-trigger = "bt";
> >
> >This should be "hci0-power".
> 
> Does this trigger work for you?
> Name is bit misleading though.
> 

I don't think I tested it, but it is the only bt led trigger exposed by
the bluetooth framework. I don't think we have any other hci devices on
this board, so it should be fine.

Regards,
Bjorn

Thread

[PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-03-23 20:50 +0100
  Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support Nicolas Dechesne <nicolas.dechesne@linaro.org> - 2016-03-24 18:00 +0100
    Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds  support Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-03-29 15:40 +0200
  Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds  support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-03-27 08:00 +0200
    Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds  support Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-03-29 16:30 +0200
      Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds  support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-03-29 17:00 +0200
        Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support Nicolas Dechesne <nicolas.dechesne@linaro.org> - 2016-03-29 20:40 +0200
          Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds  support Bjorn Andersson <bjorn.andersson@linaro.org> - 2016-03-29 23:20 +0200
    Re: [PATCH 10/12] ARM: dts: dragonboard-600c: Add on board leds support Rob Herring <robh@kernel.org> - 2016-03-30 15:00 +0200

csiph-web