linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
@ 2020-01-21  6:58 JaeJoon Jung
  2020-01-21 17:20 ` David Abdurachmanov
  2020-01-30  3:35 ` Paul Walmsley
  0 siblings, 2 replies; 7+ messages in thread
From: JaeJoon Jung @ 2020-01-21  6:58 UTC (permalink / raw)
  To: Palmer Dabbelt, Paul Walmsley, Anup Patel; +Cc: linux-riscv

I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.

diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index a2e3d54e830c..b03bf570020c 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi

+                gpio0: gpio@10060000 {
+                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
+                        reg = <0x0 0x10060000 0x0 0x1000>;
+                        reg-names = "control";
+                        gpio-controller;
+                        #gpio-cells = <2>;
+                        ngpios = <16>;
+                        interrupt-controller;
+                        #interrupt-cells = <2>;
+                        interrupt-parent = <&plic0>;
+                        interrupts = <15 16 17 18 19 20 21 22 23 24
25 26 27 28 29 30>;
+                        status = "disabled";
+                };


diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 88cfcb96bf23..f3f55dbbf737 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts

        cpus {
@@ -41,6 +41,39 @@
                clock-frequency = <RTCCLK_FREQ>;
                clock-output-names = "rtcclk";
        };
+
+
+        pwmleds {
+                compatible = "pwm-leds";
+                heartbeat {
+                        label = "led1";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 0 7812500 0>;
+                        linux,default-trigger = "heartbeat";
+                };
+                mtd {
+                        label = "led2";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 1 7812500 0>;
+                        linux,default-trigger = "mtd";
+                };
+                netdev {
+                        label = "led3";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 2 7812500 0>;
+                        linux,default-trigger = "netdev";
+                };
+                panic {
+                        label = "led4";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 3 7812500 0>;
+                        linux,default-trigger = "panic";
+                };
+        };
 };

 &uart0 {
@@ -94,3 +127,7 @@
 &pwm1 {
        status = "okay";
 };
+
+&gpio0 {
+       status = "okay";
+};


If apply above DTS, the gpio-sifive driver works well without source
modification.
drivers/gpio/gpio-sifive.c
drivers/leds/leds-pwm.c

I have checked below:

led1(D1) is acting well as heartbeat.

RISCV-FU540:/sys/class/leds# pwd
/sys/class/leds
RISCV-FU540:/sys/class/leds# ll
total 0
drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
drwxr-xr-x   32 root     root             0 Jan  1 00:00 ../
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led1 ->
../../devices/platform/pwmleds/leds/led1/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led2 ->
../../devices/platform/pwmleds/leds/led2/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led3 ->
../../devices/platform/pwmleds/leds/led3/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led4 ->
../../devices/platform/pwmleds/leds/led4/

RISCV-FU540:/sys/class/leds# cd led3
RISCV-FU540:/sys/class/leds/led3# ll
total 0
drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
drwxr-xr-x    6 root     root             0 Jan  1 00:00 ../
-rw-r--r--    1 root     root          4096 Jan  1 00:01 brightness
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 device ->
../../../pwmleds/
-r--r--r--    1 root     root          4096 Jan  1 00:00 max_brightness
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 subsystem ->
../../../../../class/leds/
-rw-r--r--    1 root     root          4096 Jan  1 00:00 trigger
-rw-r--r--    1 root     root          4096 Jan  1 00:00 uevent
RISCV-FU540:/sys/class/leds/led3# echo 1 > brightness
RISCV-FU540:/sys/class/leds/led3# echo 127 > brightness
RISCV-FU540:/sys/class/leds/led3# echo 255 > brightness

leds(D1, D2, D3, D4) are acting well as pwm brightness.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-21  6:58 [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/) JaeJoon Jung
@ 2020-01-21 17:20 ` David Abdurachmanov
  2020-01-30  3:35 ` Paul Walmsley
  1 sibling, 0 replies; 7+ messages in thread
From: David Abdurachmanov @ 2020-01-21 17:20 UTC (permalink / raw)
  To: JaeJoon Jung; +Cc: linux-riscv, Anup Patel, Palmer Dabbelt, Paul Walmsley

I have a similar patch, but still using old GPIO driver (non
upstream). See comments below.

On Tue, Jan 21, 2020 at 8:59 AM JaeJoon Jung <rgbi3307@gmail.com> wrote:
>
> I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
>
> diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> index a2e3d54e830c..b03bf570020c 100644
> --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
>
> +                gpio0: gpio@10060000 {
> +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> +                        reg = <0x0 0x10060000 0x0 0x1000>;
> +                        reg-names = "control";
> +                        gpio-controller;
> +                        #gpio-cells = <2>;
> +                        ngpios = <16>;
> +                        interrupt-controller;
> +                        #interrupt-cells = <2>;
> +                        interrupt-parent = <&plic0>;
> +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> 25 26 27 28 29 30>;
> +                        status = "disabled";
> +                };
>
>
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 88cfcb96bf23..f3f55dbbf737 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
>
>         cpus {
> @@ -41,6 +41,39 @@
>                 clock-frequency = <RTCCLK_FREQ>;
>                 clock-output-names = "rtcclk";
>         };
> +
> +
> +        pwmleds {
> +                compatible = "pwm-leds";
> +                heartbeat {
> +                        label = "led1";

Could we have labels based on schematics and prints on PCB?

I use:

label = "green:d1";
label = "green:d2";
label = "green:d3";
label = "green:d4";

> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 0 7812500 0>;
> +                        linux,default-trigger = "heartbeat";
> +                };
> +                mtd {
> +                        label = "led2";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 1 7812500 0>;
> +                        linux,default-trigger = "mtd";
> +                };
> +                netdev {

Just a quick note. For this to work properly you also need to have udev rule.
I think, there was a patch posted in 2019 to enable netdev
configuration in DTS to avoid having configuration via udev or
similar, but that wasn't merged the last time I checked (late 2019).

See:
[PATCH 4/4] leds: netdev trigger: allow setting initial values in device tree
https://www.spinics.net/lists/netdev/msg557736.html

> +                        label = "led3";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 2 7812500 0>;
> +                        linux,default-trigger = "netdev";
> +                };
> +                panic {

I find that panic PWM LED doesn't actually work for me.
IIUC this might only work for GPIO attached LEDs based on comments I found.
I have this LED remapped for mmc0 activity:

mmc0 {
  label = "green:d4";
  pwms = <&pwm0 3 2000000 0>;
  max-brightness = <255>;
  linux,default-trigger = "mmc0";
};

> +                        label = "led4";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 3 7812500 0>;
> +                        linux,default-trigger = "panic";
> +                };
> +        };
>  };
>
>  &uart0 {
> @@ -94,3 +127,7 @@
>  &pwm1 {
>         status = "okay";
>  };
> +
> +&gpio0 {
> +       status = "okay";
> +};
>
>
> If apply above DTS, the gpio-sifive driver works well without source
> modification.
> drivers/gpio/gpio-sifive.c
> drivers/leds/leds-pwm.c
>
> I have checked below:
>
> led1(D1) is acting well as heartbeat.
>
> RISCV-FU540:/sys/class/leds# pwd
> /sys/class/leds
> RISCV-FU540:/sys/class/leds# ll
> total 0
> drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
> drwxr-xr-x   32 root     root             0 Jan  1 00:00 ../
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led1 ->
> ../../devices/platform/pwmleds/leds/led1/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led2 ->
> ../../devices/platform/pwmleds/leds/led2/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led3 ->
> ../../devices/platform/pwmleds/leds/led3/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led4 ->
> ../../devices/platform/pwmleds/leds/led4/
>
> RISCV-FU540:/sys/class/leds# cd led3
> RISCV-FU540:/sys/class/leds/led3# ll
> total 0
> drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
> drwxr-xr-x    6 root     root             0 Jan  1 00:00 ../
> -rw-r--r--    1 root     root          4096 Jan  1 00:01 brightness
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 device ->
> ../../../pwmleds/
> -r--r--r--    1 root     root          4096 Jan  1 00:00 max_brightness
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 subsystem ->
> ../../../../../class/leds/
> -rw-r--r--    1 root     root          4096 Jan  1 00:00 trigger
> -rw-r--r--    1 root     root          4096 Jan  1 00:00 uevent
> RISCV-FU540:/sys/class/leds/led3# echo 1 > brightness
> RISCV-FU540:/sys/class/leds/led3# echo 127 > brightness
> RISCV-FU540:/sys/class/leds/led3# echo 255 > brightness
>
> leds(D1, D2, D3, D4) are acting well as pwm brightness.
>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-21  6:58 [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/) JaeJoon Jung
  2020-01-21 17:20 ` David Abdurachmanov
@ 2020-01-30  3:35 ` Paul Walmsley
  2020-01-30  6:23   ` JaeJoon Jung
  1 sibling, 1 reply; 7+ messages in thread
From: Paul Walmsley @ 2020-01-30  3:35 UTC (permalink / raw)
  To: JaeJoon Jung; +Cc: linux-riscv, Anup Patel, Palmer Dabbelt

On Tue, 21 Jan 2020, JaeJoon Jung wrote:

> I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> 
> diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> index a2e3d54e830c..b03bf570020c 100644
> --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> 
> +                gpio0: gpio@10060000 {
> +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> +                        reg = <0x0 0x10060000 0x0 0x1000>;
> +                        reg-names = "control";
> +                        gpio-controller;
> +                        #gpio-cells = <2>;
> +                        ngpios = <16>;
> +                        interrupt-controller;
> +                        #interrupt-cells = <2>;
> +                        interrupt-parent = <&plic0>;
> +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> 25 26 27 28 29 30>;
> +                        status = "disabled";
> +                };

Yash posted this a while ago:

https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf


> 
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 88cfcb96bf23..f3f55dbbf737 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> 
>         cpus {
> @@ -41,6 +41,39 @@
>                 clock-frequency = <RTCCLK_FREQ>;
>                 clock-output-names = "rtcclk";
>         };
> +
> +
> +        pwmleds {
> +                compatible = "pwm-leds";
> +                heartbeat {
> +                        label = "led1";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 0 7812500 0>;
> +                        linux,default-trigger = "heartbeat";
> +                };
> +                mtd {
> +                        label = "led2";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 1 7812500 0>;
> +                        linux,default-trigger = "mtd";
> +                };
> +                netdev {
> +                        label = "led3";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 2 7812500 0>;
> +                        linux,default-trigger = "netdev";
> +                };
> +                panic {
> +                        label = "led4";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 3 7812500 0>;
> +                        linux,default-trigger = "panic";
> +                };
> +        };
>  };

I don't think it's good to add these pwmleds to the default board DTS 
file.  I realize that many upstream ARM development boards have added this 
type of configuration, but it gets in the way of reusing this DT file when 
integrators wish to have the LEDs used for different purposes.  If the 
Unleashed silkscreen had text on it describing the LEDs as having these 
specific functions, or if Unleashed was sold in a case with similar 
markings on the outside, it'd be a different story, and then a change like 
the above could make sense.


- Paul


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-30  3:35 ` Paul Walmsley
@ 2020-01-30  6:23   ` JaeJoon Jung
  2020-01-30 16:13     ` David Abdurachmanov
  0 siblings, 1 reply; 7+ messages in thread
From: JaeJoon Jung @ 2020-01-30  6:23 UTC (permalink / raw)
  To: Paul Walmsley; +Cc: linux-riscv, Anup Patel, Palmer Dabbelt

I agree with you because LEDs are user defined using method.

And, I am sorry about that I confirmed lately below posted by Yash.

https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf

It is helpful for me and I am testing it.
If I find a bug, I am going to share about it.

Thanks a lot, Have a nice day.

Yours,
JaeJoon Jung

On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote:
>
> On Tue, 21 Jan 2020, JaeJoon Jung wrote:
>
> > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> >
> > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > index a2e3d54e830c..b03bf570020c 100644
> > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> >
> > +                gpio0: gpio@10060000 {
> > +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> > +                        reg = <0x0 0x10060000 0x0 0x1000>;
> > +                        reg-names = "control";
> > +                        gpio-controller;
> > +                        #gpio-cells = <2>;
> > +                        ngpios = <16>;
> > +                        interrupt-controller;
> > +                        #interrupt-cells = <2>;
> > +                        interrupt-parent = <&plic0>;
> > +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> > 25 26 27 28 29 30>;
> > +                        status = "disabled";
> > +                };
>
> Yash posted this a while ago:
>
> https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
>
>
> >
> > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > index 88cfcb96bf23..f3f55dbbf737 100644
> > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> >
> >         cpus {
> > @@ -41,6 +41,39 @@
> >                 clock-frequency = <RTCCLK_FREQ>;
> >                 clock-output-names = "rtcclk";
> >         };
> > +
> > +
> > +        pwmleds {
> > +                compatible = "pwm-leds";
> > +                heartbeat {
> > +                        label = "led1";
> > +                        max-brightness = <255>;
> > +                        active-low = <1>;
> > +                        pwms = <&pwm0 0 7812500 0>;
> > +                        linux,default-trigger = "heartbeat";
> > +                };
> > +                mtd {
> > +                        label = "led2";
> > +                        max-brightness = <255>;
> > +                        active-low = <1>;
> > +                        pwms = <&pwm0 1 7812500 0>;
> > +                        linux,default-trigger = "mtd";
> > +                };
> > +                netdev {
> > +                        label = "led3";
> > +                        max-brightness = <255>;
> > +                        active-low = <1>;
> > +                        pwms = <&pwm0 2 7812500 0>;
> > +                        linux,default-trigger = "netdev";
> > +                };
> > +                panic {
> > +                        label = "led4";
> > +                        max-brightness = <255>;
> > +                        active-low = <1>;
> > +                        pwms = <&pwm0 3 7812500 0>;
> > +                        linux,default-trigger = "panic";
> > +                };
> > +        };
> >  };
>
> I don't think it's good to add these pwmleds to the default board DTS
> file.  I realize that many upstream ARM development boards have added this
> type of configuration, but it gets in the way of reusing this DT file when
> integrators wish to have the LEDs used for different purposes.  If the
> Unleashed silkscreen had text on it describing the LEDs as having these
> specific functions, or if Unleashed was sold in a case with similar
> markings on the outside, it'd be a different story, and then a change like
> the above could make sense.
>
>
> - Paul


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-30  6:23   ` JaeJoon Jung
@ 2020-01-30 16:13     ` David Abdurachmanov
  2020-01-30 17:20       ` JaeJoon Jung
  0 siblings, 1 reply; 7+ messages in thread
From: David Abdurachmanov @ 2020-01-30 16:13 UTC (permalink / raw)
  To: JaeJoon Jung; +Cc: linux-riscv, Anup Patel, Palmer Dabbelt, Paul Walmsley

On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote:
>
> I agree with you because LEDs are user defined using method.
>
> And, I am sorry about that I confirmed lately below posted by Yash.
>
> https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
>
> It is helpful for me and I am testing it.
> If I find a bug, I am going to share about it.
>
> Thanks a lot, Have a nice day.
>
> Yours,
> JaeJoon Jung
>
> On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> >
> > On Tue, 21 Jan 2020, JaeJoon Jung wrote:
> >
> > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> > >
> > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > index a2e3d54e830c..b03bf570020c 100644
> > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > >
> > > +                gpio0: gpio@10060000 {
> > > +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> > > +                        reg = <0x0 0x10060000 0x0 0x1000>;
> > > +                        reg-names = "control";
> > > +                        gpio-controller;
> > > +                        #gpio-cells = <2>;
> > > +                        ngpios = <16>;
> > > +                        interrupt-controller;
> > > +                        #interrupt-cells = <2>;
> > > +                        interrupt-parent = <&plic0>;
> > > +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> > > 25 26 27 28 29 30>;
> > > +                        status = "disabled";
> > > +                };
> >
> > Yash posted this a while ago:
> >
> > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> >
> >
> > >
> > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > index 88cfcb96bf23..f3f55dbbf737 100644
> > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > >
> > >         cpus {
> > > @@ -41,6 +41,39 @@
> > >                 clock-frequency = <RTCCLK_FREQ>;
> > >                 clock-output-names = "rtcclk";
> > >         };
> > > +
> > > +
> > > +        pwmleds {
> > > +                compatible = "pwm-leds";
> > > +                heartbeat {
> > > +                        label = "led1";
> > > +                        max-brightness = <255>;
> > > +                        active-low = <1>;
> > > +                        pwms = <&pwm0 0 7812500 0>;
> > > +                        linux,default-trigger = "heartbeat";
> > > +                };
> > > +                mtd {
> > > +                        label = "led2";
> > > +                        max-brightness = <255>;
> > > +                        active-low = <1>;
> > > +                        pwms = <&pwm0 1 7812500 0>;
> > > +                        linux,default-trigger = "mtd";
> > > +                };
> > > +                netdev {
> > > +                        label = "led3";
> > > +                        max-brightness = <255>;
> > > +                        active-low = <1>;
> > > +                        pwms = <&pwm0 2 7812500 0>;
> > > +                        linux,default-trigger = "netdev";
> > > +                };
> > > +                panic {
> > > +                        label = "led4";
> > > +                        max-brightness = <255>;
> > > +                        active-low = <1>;
> > > +                        pwms = <&pwm0 3 7812500 0>;
> > > +                        linux,default-trigger = "panic";
> > > +                };
> > > +        };
> > >  };
> >
> > I don't think it's good to add these pwmleds to the default board DTS
> > file.  I realize that many upstream ARM development boards have added this
> > type of configuration, but it gets in the way of reusing this DT file when
> > integrators wish to have the LEDs used for different purposes.  If the
> > Unleashed silkscreen had text on it describing the LEDs as having these
> > specific functions, or if Unleashed was sold in a case with similar
> > markings on the outside, it'd be a different story, and then a change like
> > the above could make sense.

I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist.
So let's define those LEDs, but disable them (i.e. no default trigger). In that
case user has an options to write udev rules to setup those PWM LEDs as
he/she likes. I already use udev rule for netdev LED configuration as today
DTS does not provide options to configure it [within DTS] (patch
posted in 2019).

It would look something like below (not tested). We use labels from PCB and
schematics, but do not assign any default functions/triggers. Once the
distro boots
udev rules will set default triggers and do required configuration.

---
 .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
index 828f406..eb793b5 100644
--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
@@ -26,6 +26,33 @@
        };

        soc {
+               pwmleds {
+                       compatible = "pwm-leds";
+                       d1 {
+                               label = "green:d1";
+                               pwms = <&pwm0 0 2000000 0>;
+                               max-brightness = <255>;
+                               linux,default-trigger = "none";
+                       };
+                       d2 {
+                               label = "green:d2";
+                               pwms = <&pwm0 1 2000000 0>;
+                               max-brightness = <255>;
+                               linux,default-trigger = "none";
+                       };
+                       d3 {
+                               label = "green:d3";
+                               pwms = <&pwm0 2 2000000 0>;
+                               max-brightness = <255>;
+                               linux,default-trigger = "none";
+                       };
+                       d4 {
+                               label = "green:d4";
+                               pwms = <&pwm0 3 2000000 0>;
+                               max-brightness = <255>;
+                               linux,default-trigger = "none";
+                       };
+               };
        };

        hfclk: hfclk {
--
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-30 16:13     ` David Abdurachmanov
@ 2020-01-30 17:20       ` JaeJoon Jung
  2020-01-31 13:20         ` David Abdurachmanov
  0 siblings, 1 reply; 7+ messages in thread
