| 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
| ||
|
Message-ID: <2122a623-4124-4551-92a6-44273c43f71e@kernel.org>
Date: Mon, 30 Dec 2024 18:15:14 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Ivan Sergeev <ivan8215145640@...il.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>
Cc: devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] arm64: dts: rockchip: Add BigTreeTech CB2 and Pi2
On 30/12/2024 15:58, Ivan Sergeev wrote:
> BigTreeTech CB2 and Pi2 share a lot of hardware configuration, so a
> common dtsi file was used to define common nodes and properties. This is
> similar to how BigTreeTech CB1 and Pi are implemented.
>
...
> + vcc3v3_sd: SWITCH_REG2 {
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-name = "vcc3v3_sd";
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> + };
> +
> + codec {
> + rockchip,mic-in-differential;
> + };
> + };
> +};
> +
> +&i2c2 {
> + pinctrl-0 = <&i2c2m1_xfer>;
> +};
> +
> +&i2c3 {
> + status = "okay";
> +
> + tft_tp: ns2009@48 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "ti,tsc2007";
> + reg = <0x48>;
> + status = "okay";
> + ti,x-plate-ohms = <660>;
> + ti,rt-thr = <3000>;
> + ti,fuzzx = <32>;
> + ti,fuzzy = <16>;
> + };
> +};
> +
> +&i2s0_8ch {
> + status = "okay";
> +};
> +
> +&i2s1_8ch {
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2s1m0_sclktx
> + &i2s1m0_lrcktx
> + &i2s1m0_sdi0
> + &i2s1m0_sdo0>;
Why this can't be one line?
> + rockchip,trcm-sync-tx-only;
> + status = "okay";
> +};
> +
> +&spi1 {
> + pinctrl-0 = <&spi1m1_cs0 &spi1m1_pins>;
> +
> + can_mcp2515: can-mcp2515@0 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + status = "disabled";
Why? Anyway, srtatus is never the first property for new nodes.
> + compatible = "microchip,mcp2515";
> + reg = <0x00>;
> + interrupt-parent = <&gpio4>;
> + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> + spi-max-frequency = <10000000>;
> + clocks = <&can_mcp2515_osc>;
> + vdd-supply = <&vcc3v3_sys>;
> + xceiver-supply = <&vcc3v3_sys>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&mcp2515_int_pin>;
> + };
> +};
> +
> +&spi3 {
> + pinctrl-0 = <&spi3m1_cs0 &spi3m1_pins>;
> +};
> +
> +&pcie2x1 {
> + reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&pcie_reset_h>;
> + vpcie3v3-supply = <&vcc3v3_pcie>;
> + status = "okay";
> +};
> +
> +&pinctrl {
> + wireless-bluetooth {
> + uart1_gpios: uart1-gpios {
> + rockchip,pins = <2 RK_PB5 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> +
> + sd {
> + sdmmc0_pwr_h: sdmmc0-pwr-h {
> + rockchip,pins = <0 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> +
> + mxc6655xa {
> + mxc6655xa_irq_gpio: mxc6655xa_irq_gpio {
...
> + };
> +
> + bt {
> + bt_enable: bt-enable-h {
> + rockchip,pins = <2 RK_PB7 RK_FUNC_GPIO &pcfg_pull_down>;
> + };
> +
> + bt_host_wake: bt-host-wake-l {
> + rockchip,pins = <2 RK_PC1 RK_FUNC_GPIO &pcfg_pull_down>;
> + };
> +
> + bt_wake: bt-wake-l {
> + rockchip,pins = <2 RK_PC0 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> +
> + mcp2515_int_pin {
> + mcp2515_int_pin: mcp2515_in_pin {
Don't use underscore. See DTS coding style.
> + rockchip,pins = <4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_up>;
> + };
> + };
> +};
> +
> +&pmu_io_domains {
> + status = "okay";
> + pmuio1-supply = <&vcc3v3_pmu>;
> + pmuio2-supply = <&vcc3v3_pmu>;
> + vccio1-supply = <&vcc_3v3>;
> + vccio2-supply = <&vcc_1v8>;
> + vccio3-supply = <&vccio_sd>;
> + vccio4-supply = <&vcc_1v8>;
> + vccio5-supply = <&vcc_3v3>;
> + vccio6-supply = <&vcc_3v3>;
> + vccio7-supply = <&vcc_3v3>;
Messed indentation
> +
> +&sdmmc0 {
> + max-frequency = <150000000>;
> + bus-width = <4>;
> + cap-mmc-highspeed;
> + cap-sd-highspeed;
> + disable-wp;
> + //sd-uhs-sdr104;
Drop dead code
> + vmmc-supply = <&vcc_sd>;
> + vqmmc-supply = <&vccio_sd>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>;
> + status = "okay";
> +};
> +
> +&sdmmc1 {
> + /* WiFi & BT combo module AMPAK AP6256 */
> + #address-cells = <1>;
> + #size-cells = <0>;
> + max-frequency = <150000000>;
> + bus-width = <4>;
> + disable-wp;
> + cap-sd-highspeed;
> + cap-sdio-irq;
> + keep-power-in-suspend;
> + mmc-pwrseq = <&sdio_pwrseq>;
> + non-removable;
> + pinctrl-names = "default";
> + pinctrl-0 = <&sdmmc1_bus4 &sdmmc1_cmd &sdmmc1_clk>;
> + sd-uhs-sdr104;
> + status = "okay";
Status is always the last property... or the first in overrides, but not
middle.
> + rockchip,default-sample-phase = <90>;
> +
> + sdio-wifi@1 {
> + compatible = "brcm,bcm4329-fmac";
> + reg = <1>;
> + interrupt-parent = <&gpio2>;
> + interrupts = <9 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "host-wake";
> + pinctrl-names = "default";
> + pinctrl-0 = <&wifi_host_wake>;
> + brcm,drive-strength = <10>;
> + };
> +};
> +
> +&sfc {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + status = "okay";
> +};
> +
> +&tsadc {
> + status = "okay";
> +};
> +
> +&uart1 {
> + status = "okay";
> + pinctrl-names = "default";
> + pinctrl-0 = <&uart1m0_xfer &uart1m0_ctsn &uart1m0_rtsn>;
> + uart-has-rtscts;
> + dma-names = "tx\0rx";
Hm? NUL byte?
> +
> + bluetooth {
> + compatible = "brcm,bcm4345c5";
> + clocks = <&rk809 1>;
> + clock-names = "lpo";
> + device-wakeup-gpios = <&gpio2 RK_PC0 GPIO_ACTIVE_HIGH>;
> + host-wakeup-gpios = <&gpio2 RK_PC1 GPIO_ACTIVE_HIGH>;
> + shutdown-gpios = <&gpio2 RK_PB7 GPIO_ACTIVE_HIGH>;
> + //pinctrl-names = "default";
> + //pinctrl-0 = <&bt_host_wake &bt_wake &bt_enable>;
Drop dead code
Best regards,
Krzysztof
Powered by blists - more mailing lists