From: David Abdurachmanov <david.abdurachmanov@gmail.com>
To: JaeJoon Jung <rgbi3307@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 14:20:42 +0100 [thread overview]
Message-ID: <CAEn-LTqLD1Vz56BXLS-6+f_fiXG8WE1=Diot4uH7bXTdryZr3Q@mail.gmail.com> (raw)
In-Reply-To: <CAHOvCC5sw8BapFTDPK4zAW4mU=2KZ_PDg5=_verixYn22pHcYg@mail.gmail.com>
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
prev parent reply other threads:[~2020-01-31 13:21 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
2020-01-31 13:20 ` David Abdurachmanov [this message]
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='CAEn-LTqLD1Vz56BXLS-6+f_fiXG8WE1=Diot4uH7bXTdryZr3Q@mail.gmail.com' \
--to=david.abdurachmanov@gmail.com \
--cc=Anup.Patel@wdc.com \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=rgbi3307@gmail.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).