From: JaeJoon Jung @ 2020-01-30 17:20 UTC (permalink / raw)
  To: David Abdurachmanov
  Cc: linux-riscv, Anup Patel, Palmer Dabbelt, Paul Walmsley

Hello David Abdurachmanov,
Thanks to your deep opinions,
but it's not that important, but there are things to check.
If you apply below DTS, It works all fine without source modification.
I think that you are missing the active-low = <1> and
linux,default-trigger = "heartbeat"...

The following setting is not set by default, but it works well if you set it.
CONFIG_PWM_SIFIVE
CONFIG_NEW_LEDS
CONFIG_LEDS_PWM
CONFIG_LEDS_CLASS
CONFIG_LEDS_TRIGGERS
CONFIG_LEDS_TRIGGER_HEARTBEAT

--- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
+++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts

        cpus {
@@ -41,6 +41,39 @@
                clock-frequency = <RTCCLK_FREQ>;
                clock-output-names = "rtcclk";
        };
+
+
+        pwmleds {
+                compatible = "pwm-leds";
+                heartbeat {
+                        label = "led1";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 0 1000000 0>;
+                        linux,default-trigger = "heartbeat";
+                };
+                mtd {
+                        label = "led2";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 1 1000000 0>;
+                        linux,default-trigger = "mtd";
+                };
+                netdev {
+                        label = "led3";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 2 1000000 0>;
+                        linux,default-trigger = "netdev";
+                };
+                panic {
+                        label = "led4";
+                        max-brightness = <255>;
+                        active-low = <1>;
+                        pwms = <&pwm0 3 1000000 0>;
+                        linux,default-trigger = "panic";
+                };
+        };
 };

