Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Marco Felsch]

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@xxxxxxxxxxxxxx> 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 |