Message-ID: <1346ce4e-f1fd-1a77-f38e-cd87efc59082@linaro.org>
Date:   Sun, 12 Mar 2023 22:02:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH 2/2] ARM: dts: imx: Add devicetree for Tolino Vison
Content-Language: en-US
To:     Andreas Kemnade <andreas@kemnade.info>, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, arnd@arndb.de, olof@lixom.net,
        soc@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        marex@denx.de, max.krummenacher@toradex.com, leoyang.li@nxp.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
References: <20230312205236.2281466-1-andreas@kemnade.info>
 <20230312205236.2281466-3-andreas@kemnade.info>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230312205236.2281466-3-andreas@kemnade.info>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/03/2023 21:52, Andreas Kemnade wrote:
> This adds a devicetree for the Kobo Aura 2 Ebook reader. It is based
> on boards marked with "37NB-E60Q30+4A3". It is equipped with an i.MX6SL
> SoC.
> 

Thank you for your patch. There is something to discuss/improve.

> +	wifi_pwrseq: wifi_pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_wifi_reset>;
> +		post-power-on-delay-ms = <20>;
> +		reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> +	};
> +};
> +
> +&i2c1 {
> +	pinctrl-names = "default","sleep";
> +	pinctrl-0 = <&pinctrl_i2c1>;
> +	pinctrl-1 = <&pinctrl_i2c1_sleep>;
> +	status = "okay";
> +
> +	touchscreen@15 {
> +		reg = <0x15>;
> +		compatible = "elan,ektf2132";

compatible first, then reg.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ts>;
> +		power-gpios = <&gpio5 13 GPIO_ACTIVE_HIGH>;
> +		interrupts-extended = <&gpio5 6 IRQ_TYPE_EDGE_FALLING>;
> +	};
> +
> +	accelerometer@1d {
> +		reg = <0x1d>;
> +		compatible = "fsl,mma8652";
> +	};
> +};
> +
> +&i2c2 {
> +	pinctrl-names = "default","sleep";
> +	pinctrl-0 = <&pinctrl_i2c2>;
> +	pinctrl-1 = <&pinctrl_i2c2_sleep>;
> +	clock-frequency = <100000>;
> +	status = "okay";
> +};
> +
> +&i2c3 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c3>;
> +	clock-frequency = <100000>;
> +	status = "okay";
> +
> +	ec: embedded-controller@43 {
> +		compatible = "netronix,ntxec";
> +		reg = <0x43>;
> +		#pwm-cells = <2>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_ec>;
> +		interrupts-extended = <&gpio5 11 IRQ_TYPE_EDGE_FALLING>;
> +		system-power-controller;
> +	};
> +};
> +
> +&snvs_rtc {
> +	/*
> +	 * We are using the RTC in the PMIC, but this one is not disabled
> +	 * in imx6sl.dtsi.
> +	 */
> +	status = "disabled";
> +};
> +
> +&uart1 {
> +	/* J4 */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart1>;
> +	status = "okay";
> +};
> +
> +&uart4 {
> +	/* J9 */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart4>;
> +	status = "okay";
> +};
> +
> +&usdhc2 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> +	pinctrl-0 = <&pinctrl_usdhc2>;
> +	pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc2_sleep>;
> +	cd-gpios = <&gpio5 2 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +
> +	/* removable uSD card */
> +};
> +
> +&usdhc3 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> +	pinctrl-0 = <&pinctrl_usdhc3>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc3_sleep>;
> +	vmmc-supply = <&reg_wifi>;
> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	cap-power-off-card;
> +	non-removable;
> +	status = "okay";
> +
> +	/* CyberTan WC121 (BCM43362) SDIO WiFi */
> +};
> +
> +&usdhc4 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep";
> +	pinctrl-0 = <&pinctrl_usdhc4>;
> +	pinctrl-1 = <&pinctrl_usdhc4_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc4_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc4_sleep>;
> +	bus-width = <8>;
> +	no-1-8-v;
> +	non-removable;
> +	status = "okay";
> +
> +	/* internal eMMC */
> +};
> +
> +&usbotg1 {
> +	pinctrl-names = "default";
> +	disable-over-current;
> +	srp-disable;
> +	hnp-disable;
> +	adp-disable;
> +	status = "okay";
> +};
> +
> +&iomuxc {
> +	pinctrl_backlight_power: backlight-powergrp {
> +		fsl,pins = <
> +			MX6SL_PAD_EPDC_PWRCTRL3__GPIO2_IO10 0x10059
> +		>;
> +	};
> +
> +	pinctrl_ec: ecgrp {
> +		fsl,pins = <
> +			MX6SL_PAD_SD1_DAT0__GPIO5_IO11  0x17000
> +		>;
> +	};
> +
> +	pinctrl_gpio_keys: gpio-keysgrp {
> +		fsl,pins = <
> +			MX6SL_PAD_SD1_DAT1__GPIO5_IO08  0x110B0
> +			MX6SL_PAD_SD1_DAT4__GPIO5_IO12  0x110B0
> +			MX6SL_PAD_KEY_COL1__GPIO3_IO26  0x11030
> +		>;
> +	};
> +
> +	pinctrl_i2c1: i2c1grp {
> +		fsl,pins = <
> +			MX6SL_PAD_I2C1_SCL__I2C1_SCL	 0x4001f8b1
> +			MX6SL_PAD_I2C1_SDA__I2C1_SDA	 0x4001f8b1
> +		>;
> +	};
> +
> +	pinctrl_i2c1_sleep: i2c1grp-sleep {
> +		fsl,pins = <
> +			MX6SL_PAD_I2C1_SCL__I2C1_SCL	 0x400108b1
> +			MX6SL_PAD_I2C1_SDA__I2C1_SDA	 0x400108b1
> +		>;
> +	};
> +
> +	pinctrl_i2c2: i2c2grp {
> +		fsl,pins = <
> +			MX6SL_PAD_I2C2_SCL__I2C2_SCL	 0x4001f8b1
> +			MX6SL_PAD_I2C2_SDA__I2C2_SDA	 0x4001f8b1
> +		>;
> +	};
> +
> +	pinctrl_i2c2_sleep: i2c2grp-sleep {

Shouldn't all groups end with 'grp' suffix? Are you sure this passes
dtbs_check?

...

> +
> +	pinctrl_usdhc2_100mhz: usdhc2grp-100mhz {

Name looks wrong. Same in other places further.


Best regards,
Krzysztof