And I think that the simpler the name, the better.
labels are displayed at /sys/class/leds and user can access and control it here.

RISCV-FU540:/sys/class/leds# pwd
/sys/class/leds
RISCV-FU540:/sys/class/leds# ll
total 0
drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
drwxr-xr-x   33 root     root             0 Jan  1 00:00 ../
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led1 ->
../../devices/platform/pwmleds/leds/led1/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led2 ->
../../devices/platform/pwmleds/leds/led2/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led3 ->
../../devices/platform/pwmleds/leds/led3/
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led4 ->
../../devices/platform/pwmleds/leds/led4/

RISCV-FU540:/sys/class/leds# cd led1
RISCV-FU540:/sys/class/leds/led1# ll
total 0
drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
drwxr-xr-x    6 root     root             0 Jan  1 00:00 ../
-rw-r--r--    1 root     root          4096 Jan  1 00:00 brightness
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 device ->
../../../pwmleds/
-r--r--r--    1 root     root          4096 Jan  1 00:00 max_brightness
lrwxrwxrwx    1 root     root             0 Jan  1 00:00 subsystem ->
../../../../../class/leds/
-rw-r--r--    1 root     root             0 Jan  1 00:00 trigger
-rw-r--r--    1 root     root          4096 Jan  1 00:00 uevent

