Hi Andreas,

On 19-09-27 21:08, Andreas Kemnade wrote:

Hi Marco,

On Fri, 27 Sep 2019 11:47:21 +0200
Marco Felsch [off-list ref] wrote:

quoted

Hi Andreas,

thanks for the patch.

thanks for the quick review. Most of your comments are clear.

[...]

quoted

quoted

+	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.
[...]

quoted

quoted

+	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.

quoted

quoted

+
+		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.

quoted

quoted

+			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.

quoted

quoted

+			gpio-key,wakeup;
+		};
+	};
+
+};
+
+
+  

Whitespaces

quoted

+&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.

quoted

quoted

+};
+
+&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

quoted

quoted

+};
+
+&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.

quoted

+	pinctrl-1 = <&pinctrl_i2c1_sleep>;
+	status = "okay";
+
+	lm3630a: lm3630a-i2c@36 {  

please name it backlight@36

quoted

+		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