From: JaeJoon Jung <rgbi3307@gmail.com>
To: David Abdurachmanov <david.abdurachmanov@gmail.com>
Cc: linux-riscv <linux-riscv@lists.infradead.org>,
Anup Patel <Anup.Patel@wdc.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Paul Walmsley <paul.walmsley@sifive.com>
Subject: Re: [PATCH] riscv: Add gpio and pwmleds to DTS(/arch/riscv/boot/dts/sifive/)
Date: Fri, 31 Jan 2020 02:20:19 +0900 [thread overview]
Message-ID: <CAHOvCC5sw8BapFTDPK4zAW4mU=2KZ_PDg5=_verixYn22pHcYg@mail.gmail.com> (raw)
In-Reply-To: <CAEn-LTpZCdeFYCt6DKrHfp8xcOuUejYXYqJ=wgtXHE8Qf0oRgA@mail.gmail.com>
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
next prev parent reply other threads:[~2020-01-30 17:20 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-01-31 13:20 ` David Abdurachmanov
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='CAHOvCC5sw8BapFTDPK4zAW4mU=2KZ_PDg5=_verixYn22pHcYg@mail.gmail.com' \
--to=rgbi3307@gmail.com \
--cc=Anup.Patel@wdc.com \
--cc=david.abdurachmanov@gmail.com \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).