linux-kernel - Re: [PATCH 2/2] ARM: dts: imx: Add devicetree for Tolino Vison

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 12 Mar 2023 22:02:24 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Andreas Kemnade <andreas@...nade.info>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, arnd@...db.de, olof@...om.net,
        soc@...nel.org, shawnguo@...nel.org, s.hauer@...gutronix.de,
        kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
        marex@...x.de, max.krummenacher@...adex.com, leoyang.li@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] ARM: dts: imx: Add devicetree for Tolino Vison

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