Re: [PATCH v3 4/4] arm64: dts: rockchip: Add devicetree for Pine64 PineTab2

[Manuel Traut <manut@mecka.net>]

On Tue, Jan 02, 2024 at 07:07:56PM +0100, Ondřej Jirman wrote:
> Hello Manuel,

Hello Ondřej,

> On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > From: Alexander Warnecke <awarnecke002@hotmail.com>
> > 
> > [...]
> >
> > +
> > +	backlight: backlight {
> > +		compatible = "pwm-backlight";
> > +		pwms = <&pwm4 0 25000 0>;
> > +		brightness-levels = <20 220>;
> > +		num-interpolated-steps = <200>;
> 
> Does this linear brightness -> PWM duty cycle mapping lead to linear
> relationship between brighntess level and subjective brightness on this HW?
> 
> I doubt it a bit...

I tested it with the brightness slider in phosh, for me it looks good.

> > +
> > +	hdmi-con {
> 
> hdmi-connector

ack, changed for v4

> > +		compatible = "hdmi-connector";
> > +		type = "d";
> > +
> > +		port {
> > +			hdmi_con_in: endpoint {
> > +				remote-endpoint = <&hdmi_out_con>;
> > +			};
> > +		};
> > +	};
> > +
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> 
> Spurious newline ^

ack, changed for v4

> > +	vcc_3v3: vcc-3v3 {
> 
> Regulator node names shoule end with -regulator suffix. The same applies for
> all of the below nodes.

ack, changed for v4

> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_3v3";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc3v3_sys>;
> > +	};
> > +
> > +	vdd1v2_dvp: vdd1v2-dvp {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vdd1v2_dvp";
> > +		regulator-min-microvolt = <1200000>;
> > +		regulator-max-microvolt = <1200000>;
> > +		vin-supply = <&vcc_3v3>;
> > +		/*enable-supply = <&vcc2v8_dvp>;*/
> 
> There's no such property. Delete this commented out line.

ack, changed for v4

> > +	lcd: panel@0 {
> > +		compatible = "boe,th101mb31ig002-28a";
> > +		reg = <0>;
> > +		backlight = <&backlight>;
> > +		enable-gpios = <&gpio0 RK_PC7 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&lcd_pwren_h &lcd0_rst_l>;
> 
> Re lcd0_rst_l:
> 
> It's a bit weird conceptually to reference from dtsi something that's only 
> declared in dts that includes the dtsi. Maybe move pinctrl-* properties
> to dts &lcd, too...

Will be better I guess, changed for v4.

> > +			vcc5v_midu: BOOST {
> > +				regulator-always-on;
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <5000000>;
> > +				regulator-max-microvolt = <5000000>;
> > +				regulator-name = "boost";
> > +				regulator-state-mem {
> > +					regulator-off-in-suspend;
> 
> I guess this prevents remote USB wakeup by USB devices. Like wakeup via USB
> keyboard. Probably not a bad thing, though.

That is true. After 'echo mem > /sys/power/state' It is not possible to wakeup
the device with a USB Keyboard or mouse. However if the surface like keyboard
is used that is shipped with the device, wakeup works if the keyboard/tablet
gets unfold. For me this behaviour is fine. Other opinions?

> > +&pcie2x1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pcie_reset_h>;
> > +	reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> > +	vpcie3v3-supply = <&vcc3v3_minipcie>;
> > +	status = "okay";
> > +};
> 
> Does it make sense to enable this HW block by default, when it isn't used on
> actual HW?

There is a flat ribbon connector, if someone wants to build sth. it might be
helpful. However I am also fine with removing it for now.

> > +&pinctrl {
> > +	bt {
> > +		bt_wake_host_h: bt-wake-host-h {
> > +			rockchip,pins = <0 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
> > +		};
> > +	};
> 
> ^^^ unused

I do not bother to removing unused pinctrls, however even if there is no user at
the moment, if we look at a dtb as a machine parseable device description it
is probably ok, that it is there?

> > +
> > +	camerab {
> > +		camerab_pdn_l: camerab-pdn-l {
> > +			rockchip,pins = <4 RK_PB3 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +
> > +		camerab_rst_l: camerab-rst-l {
> > +			rockchip,pins = <4 RK_PB1 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +	};
> > +
> > +	cameraf {
> > +		cameraf_pdn_l: cameraf-pdn-l {
> > +			rockchip,pins = <4 RK_PB2 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +
> > +		cameraf_rst_l: cameraf-rst-l {
> > +			rockchip,pins = <4 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +	};
> 
> ^^^ unused
> 
> > +	usb {
> > +		usbcc_int_l: usbcc-int-l {
> > +			rockchip,pins = <0 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> 
> ^^^ unused
> 
> > +	wifi {
> > +		host_wake_wl: host-wake-wl {
> > +			rockchip,pins = <0 RK_PB7 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +
> > +		wifi_pwren: wifi-pwren {
> > +			rockchip,pins = <0 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +
> > +		wifi_wake_host_h: wifi-wake-host-h {
> > +			rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_down>;
> > +		};
> > +	};
> 
> ^^^ all of this wifi stuff is unused
> 
> Also wifi_pwren is not really useful on actual HW. W_VBAT is routed directly
> to wifi chip, with wifi_pwren_h signal having no effect: (short via R9664)
> 
>    https://megous.com/dl/tmp/b499859c1012f969.png

ack, removed for v4

> > +&uart1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&uart1m0_xfer
> > +		     &uart1m0_ctsn
> > +		     &uart1m0_rtsn>;
> > +	status = "okay";
> > +	uart-has-rtscts;
> > +};
> 
> Not sure about enabling UART for bluetooth, without having the bluetooth driver
> hooked in, somehow. UART will by default pull TX signal high, which may cause
> current leakage into gpio/uart pin of the bluetooth chip, if it's not powered up.
> 
> Maybe just remove this, until bluetooth is figured out...

Makes sense, removed for v4.

Thanks for your feedback,

Manuel