Re: [PATCH 08/10] arm64: dts: Add DTS for Marvell PXA1908 and samsung,coreprimevelte

[Krzysztof Kozlowski]

On 21/07/2023 22:37, Duje Mihanović wrote:
> Add DTS for Marvell PXA1908 SoC and Samsung Galaxy Core Prime Value
> Edition LTE, a smartphone based on said SoC.
> 
> Signed-off-by: Duje Mihanović <duje.mihanovic@xxxxxxxx>
> ---
>  arch/arm64/boot/dts/marvell/Makefile          |   1 +
>  .../pxa1908-samsung-coreprimevelte.dts        | 321 ++++++++++++++++++
>  arch/arm64/boot/dts/marvell/pxa1908.dtsi      | 298 ++++++++++++++++
>  3 files changed, 620 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/marvell/pxa1908-samsung-coreprimevelte.dts
>  create mode 100644 arch/arm64/boot/dts/marvell/pxa1908.dtsi
> 
> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
> index 79ac09b58a89..0e277a0d368b 100644
> --- a/arch/arm64/boot/dts/marvell/Makefile
> +++ b/arch/arm64/boot/dts/marvell/Makefile
> @@ -27,3 +27,4 @@ dtb-$(CONFIG_ARCH_MVEBU) += cn9132-db-B.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-A.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += cn9130-crb-B.dtb
>  dtb-$(CONFIG_ARCH_MVEBU) += ac5-98dx35xx-rd.dtb
> +dtb-$(CONFIG_ARCH_MMP) += pxa1908-samsung-coreprimevelte.dtb
> diff --git a/arch/arm64/boot/dts/marvell/pxa1908-samsung-coreprimevelte.dts b/arch/arm64/boot/dts/marvell/pxa1908-samsung-coreprimevelte.dts
> new file mode 100644
> index 000000000000..3e10a77a106e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/pxa1908-samsung-coreprimevelte.dts
> @@ -0,0 +1,321 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "pxa1908.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/linux-event-codes.h>
> +
> +/ {
> +	pxa,rev-id = <3928 2>;

Drop. This is not documented.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).


> +	model = "Samsung Galaxy Core Prime VE LTE";
> +	compatible = "samsung,coreprimevelte", "marvell,pxa1908";

Missing bindings.

> +
> +	/* Bootloader fills this in */
> +	memory {
> +		device_type = "memory";
> +		reg = <0 0 0 0>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		framebuffer@17000000 {
> +			reg = <0 0x17000000 0 0x1800000>;
> +			no-map;
> +		};
> +
> +		gpu@9000000 {
> +			reg = <0 0x9000000 0 0x1000000>;
> +		};
> +
> +		/* Communications processor, aka modem */
> +		cp@3000000 {
> +			reg = <0 0x3000000 0 0x5000000>;
> +		};
> +
> +		cm3@a000000 {
> +			reg = <0 0xa000000 0 0x80000>;
> +		};
> +
> +		seclog@8000000 {
> +			reg = <0 0x8000000 0 0x100000>;
> +		};
> +
> +		ramoops@8100000 {
> +			compatible = "ramoops";
> +			reg = <0 0x8100000 0 0x40000>;
> +			record-size = <0x8000>;
> +			console-size = <0x20000>;
> +			max-reason = <5>;
> +		};
> +	};
> +
> +	fb0: framebuffer@17177000 {
> +		compatible = "simple-framebuffer";
> +		reg = <0 0x17177000 0 (480 * 800 * 4)>;
> +		width = <480>;
> +		height = <800>;
> +		stride = <(480 * 4)>;
> +		format = "a8r8g8b8";
> +	};
> +
> +	muic-i2c {
> +		compatible = "i2c-gpio";
> +		sda-gpios = <&gpio 30 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		scl-gpios = <&gpio 29 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		i2c-gpio,delay-us = <3>;
> +		i2c-gpio,timeout-ms = <100>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&muic_i2c_pins>;
> +
> +		muic: extcon@14 {
> +			compatible = "siliconmitus,sm5504-muic";
> +			reg = <0x14>;
> +			interrupt-parent = <&gpio>;
> +			interrupts = <0 IRQ_TYPE_EDGE_FALLING>;
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&gpio_keys_pins>;
> +		autorepeat;
> +
> +		key-home {
> +			label = "Home";
> +			linux,code = <KEY_HOME>;
> +			gpios = <&gpio 50 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		key-volup {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&gpio 16 GPIO_ACTIVE_LOW>;
> +		};
> +
> +		key-voldown {
> +			label = "Volume Down";
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			gpios = <&gpio 17 GPIO_ACTIVE_LOW>;
> +		};
> +	};
> +
> +	chosen {};

chosen is usually at the top (as alphabetical order suggests)

> +};
> +
> +&smmu {
> +	status = "okay";
> +};
> +
> +&pmx {
> +	pinctrl-single,gpio-range = <&range 55 55 0>,
> +				    <&range 110 32 0>,
> +				    <&range 52 1 0>;
> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&board_pins_1 &board_pins_2 &board_pins_3>;
> +
> +	board_pins_1: pinmux_board_1 {

No underscores in node names.

...

> diff --git a/arch/arm64/boot/dts/marvell/pxa1908.dtsi b/arch/arm64/boot/dts/marvell/pxa1908.dtsi
> new file mode 100644
> index 000000000000..7131b2070b72
> --- /dev/null
> +++ b/arch/arm64/boot/dts/marvell/pxa1908.dtsi
> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/dts-v1/;
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/marvell,pxa1908.h>
> +
> +/ {
> +	model = "Marvell Armada PXA1908";
> +	compatible = "marvell,pxa1908";

Undocumented compatible.

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.


Best regards,
Krzysztof