Re: [PATCH 2/5] ARM: dts: add Samsung's exynos4412-based p4note boards

[Martin Juecker <martin.juecker@gmail.com>]

On Mon, Nov 02, 2020 at 08:18:45PM +0100, Krzysztof Kozlowski wrote:
> On Sat, Oct 31, 2020 at 06:58:34PM +0100, Martin Jücker wrote:
> > The p4note family contains a couple of variants of the Galaxy Note 10.1
> > tablet with mainly different modems. The GT-N8010/GT-N8013 is the wifi
> > only version.
> 
> The subject is v1. Did you send correct patch?

I probably messed up with git send-email here. I need some more time to
get used to it. The patch is indeed v2.

> 
> > 
> > Signed-off-by: Martin Jücker <martin.juecker@gmail.com>
> > ---
> >  arch/arm/boot/dts/Makefile                    |    1 +
> >  arch/arm/boot/dts/exynos4412-p4note-n8010.dts |   16 +
> >  arch/arm/boot/dts/exynos4412-p4note.dtsi      | 1128 +++++++++++++++++
> >  3 files changed, 1145 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/exynos4412-p4note-n8010.dts
> >  create mode 100644 arch/arm/boot/dts/exynos4412-p4note.dtsi
> > 
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 4572db3fa5ae..951853e55edb 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -195,6 +195,7 @@ dtb-$(CONFIG_ARCH_EXYNOS4) += \
> >  	exynos4412-odroidx.dtb \
> >  	exynos4412-odroidx2.dtb \
> >  	exynos4412-origen.dtb \
> > +	exynos4412-p4note-n8010.dtb \
> >  	exynos4412-smdk4412.dtb \
> >  	exynos4412-tiny4412.dtb \
> >  	exynos4412-trats2.dtb
> > diff --git a/arch/arm/boot/dts/exynos4412-p4note-n8010.dts b/arch/arm/boot/dts/exynos4412-p4note-n8010.dts
> > new file mode 100644
> > index 000000000000..f99358750e01
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/exynos4412-p4note-n8010.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's Galaxy Note 10.1 - N801x (wifi only version)
> > + *
> > + * Copyright (c) 2020 Martin Jücker <martin.juecker@gmail.com>
> > + */
> > +
> > +/dts-v1/;
> > +#include "exynos4412-p4note.dtsi"
> > +
> > +/ {
> > +	model = "Samsung Galaxy Note 10.1 (GT-N8010/N8013) based on Exynos4412";
> > +	compatible = "samsung,n8010", "samsung,p4note", "samsung,exynos4412", "samsung,exynos4";
> > +
> > +	/* this is the base variant without any kind of modem */
> > +};
> > diff --git a/arch/arm/boot/dts/exynos4412-p4note.dtsi b/arch/arm/boot/dts/exynos4412-p4note.dtsi
> > new file mode 100644
> > index 000000000000..15b6acbbecb2
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/exynos4412-p4note.dtsi
> > @@ -0,0 +1,1128 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Samsung's Exynos4412 based p4note device family base DT
> > + *
> > + * Copyright (c) 2020 Martin Jücker <martin.juecker@gmail.com>
> 
> Multiple parts of this file look like copied from the existing ones. Few
> comments are the same. The v1 duplicated even few minor issues like not
> that good naming of nodes.  It looks like it is not only based on
> existing sources, but uses parts of them directly.  If it is true, then
> include the copyright of original work as well.
> https://www.gnu.org/prep/maintain/html_node/Copyright-Notices.html
> 

I had a look through multiple files and ended up with a mix of copying
and writing nodes myself. Most if not all of the copying is from the
midas dt, is it enough to add a note that this file is based on the
midas dt or should I go with the same approach like midas and only add
the samsung copyright here? I don't care too much what's in the header.

