Re: [PATCH 11/11] arm64: dts: qcom: qcm6490: Add device-tree for Fairphone 5

["Luca Weiss" <luca.weiss@fairphone.com>]

On Wed Aug 30, 2023 at 12:45 PM CEST, Konrad Dybcio wrote:
> On 30.08.2023 11:58, Luca Weiss wrote:
> > Add device tree for the Fairphone 5 smartphone which is based on
> > the QCM6490 SoC.
> > 
> > Supported features are, as of now:
> > * Bluetooth
> > * Debug UART
> > * Display via simplefb
> > * Flash/torch LED
> > * Flip cover sensor
> > * Power & volume buttons
> > * RTC
> > * SD card
> > * USB
> > * Various plumbing like regulators, i2c, spi, etc
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  arch/arm64/boot/dts/qcom/Makefile                  |   1 +
> >  arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts | 659 +++++++++++++++++++++
> >  2 files changed, 660 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index 2cca20563a1d..73c3be0f8872 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -81,6 +81,7 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-sony-xperia-yoshino-lilac.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-sony-xperia-yoshino-maple.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-sony-xperia-yoshino-poplar.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= msm8998-xiaomi-sagit.dtb
> > +dtb-$(CONFIG_ARCH_QCOM)	+= qcm6490-fairphone-fp5.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= qcs404-evb-1000.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= qcs404-evb-4000.dtb
> >  dtb-$(CONFIG_ARCH_QCOM)	+= qdu1000-idp.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > new file mode 100644
> > index 000000000000..572b254d3af2
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/qcm6490-fairphone-fp5.dts
> > @@ -0,0 +1,659 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2023, Luca Weiss <luca.weiss@fairphone.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +/* PM7250B is configured to use SID8/9 instead of SID2/3 */
> > +#define PM7250B_SID 8
> > +#define PM7250B_SID1 9
> > +
> > +#include <dt-bindings/arm/qcom,ids.h>
> > +#include <dt-bindings/leds/common.h>
> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > +#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> > +#include "sc7280.dtsi"
> > +#include "pm7250b.dtsi"
> > +#include "pm7325.dtsi"
> > +#include "pm8350c.dtsi" /* PM7350C */
> > +#include "pmk8350.dtsi" /* PMK7325 */
> > +
> > +/ {
> > +	model = "Fairphone 5";
> > +	compatible = "fairphone,fp5", "qcom,qcm6490";
> > +	chassis-type = "handset";
> > +
> > +	/* required for bootloader to select correct board */
> > +	qcom,msm-id = <QCOM_ID_QCM6490 0x10000>;
> > +	qcom,board-id = <34 0>;
> Try [1], it worked for me on a picky device.

Yes, that works! Thanks!

>
> > +
> > +	aliases {
> > +		serial0 = &uart5;
> > +		serial1 = &uart7;
> > +	};
> > +
> > +	chosen {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		framebuffer0: framebuffer@a000000 {
> > +			compatible = "simple-framebuffer";
> > +			reg = <0 0xe1000000 0 (2700 * 1224 * 4)>;
> 0x0?

Ack

>
> > +			width = <1224>;
> > +			height = <2700>;
> > +			stride = <(1224 * 4)>;
> > +			format = "a8r8g8b8";
> > +			panel = <&panel>;
> > +			clocks = <&gcc GCC_DISP_HF_AXI_CLK>;
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +
> > +		pinctrl-0 = <&volume_down_default>, <&hall_sensor_default>;
> > +		pinctrl-names = "default";
> > +
> > +		key-volume-up {
> > +			label = "Volume up";
> > +			gpios = <&pm7325_gpios 6 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_VOLUMEUP>;
> > +		};
> > +
> > +		/* Hall sensor uses vreg_l8c as VDD, it's being enabled using
> > +		 * always-on on the regulator
> > +		 */
> /* Powered by the always-on vreg_l8c */ - fits in a single line

Ack

>
> > +		event-hall-sensor {
> > +			label = "Hall Effect Sensor";
> > +			gpios = <&tlmm 155 GPIO_ACTIVE_LOW>;
> > +			linux,input-type = <EV_SW>;
> > +			linux,code = <SW_LID>;
> > +			linux,can-disable;
> > +			wakeup-source;
> > +		};
> > +	};
> > +
> > +	panel: panel {
> > +		compatible = "boe,rm692e5";
> Undocumented compatbile

Not quite sure how to document this properly. This dummy panel node is
used for the simplefb/simpledrm so that user space can get panel
dimensions already.

This approach of a separate panel node was apparently preferred when the
functionality was upstreamed but of course as can be seen this panel
node is very barebones where many properties are not present which would
be needed with the full panel driver (once it's working).

Also for example for the bindings I think width-mm and height-mm won't
be needed anymore since that information will be provided by the driver
then.

Please let me know how/where to document this.

>
> > +
> > +		width-mm = <68>;
> > +		height-mm = <150>;
> > +	};
> > +
> > +	reserved-memory {
> > +		cont_splash_mem: cont-splash@e1000000 {
> > +			reg = <0 0xe1000000 0 0x2300000>;
> 0x0

Ack

>
> [...]
>
> > +		vreg_s1b: smps1 {
> > +			regulator-min-microvolt = <1840000>;
> > +			regulator-max-microvolt = <2040000>;
> No regulator-initial-mode on this pmic?

This specific regulator doesn't have one downstream, but it seems other
pm7325 regulators do have this downstream so I'll add it in v2.

>
> [...]
>
> > +
> > +	/* PM8008 PMIC @ 8 and 9 */
> > +	/* Pixelworks @ 26 */
> > +	/* FSA4480 USB audio switch @ 42 */
> There's a driver for the fsa4480, you may wanna include a node here

I'm aware, but will work on this later since it's not useful without
USB-C role switching and working audio.

>
> > +	/* AW86927FCR haptics @ 5a */
> > +};
> [...]
>
> > +
> > +	led-0 {
> > +		function = LED_FUNCTION_FLASH;
> No dual-tone stuff?

Nope.

>
> > +		color = <LED_COLOR_ID_WHITE>;
> > +		led-sources = <1>, <4>;
> > +		led-max-microamp = <500000>;
> > +		flash-max-microamp = <1500000>;
> > +		flash-max-timeout-us = <1280000>;
> > +	};
> > +};
> > +
> > +&pmk8350_rtc {
> > +	status = "okay";
> > +};
> > +
> > +&pon_pwrkey {
> > +	status = "okay";
> > +};
> > +
> > +&pon_resin {
> > +	status = "okay";
> > +	linux,code = <KEY_VOLUMEDOWN>;
> status last

Ack

>
> [...]
>
> > +&rmtfs_mem {
> > +	qcom,vmid = <QCOM_SCM_VMID_MSS_MSA>, <QCOM_SCM_VMID_NAV>;
> > +	reg = <0x0 0xf8500000 0x0 0x600000>;
> > +};
> /delete-node/ + redefinition would probably be cleaner-looking

Ack, looks better.

>
> [...]
>
> > +&uart7 {
> > +	/delete-property/interrupts;
> > +	interrupts-extended = <&intc GIC_SPI 608 IRQ_TYPE_LEVEL_HIGH>,
> > +			      <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
> property-n
> property-names

Ack

>
> > +
> > +	status = "okay";
> > +
> > +	bluetooth: bluetooth {
> > +		compatible = "qcom,wcn6750-bt";
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&bluetooth_enable_default>, <&sw_ctrl_default>;
> property-n
> property-names

Ack

Regards
Luca

>
> Konrad
>
> [1] https://lore.kernel.org/linux-arm-msm/9db02015-2c41-40d6-bf35-69ef277e9ce4@linaro.org/