From: Marco Felsch <m.felsch@pengutronix.de>
To: Andreas Kemnade <andreas@kemnade.info>
Cc: mark.rutland@arm.com, marex@denx.de, devicetree@vger.kernel.org,
andrew.smirnov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, angus@akkea.ca,
linux-kernel@vger.kernel.org, j.neuschaefer@gmx.net,
robh+dt@kernel.org, linux-imx@nxp.com, kernel@pengutronix.de,
manivannan.sadhasivam@linaro.org,
Discussions about the Letux Kernel <letux-kernel@openphoenux.org>,
festevam@gmail.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file
Date: Mon, 30 Sep 2019 10:27:15 +0200 [thread overview]
Message-ID: <20190930082715.mplf5ra35ikqmbyr@pengutronix.de> (raw)
In-Reply-To: <20190927210807.26439a94@aktux>
Hi Andreas,
On 19-09-27 21:08, Andreas Kemnade wrote:
> Hi Marco,
>
> On Fri, 27 Sep 2019 11:47:21 +0200
> Marco Felsch <m.felsch@pengutronix.de> wrote:
>
> > Hi Andreas,
> >
> > thanks for the patch.
> >
> thanks for the quick review. Most of your comments are clear.
>
> [...]
> > > + wifi_pwrseq: wifi_pwrseq {
> > > + compatible = "mmc-pwrseq-simple";
> > > + post-power-on-delay-ms = <20>;
> > > + reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> >
> > Can you add a pinctrl-entry here please? The general rule is to mux
> > things where you use it.
> >
> yes, there are many places in my patch where they are added to some
> parent devices.
> I will fix that.
> [...]
> > > + leds {
> > > + compatible = "gpio-leds";
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_led>;
> >
> > Please move all muxing you made here into this file or add phandles so
> > the dts file need to add only the muxing stuff. This applies to all
> > pinctrl you made here.
> >
> so you disagree with this pattern:
> in .dtsi
> some_device {
> pinctrl-0 = <&pinctrl_some_device>;
> };
>
> and in .dts (one I sent with this patch series and the tolino/mx6sl one
> is not ready-cooked yet, will be part of a later series)
> &iomuxc {
> pinctrl_some_device: some_devicegrp {
> fsl,pins = <...>;
> };
> };
>
> ?
Yes, because IMHO a dtsi is self contained as well as a dts. If it is
common for all boards you can move the muxing into the dtsi else it
should be done within the dts.
> > > +
> > > + GLED {
> > > + gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
> > > + linux,default-trigger = "timer";
> > > + };
> > > + };
> > > +
> > > + gpio-keys {
> > > + compatible = "gpio-keys";
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <&pinctrl_gpio_keys>;
> > > + power {
> > > + label = "Power";
> > > + gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
> > > + linux,code = <KEY_POWER>;
> >
> > Add missing header: dt-bindings/input/input.h to use this.
> >
> I am doing this in the .dts but it is probably better to do it here
> because it is used here.
>
> > > + gpio-key,wakeup;
> > > + };
> > > + cover {
> > > + label = "Cover";
> > > + gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> > > + linux,code = <SW_LID>;
> > > + linux,input-type = <0x05>; /* EV_SW */
> >
> > In the header above EV_SW is also specified so please use it here.
> >
> This pattern is in many files. I took one as an example. It seems that
> 50% of devicetree files have this pattern, the other 50% do have the
> pattern you proposed (which I like more). So probably EV_SW was not
> available in former times?
I don't know, checking the git history should bring the answer ;)
Anyway, if it is available we should use it.
> > > + gpio-key,wakeup;
> > > + };
> > > + };
> > > +
> > > +};
> > > +
> > > +
> > > +
> >
> > Whitespaces
> >
> > > +&audmux {
> > > + pinctrl-names = "default";
> > > + status = "disabled";
> >
> > Why you mentioned a pinctrl-names here without the mux? Do we need the
> > status line here? The common case is that such devices are off by
> > default/the base dt.
> >
> yes, that things can be removed.
> > > +};
> > > +
> > > +&snvs_rtc {
> > > + status = "disabled";
> >
> > Same applies here.
> >
>
> No, seems to be an exception, it does not have a status = "disabled" in
> imx6sll.dtsi.
Did you mean 6sll or 6ull?
Okay, is this baseboard only used with a 6ull?
Regards,
Marco
> > > +};
> > > +
> > > +&i2c1 {
> > > + clock-frequency = <100000>;
> > > + pinctrl-names = "default","sleep";
> > > + pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;
> >
> > The &pinctrl_lm3630a_bl_gpio should be moved into the lm3630a node.
> >
> > > + pinctrl-1 = <&pinctrl_i2c1_sleep>;
> > > + status = "okay";
> > > +
> > > + lm3630a: lm3630a-i2c@36 {
> >
> > please name it backlight@36
> >
> > > + reg = <0x36>;
> > > + status = "ok";
> >
> > status lines are always be the last and if it is okay you can drop it
> > because the default is okay.
> >
> well, I added it because the driver was not loaded but later I found out
> that the real reason is that module aliases were broken and forgot to
> remove that "ok".
>
> Regards,
> Andreas
>
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-09-30 8:27 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-27 6:14 [PATCH 0/3] dts: ARM: add Kobo Clara HD eBook reader Andreas Kemnade
2019-09-27 6:14 ` [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file Andreas Kemnade
2019-09-27 9:47 ` Marco Felsch
2019-09-27 19:08 ` Andreas Kemnade
2019-09-30 8:27 ` Marco Felsch [this message]
2019-09-30 11:02 ` Andreas Kemnade
2019-09-27 6:14 ` [PATCH 2/3] dt-bindings: arm: fsl: add compatible string for Kobo Clara HD Andreas Kemnade
2019-09-27 9:19 ` Marco Felsch
2019-09-27 6:14 ` [PATCH 3/3] ARM: dts: imx: add devicetree " Andreas Kemnade
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=20190930082715.mplf5ra35ikqmbyr@pengutronix.de \
--to=m.felsch@pengutronix.de \
--cc=andreas@kemnade.info \
--cc=andrew.smirnov@gmail.com \
--cc=angus@akkea.ca \
--cc=devicetree@vger.kernel.org \
--cc=festevam@gmail.com \
--cc=j.neuschaefer@gmx.net \
--cc=kernel@pengutronix.de \
--cc=letux-kernel@openphoenux.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=marex@denx.de \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
/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).