> 
> > + */
> > +
> > +/dts-v1/;
> > +#include "exynos4412.dtsi"
> > +#include "exynos4412-ppmu-common.dtsi"
> > +
> > +#include <dt-bindings/clock/maxim,max77686.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/linux-event-codes.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/pinctrl/samsung.h>
> > +
> > +/ {
> > +	compatible = "samsung,p4note", "samsung,exynos4412", "samsung,exynos4";
> > +
> > +	memory@40000000 {
> > +		device_type = "memory";
> > +		reg = <0x40000000 0x80000000>;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = &serial_2;
> > +	};
> > +
> > +	firmware@204f000 {
> > +		compatible = "samsung,secure-firmware";
> > +		reg = <0x0204F000 0x1000>;
> > +	};
> > +
> > +	fixed-rate-clocks {
> > +		xxti {
> > +			compatible = "samsung,clock-xxti";
> > +			clock-frequency = <0>;
> > +		};
> > +
> > +		xusbxti {
> > +			compatible = "samsung,clock-xusbxti";
> > +			clock-frequency = <24000000>;
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&gpio_keys>;
> > +
> > +		key-down {
> > +			gpios = <&gpx2 2 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_VOLUMEDOWN>;
> > +			label = "volume down";
> > +			debounce-interval = <10>;
> > +		};
> > +
> > +		key-up {
> > +			gpios = <&gpx3 3 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_VOLUMEUP>;
> > +			label = "volume up";
> > +			debounce-interval = <10>;
> > +		};
> > +
> > +		key-power {
> > +			gpios = <&gpx2 7 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_POWER>;
> > +			label = "power";
> > +			debounce-interval = <10>;
> > +			wakeup-source;
> > +		};
> > +	};
> > +
> > +	voltage-regulator-1 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "TSP_LDO1";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&tsp_reg_gpio_1>;
> > +		gpios = <&gpm4 5 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	voltage-regulator-2 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "TSP_LDO2";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&tsp_reg_gpio_2>;
> > +		gpios = <&gpb 5 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	voltage-regulator-3 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "TSP_LDO3";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&tsp_reg_gpio_3>;
> > +		gpios = <&gpb 7 GPIO_ACTIVE_HIGH>;
> > +		startup-delay-us = <20000>;
> > +		enable-active-high;
> > +		regulator-always-on;
> > +	};
> > +
> > +	wlan_pwrseq: sdhci3-pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		reset-gpios = <&gpm3 5 GPIO_ACTIVE_LOW>;
> > +		pinctrl-0 = <&wifi_reset>;
> > +		pinctrl-names = "default";
> > +		clocks = <&max77686 MAX77686_CLK_PMIC>;
> > +		clock-names = "ext_clock";
> > +	};
> > +
> > +	i2c-gpio-1 {
> > +		compatible = "i2c-gpio";
> > +		sda-gpios = <&gpy2 4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		scl-gpios = <&gpy2 5 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		i2c-gpio,delay-us = <2>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		magnetometer@c {
> > +			compatible = "asahi-kasei,ak8975";
> > +			reg = <0x0c>;
> > +			pinctrl-0 = <&ak8975_irq>;
> > +			pinctrl-names = "default";
> > +			interrupt-parent = <&gpm4>;
> > +			interrupts = <7 IRQ_TYPE_EDGE_RISING>;
> > +		};
> > +	};
> > +
> > +	i2c-gpio-2 {
> > +		compatible = "i2c-gpio";
> > +		sda-gpios = <&gpy0 2 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		scl-gpios = <&gpy0 3 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		i2c-gpio,delay-us = <2>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		fuel-gauge@36 {
> > +			compatible = "maxim,max17042";
> > +			reg = <0x36>;
> > +			pinctrl-0 = <&fuel_alert_irq>;
> > +			pinctrl-names = "default";
> > +			interrupt-parent = <&gpx2>;
> > +			interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> > +			maxim,rsns-microohm = <10000>;
> > +			maxim,over-heat-temp = <600>;
> > +			maxim,over-volt = <4300>;
> > +		};
> > +	};
> > +
> > +	i2c-gpio-3 {
> > +		compatible = "i2c-gpio";
> > +		sda-gpios = <&gpm4 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		scl-gpios = <&gpm4 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> > +		i2c-gpio,delay-us = <5>;
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		adc@41 {
> > +			compatible = "st,stmpe811";
> > +			reg = <0x41>;
> > +			pinctrl-0 = <&stmpe_adc_irq>;
> > +			pinctrl-names = "default";
> > +			interrupt-parent = <&gpx0>;
> > +			interrupts = <1 IRQ_TYPE_LEVEL_LOW>;
> > +			interrupt-controller;
> > +			irq-trigger = <0x1>;
> > +			st,adc-freq = <3>;
> > +			st,mod-12b = <1>;
> > +			st,ref-sel = <0>;
> > +			st,sample-time = <3>;
> > +
> > +			stmpe_adc {
> > +				compatible = "st,stmpe-adc";
> > +				#io-channel-cells = <1>;
> > +				st,norequest-mask = <0x2F>;
> > +			};
> > +		};
> > +	};
> > +};
> > +
> > +&adc {
> > +	vdd-supply = <&ldo3_reg>;
> > +	/* not verified */
> > +	status = "okay";
> > +};
> > +
> > +&bus_dmc {
> > +	devfreq-events = <&ppmu_dmc0_3>, <&ppmu_dmc1_3>;
> > +	vdd-supply = <&buck1_reg>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_acp {
> > +	devfreq = <&bus_dmc>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_c2c {
> > +	devfreq = <&bus_dmc>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_leftbus {
> > +	devfreq-events = <&ppmu_leftbus_3>, <&ppmu_rightbus_3>;
> > +	vdd-supply = <&buck3_reg>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_rightbus {
> > +	devfreq = <&bus_leftbus>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_display {
> > +	devfreq = <&bus_leftbus>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_fsys {
> > +	devfreq = <&bus_leftbus>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_peri {
> > +	devfreq = <&bus_leftbus>;
> > +	status = "okay";
> > +};
> > +
> > +&bus_mfc {
> > +	devfreq = <&bus_leftbus>;
> > +	status = "okay";
> > +};
> > +
> > +&cpu0 {
> > +	cpu0-supply = <&buck2_reg>;
> > +};
> > +
> > +&cpu_thermal {
> > +	cooling-maps {
> > +		map0 {
> > +			/* Corresponds to 800MHz at freq_table */
> > +			cooling-device = <&cpu0 7 7>, <&cpu1 7 7>, <&cpu2 7 7>, <&cpu3 7 7>;
> > +		};
> > +		map1 {
> > +			/* Corresponds to 200MHz at freq_table */
> > +			cooling-device = <&cpu0 13 13>, <&cpu1 13 13>, <&cpu2 13 13>, <&cpu3 13 13>;
> 
> This is too long. Please split it at 80 with indentation of next line
> starting at < from the previous one.
> 

Okay

> 
> > +		};
> > +	};
> > +};
> > +
> > +&exynos_usbphy {
> > +	status = "okay";
> > +};
> > +
> > +&fimd {
> > +	pinctrl-0 = <&lcd_clk &lcd_data24 &pwm1_out>;
> > +	pinctrl-names = "default";
> > +	status = "okay";
> > +
> > +	display-timings {
> > +		timing0 {
> > +			clock-frequency = <66666666>;
> > +			hactive = <1280>;
> > +			vactive = <800>;
> > +			hfront-porch = <18>;
> > +			hback-porch = <36>;
> > +			hsync-len = <16>;
> > +			vback-porch = <16>;
> > +			vfront-porch = <4>;
> > +			vsync-len = <3>;
> > +			hsync-active = <1>;
> > +		};
> > +	};
> 
> What happened with Marek's comment about this?
> 

Should have mentioned it in the introduction mail. I had a look at the
simple panel driver and it's not enough for the display in the p4note. I
asked Marek in IRC whether it's ok to have this in a separate patch set
to have a fully working display and he agreed.

> Best regards,
> Krzysztof