| 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: <10578885.nUPlyArG6x@diego>
Date: Fri, 13 Dec 2024 19:34:36 +0100
From: Heiko Stübner <heiko@...ech.de>
To: linux-kernel@...r.kernel.org, Shimrra Shai <shimrrashai@...il.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-rockchip@...ts.infradead.org, linux-pm@...r.kernel.org,
Shimrra Shai <shimrrashai@...il.com>
Subject:
Re: [PATCH v2 1/2] arm64: dts: rockchip: add DTs for Firefly ITX-3588J
Am Freitag, 13. Dezember 2024, 19:08:54 CET schrieb Shimrra Shai:
> Main DTS for the board and Makefile addition.
>
> Signed-off-by: Shimrra Shai <shimrrashai@...il.com>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts b/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts
> new file mode 100644
> index 000000000..a99c007c7
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts
> @@ -0,0 +1,1133 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/pwm/pwm.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "dt-bindings/usb/pd.h"
> +#include "rockchip-pca9555.h"
> +#include "rk3588.dtsi"
in line with my comment in the binding, please split the system-on-module
parts into a rk3588-firefly-core-3588j.dtsi and then include that file here.
> +
> +/ {
> + model = "Firefly ITX-3588J";
> + compatible = "firefly,itx-3588j", "rockchip,rk3588";
> +
> + aliases {
> + ethernet0 = &gmac0;
> + ethernet1 = &gmac1;
> + mmc0 = &sdhci;
> + };
> +
> + chosen {
> + stdout-path = "serial2:1500000n8";
> + };
> +
> + /* NB: There are also a "Reset" and "Mask ROM" button but I don't
> + * know the right settings for these. - Shimrra Shai
> + */
same comment-style as below, plus
/*
* There are additional Reset and Maskrom keys connected, but their
* settings are still unknown right now.
*/
> + adc-keys-1 {
why is it adc-keys-1 ? (where is -0 ?)
> + compatible = "adc-keys";
> + io-channels = <&saradc 1>;
> + io-channel-names = "buttons";
> + keyup-threshold-microvolt = <1800000>;
> + poll-interval = <100>;
> +
> + button-recovery {
> + label = "Recovery";
> + linux,code = <KEY_VENDOR>;
> + press-threshold-microvolt = <2000>;
> + };
> + };
> +
> + analog-sound {
> + compatible = "simple-audio-card";
> + pinctrl-0 = <&hp_detect>;
> + pinctrl-names = "default";
> + simple-audio-card,aux-devs = <&_headphones>, <&_speaker>;
> + simple-audio-card,format = "i2s";
> + simple-audio-card,hp-det-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_LOW>;
> + simple-audio-card,mclk-fs = <384>;
> + simple-audio-card,name = "rockchip_es8323";
> + simple-audio-card,pin-switches = "Headphones", "Speaker";
> + simple-audio-card,routing =
> + "Speaker Amplifier INL", "LOUT2",
> + "Speaker Amplifier INR", "ROUT2",
> + "Speaker", "Speaker Amplifier OUTL",
> + "Speaker", "Speaker Amplifier OUTR",
> + "Headphones Amplifier INL", "LOUT1",
> + "Headphones Amplifier INR", "ROUT1",
> + "Headphones", "Headphones Amplifier OUTL",
> + "Headphones", "Headphones Amplifier OUTR",
> + "LINPUT1", "Microphone Jack",
> + "RINPUT1", "Microphone Jack",
> + "LINPUT2", "Onboard Microphone",
> + "RINPUT2", "Onboard Microphone";
> + simple-audio-card,widgets =
> + "Microphone", "Microphone Jack",
> + "Microphone", "Onboard Microphone",
> + "Headphone", "Headphones",
> + "Speaker", "Speaker";
> +
> + simple-audio-card,cpu {
> + sound-dai = <&i2s0_8ch>;
> + };
> +
> + simple-audio-card,codec {
> + sound-dai = <&es8323>;
> + system-clock-frequency = <12288000>;
> + };
> + };
> +
> + /* note: this does not seem to be a proper "amplifier" but is just
/*
* note: this does not seem to be a proper "amplifier" but is just
comment formatting please
> + * a way to control the GPIO pins to switch on or off the given
> + * sound output device
> + */
> + amp_headphones: headphones-audio-amplifier {
> + compatible = "simple-audio-amplifier";
> + enable-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&headphone_amplifier_en>;
> + sound-name-prefix = "Headphones Amplifier";
> + };
> + leds {
> + compatible = "gpio-leds";
> +
> + /* NB: This Power LED control does not seem to work for
> + * some reason. - Shimrra Shai
> + */
> +#if 0
> + power_led: led-0 {
> + gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "default-on";
> + };
> +#endif
please don't add dead code
> +
> + user_led: led-1 {
> + gpios = <&pca9555 PCA_IO0_3 GPIO_ACTIVE_HIGH>;
> + linux,default-trigger = "disk-activity";
> + };
> + };
> +
> + vcc_sata_pwr_en: vcc-sata-pwr-en-regulator {
vcc_sata_pwr_en: regulator-vcc-sata-pwr-en
Applied to all regulator nodes
> + compatible = "regulator-fixed";
> + regulator-name = "vcc_sata_pwr_en";
> + regulator-boot-on;
> + regulator-always-on;
> + enable-active-high;
> + gpio = <&pca9555 PCA_IO1_2 GPIO_ACTIVE_HIGH>; //PCA_IO 12
please sort properties (compatible, regs, [alphabetically], status) and please
drop those comments at the end.
Applied to all regulator nodes
> + };
[...]
> +&cpu_l0 {
> + cpu-supply = <&vdd_cpu_lit_s0>;
> + mem-supply = <&vdd_cpu_lit_mem_s0>;
that mem-supply property is not specified and also is not necessary here.
Same for the other cases above.
> +};
[...]
> + usbc0: usb-typec@22 {
> + compatible = "fcs,fusb302";
> + reg = <0x22>;
> + interrupt-parent = <&gpio0>;
> + interrupts = <RK_PD3 IRQ_TYPE_LEVEL_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <&usbc0_int>;
> + vbus-supply = <&vbus5v0_typec_pwr_en>;
> + status = "okay";
default status is always "okay", so no need to add it for new nodes.
Same for possible other places in this file.
> diff --git a/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h b/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h
> new file mode 100644
> index 000000000..c4c9a2471
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h
if anything, that include needs to live in include/dt-bindings/something
and needs to be a separate patch, if you really need that
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Bindings for the PCA9555 GPIO extender used on some Rockchip devices, e.g.
> + * Firefly.
> + *
> + * Copyright (c) 2013 MundoReader S.L.
> + * Authors: Heiko Stuebner <heiko@...ech.de>
where does this copyright come from? I don't remember being involved
in a binding header for that expander?
> + * Shimrra Shai <shimrrashai@...il.com>
> + */
> +
> +#ifndef __RK_PCA9555_H__
> +#define __RK_PCA9555_H__
> +
> +#define PCA_IO0_0 0
> +#define PCA_IO0_1 1
> +#define PCA_IO0_2 2
> +#define PCA_IO0_3 3
> +#define PCA_IO0_4 4
> +#define PCA_IO0_5 5
> +#define PCA_IO0_6 6
> +#define PCA_IO0_7 7
> +#define PCA_IO1_0 8
> +#define PCA_IO1_1 9
> +#define PCA_IO1_2 10
> +#define PCA_IO1_3 11
> +#define PCA_IO1_4 12
> +#define PCA_IO1_5 13
> +#define PCA_IO1_6 14
> +#define PCA_IO1_7 15
> +
> +#endif
>
Heiko
Powered by blists - more mailing lists