Yours,
JaeJoon Jung


On Fri, 31 Jan 2020 at 01:14, David Abdurachmanov
<david.abdurachmanov@gmail.com> wrote:
>
> On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote:
> >
> > I agree with you because LEDs are user defined using method.
> >
> > And, I am sorry about that I confirmed lately below posted by Yash.
> >
> > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> >
> > It is helpful for me and I am testing it.
> > If I find a bug, I am going to share about it.
> >
> > Thanks a lot, Have a nice day.
> >
> > Yours,
> > JaeJoon Jung
> >
> > On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > >
> > > On Tue, 21 Jan 2020, JaeJoon Jung wrote:
> > >
> > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> > > >
> > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > index a2e3d54e830c..b03bf570020c 100644
> > > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > >
> > > > +                gpio0: gpio@10060000 {
> > > > +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> > > > +                        reg = <0x0 0x10060000 0x0 0x1000>;
> > > > +                        reg-names = "control";
> > > > +                        gpio-controller;
> > > > +                        #gpio-cells = <2>;
> > > > +                        ngpios = <16>;
> > > > +                        interrupt-controller;
> > > > +                        #interrupt-cells = <2>;
> > > > +                        interrupt-parent = <&plic0>;
> > > > +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> > > > 25 26 27 28 29 30>;
> > > > +                        status = "disabled";
> > > > +                };
> > >
> > > Yash posted this a while ago:
> > >
> > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> > >
> > >
> > > >
> > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > index 88cfcb96bf23..f3f55dbbf737 100644
> > > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > >
> > > >         cpus {
> > > > @@ -41,6 +41,39 @@
> > > >                 clock-frequency = <RTCCLK_FREQ>;
> > > >                 clock-output-names = "rtcclk";
> > > >         };
> > > > +
> > > > +
> > > > +        pwmleds {
> > > > +                compatible = "pwm-leds";
> > > > +                heartbeat {
> > > > +                        label = "led1";
> > > > +                        max-brightness = <255>;
> > > > +                        active-low = <1>;
> > > > +                        pwms = <&pwm0 0 7812500 0>;
> > > > +                        linux,default-trigger = "heartbeat";
> > > > +                };
> > > > +                mtd {
> > > > +                        label = "led2";
> > > > +                        max-brightness = <255>;
> > > > +                        active-low = <1>;
> > > > +                        pwms = <&pwm0 1 7812500 0>;
> > > > +                        linux,default-trigger = "mtd";
> > > > +                };
> > > > +                netdev {
> > > > +                        label = "led3";
> > > > +                        max-brightness = <255>;
> > > > +                        active-low = <1>;
> > > > +                        pwms = <&pwm0 2 7812500 0>;
> > > > +                        linux,default-trigger = "netdev";
> > > > +                };
> > > > +                panic {
> > > > +                        label = "led4";
> > > > +                        max-brightness = <255>;
> > > > +                        active-low = <1>;
> > > > +                        pwms = <&pwm0 3 7812500 0>;
> > > > +                        linux,default-trigger = "panic";
> > > > +                };
> > > > +        };
> > > >  };
> > >
> > > I don't think it's good to add these pwmleds to the default board DTS
> > > file.  I realize that many upstream ARM development boards have added this
> > > type of configuration, but it gets in the way of reusing this DT file when
> > > integrators wish to have the LEDs used for different purposes.  If the
> > > Unleashed silkscreen had text on it describing the LEDs as having these
> > > specific functions, or if Unleashed was sold in a case with similar
> > > markings on the outside, it'd be a different story, and then a change like
> > > the above could make sense.
>
> I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist.
> So let's define those LEDs, but disable them (i.e. no default trigger). In that
> case user has an options to write udev rules to setup those PWM LEDs as
> he/she likes. I already use udev rule for netdev LED configuration as today
> DTS does not provide options to configure it [within DTS] (patch
> posted in 2019).
>
> It would look something like below (not tested). We use labels from PCB and
> schematics, but do not assign any default functions/triggers. Once the
> distro boots
> udev rules will set default triggers and do required configuration.
>
> ---
>  .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> index 828f406..eb793b5 100644
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> @@ -26,6 +26,33 @@
>         };
>
>         soc {
> +               pwmleds {
> +                       compatible = "pwm-leds";
> +                       d1 {
> +                               label = "green:d1";
> +                               pwms = <&pwm0 0 2000000 0>;
> +                               max-brightness = <255>;
> +                               linux,default-trigger = "none";
> +                       };
> +                       d2 {
> +                               label = "green:d2";
> +                               pwms = <&pwm0 1 2000000 0>;
> +                               max-brightness = <255>;
> +                               linux,default-trigger = "none";
> +                       };
> +                       d3 {
> +                               label = "green:d3";
> +                               pwms = <&pwm0 2 2000000 0>;
> +                               max-brightness = <255>;
> +                               linux,default-trigger = "none";
> +                       };
> +                       d4 {
> +                               label = "green:d4";
> +                               pwms = <&pwm0 3 2000000 0>;
> +                               max-brightness = <255>;
> +                               linux,default-trigger = "none";
> +                       };
> +               };
>         };
>
>         hfclk: hfclk {
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
  2020-01-30 17:20       ` JaeJoon Jung
@ 2020-01-31 13:20         ` David Abdurachmanov
  0 siblings, 0 replies; 7+ messages in thread
From: David Abdurachmanov @ 2020-01-31 13:20 UTC (permalink / raw)
  To: JaeJoon Jung; +Cc: linux-riscv, Anup Patel, Palmer Dabbelt, Paul Walmsley

On Thu, Jan 30, 2020 at 6:20 PM JaeJoon Jung <rgbi3307@gmail.com> wrote:
>
> Hello David Abdurachmanov,
> Thanks to your deep opinions,
> but it's not that important, but there are things to check.
> If you apply below DTS, It works all fine without source modification.
> I think that you are missing the active-low = <1> and

Everything works fine for me as-is, but I am using the old (non-upstream)
GPIO driver right now. I am rebasing to a new one.

> linux,default-trigger = "heartbeat"...

The idea is not to do that (even if it's documented in SiFive Unleashed
Manual). We leave the decision of triggers to udev rules (distro) and a user.
You can change the trigger via sysfs knobs as a user (I tried it).

Without these entries you don't have those PWM LEDs available on sysfs
for udev/user to configure triggers.

Just reminder again. We just upstream the fact that PWM LEDs exist on
Unleashed, but not what they do. By default they do nothing until configured
by udev rules (distro) or  a user (e.g. via sysfs knobs directly).

Example rules I use:

SUBSYSTEM=="leds", KERNEL=="green:d3", ATTR{link}="1", ATTR{tx}="1",
ATTR{rx}="1", ATTR{device_name}="eth0"

This does require having PWM LEDs in DTS.

david

>
> The following setting is not set by default, but it works well if you set it.
> CONFIG_PWM_SIFIVE
> CONFIG_NEW_LEDS
> CONFIG_LEDS_PWM
> CONFIG_LEDS_CLASS
> CONFIG_LEDS_TRIGGERS
> CONFIG_LEDS_TRIGGER_HEARTBEAT
>
> --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
>
>         cpus {
> @@ -41,6 +41,39 @@
>                 clock-frequency = <RTCCLK_FREQ>;
>                 clock-output-names = "rtcclk";
>         };
> +
> +
> +        pwmleds {
> +                compatible = "pwm-leds";
> +                heartbeat {
> +                        label = "led1";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 0 1000000 0>;
> +                        linux,default-trigger = "heartbeat";
> +                };
> +                mtd {
> +                        label = "led2";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 1 1000000 0>;
> +                        linux,default-trigger = "mtd";
> +                };
> +                netdev {
> +                        label = "led3";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 2 1000000 0>;
> +                        linux,default-trigger = "netdev";
> +                };
> +                panic {
> +                        label = "led4";
> +                        max-brightness = <255>;
> +                        active-low = <1>;
> +                        pwms = <&pwm0 3 1000000 0>;
> +                        linux,default-trigger = "panic";
> +                };
> +        };
>  };
>
> And I think that the simpler the name, the better.
> labels are displayed at /sys/class/leds and user can access and control it here.
>
> RISCV-FU540:/sys/class/leds# pwd
> /sys/class/leds
> RISCV-FU540:/sys/class/leds# ll
> total 0
> drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
> drwxr-xr-x   33 root     root             0 Jan  1 00:00 ../
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led1 ->
> ../../devices/platform/pwmleds/leds/led1/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led2 ->
> ../../devices/platform/pwmleds/leds/led2/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led3 ->
> ../../devices/platform/pwmleds/leds/led3/
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 led4 ->
> ../../devices/platform/pwmleds/leds/led4/
>
> RISCV-FU540:/sys/class/leds# cd led1
> RISCV-FU540:/sys/class/leds/led1# ll
> total 0
> drwxr-xr-x    2 root     root             0 Jan  1 00:00 ./
> drwxr-xr-x    6 root     root             0 Jan  1 00:00 ../
> -rw-r--r--    1 root     root          4096 Jan  1 00:00 brightness
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 device ->
> ../../../pwmleds/
> -r--r--r--    1 root     root          4096 Jan  1 00:00 max_brightness
> lrwxrwxrwx    1 root     root             0 Jan  1 00:00 subsystem ->
> ../../../../../class/leds/
> -rw-r--r--    1 root     root             0 Jan  1 00:00 trigger
> -rw-r--r--    1 root     root          4096 Jan  1 00:00 uevent
>
> Yours,
> JaeJoon Jung
>
>
> On Fri, 31 Jan 2020 at 01:14, David Abdurachmanov
> <david.abdurachmanov@gmail.com> wrote:
> >
> > On Thu, Jan 30, 2020 at 7:24 AM JaeJoon Jung <rgbi3307@gmail.com> wrote:
> > >
> > > I agree with you because LEDs are user defined using method.
> > >
> > > And, I am sorry about that I confirmed lately below posted by Yash.
> > >
> > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> > >
> > > It is helpful for me and I am testing it.
> > > If I find a bug, I am going to share about it.
> > >
> > > Thanks a lot, Have a nice day.
> > >
> > > Yours,
> > > JaeJoon Jung
> > >
> > > On Thu, 30 Jan 2020 at 12:35, Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > > >
> > > > On Tue, 21 Jan 2020, JaeJoon Jung wrote:
> > > >
> > > > > I added below DTS to act gpio and pwmleds for SiFive FU540 Unleashed board.
> > > > >
> > > > > diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > > b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > > index a2e3d54e830c..b03bf570020c 100644
> > > > > --- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > > +++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
> > > > >
> > > > > +                gpio0: gpio@10060000 {
> > > > > +                        compatible = "sifive,fu540-c000-gpio", "sifive,gpio0";
> > > > > +                        reg = <0x0 0x10060000 0x0 0x1000>;
> > > > > +                        reg-names = "control";
> > > > > +                        gpio-controller;
> > > > > +                        #gpio-cells = <2>;
> > > > > +                        ngpios = <16>;
> > > > > +                        interrupt-controller;
> > > > > +                        #interrupt-cells = <2>;
> > > > > +                        interrupt-parent = <&plic0>;
> > > > > +                        interrupts = <15 16 17 18 19 20 21 22 23 24
> > > > > 25 26 27 28 29 30>;
> > > > > +                        status = "disabled";
> > > > > +                };
> > > >
> > > > Yash posted this a while ago:
> > > >
> > > > https://lore.kernel.org/linux-riscv/mhng-cb360722-bdb6-4cf7-9fa7-1d92f6b6bbfa@palmerdabbelt-glaptop1/T/#madb19f55bac11a9a675b1ca73ca3f0c2d88c57cf
> > > >
> > > >
> > > > >
> > > > > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > > index 88cfcb96bf23..f3f55dbbf737 100644
> > > > > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > > > >
> > > > >         cpus {
> > > > > @@ -41,6 +41,39 @@
> > > > >                 clock-frequency = <RTCCLK_FREQ>;
> > > > >                 clock-output-names = "rtcclk";
> > > > >         };
> > > > > +
> > > > > +
> > > > > +        pwmleds {
> > > > > +                compatible = "pwm-leds";
> > > > > +                heartbeat {
> > > > > +                        label = "led1";
> > > > > +                        max-brightness = <255>;
> > > > > +                        active-low = <1>;
> > > > > +                        pwms = <&pwm0 0 7812500 0>;
> > > > > +                        linux,default-trigger = "heartbeat";
> > > > > +                };
> > > > > +                mtd {
> > > > > +                        label = "led2";
> > > > > +                        max-brightness = <255>;
> > > > > +                        active-low = <1>;
> > > > > +                        pwms = <&pwm0 1 7812500 0>;
> > > > > +                        linux,default-trigger = "mtd";
> > > > > +                };
> > > > > +                netdev {
> > > > > +                        label = "led3";
> > > > > +                        max-brightness = <255>;
> > > > > +                        active-low = <1>;
> > > > > +                        pwms = <&pwm0 2 7812500 0>;
> > > > > +                        linux,default-trigger = "netdev";
> > > > > +                };
> > > > > +                panic {
> > > > > +                        label = "led4";
> > > > > +                        max-brightness = <255>;
> > > > > +                        active-low = <1>;
> > > > > +                        pwms = <&pwm0 3 7812500 0>;
> > > > > +                        linux,default-trigger = "panic";
> > > > > +                };
> > > > > +        };
> > > > >  };
> > > >
> > > > I don't think it's good to add these pwmleds to the default board DTS
> > > > file.  I realize that many upstream ARM development boards have added this
> > > > type of configuration, but it gets in the way of reusing this DT file when
> > > > integrators wish to have the LEDs used for different purposes.  If the
> > > > Unleashed silkscreen had text on it describing the LEDs as having these
> > > > specific functions, or if Unleashed was sold in a case with similar
> > > > markings on the outside, it'd be a different story, and then a change like
> > > > the above could make sense.
> >
> > I think, there is a middle solution. On SiFive Unleashed PWM LEDs still exist.
> > So let's define those LEDs, but disable them (i.e. no default trigger). In that
> > case user has an options to write udev rules to setup those PWM LEDs as
> > he/she likes. I already use udev rule for netdev LED configuration as today
> > DTS does not provide options to configure it [within DTS] (patch
> > posted in 2019).
> >
> > It would look something like below (not tested). We use labels from PCB and
> > schematics, but do not assign any default functions/triggers. Once the
> > distro boots
> > udev rules will set default triggers and do required configuration.
> >
> > ---
> >  .../riscv/boot/dts/sifive/hifive-unleashed-a00.dts | 27 ++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > index 828f406..eb793b5 100644
> > --- a/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > +++ b/arch/riscv/boot/dts/sifive/hifive-unleashed-a00.dts
> > @@ -26,6 +26,33 @@
> >         };
> >
> >         soc {
> > +               pwmleds {
> > +                       compatible = "pwm-leds";
> > +                       d1 {
> > +                               label = "green:d1";
> > +                               pwms = <&pwm0 0 2000000 0>;
> > +                               max-brightness = <255>;
> > +                               linux,default-trigger = "none";
> > +                       };
> > +                       d2 {
> > +                               label = "green:d2";
> > +                               pwms = <&pwm0 1 2000000 0>;
> > +                               max-brightness = <255>;
> > +                               linux,default-trigger = "none";
> > +                       };
> > +                       d3 {
> > +                               label = "green:d3";
> > +                               pwms = <&pwm0 2 2000000 0>;
> > +                               max-brightness = <255>;
> > +                               linux,default-trigger = "none";
> > +                       };
> > +                       d4 {
> > +                               label = "green:d4";
> > +                               pwms = <&pwm0 3 2000000 0>;
> > +                               max-brightness = <255>;
> > +                               linux,default-trigger = "none";
> > +                       };
> > +               };
> >         };
> >
> >         hfclk: hfclk {
> > --
> > 2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-31 13:21 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-21  6:58 [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/) JaeJoon Jung
2020-01-21 17:20 ` David Abdurachmanov
2020-01-30  3:35 ` Paul Walmsley
2020-01-30  6:23   ` JaeJoon Jung
2020-01-30 16:13     ` David Abdurachmanov
2020-01-30 17:20       ` JaeJoon Jung
2020-01-31 13:20         ` David Abdurachmanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).