Re: (EXT) Re: [PATCH 2/2] arm64: dts: freescale: add initial device tree for TQMa8MPQL with i.MX8MP

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Krzysztof,

thanks for the fast feedback.

Am Donnerstag, 16. Juni 2022, 16:26:42 CEST schrieb Krzysztof Kozlowski:
> On 16/06/2022 07:22, Alexander Stein wrote:
> > This adds support for TQMa8MPQL module on MBa8MPxL board.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  arch/arm64/boot/dts/freescale/Makefile        |   1 +
> >  .../freescale/imx8mp-tqma8mpql-mba8mpxl.dts   | 689 ++++++++++++++++++
> >  .../boot/dts/freescale/imx8mp-tqma8mpql.dtsi  | 284 ++++++++
> >  3 files changed, 974 insertions(+)
> >  create mode 100644
> >  arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts create mode
> >  100644 arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql.dtsi> 
> > diff --git a/arch/arm64/boot/dts/freescale/Makefile
> > b/arch/arm64/boot/dts/freescale/Makefile index c288bda8aa36..ab8e61d4dd7c
> > 100644
> > --- a/arch/arm64/boot/dts/freescale/Makefile
> > +++ b/arch/arm64/boot/dts/freescale/Makefile
> > @@ -82,6 +82,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-venice-gw7902.dtb
> > 
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-icore-mx8mp-edimm2.2.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-phyboard-pollux-rdk.dtb
> > 
> > +dtb-$(CONFIG_ARCH_MXC) += imx8mp-tqma8mpql-mba8mpxl.dtb
> > 
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-venice-gw74xx.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-verdin-nonwifi-dahlia.dtb
> >  dtb-$(CONFIG_ARCH_MXC) += imx8mp-verdin-nonwifi-dev.dtb
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > b/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts new file
> > mode 100644
> > index 000000000000..e9755bd7619c
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp-tqma8mpql-mba8mpxl.dts
> > @@ -0,0 +1,689 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> > +/*
> > + * Copyright 2021-2022 TQ-Systems GmbH
> > + * Author: Alexander Stein <alexander.stein@tq-group.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/net/ti-dp83867.h>
> > +#include <dt-bindings/pwm/pwm.h>
> > +#include "imx8mp-tqma8mpql.dtsi"
> > +
> > +/ {
> > +	model = "TQ-Systems i.MX8MPlus TQMa8MPxL on MBa8MPxL";
> > +	compatible = "tq,imx8mp-tqma8mpql-mba8mpxl", "tq,imx8mp-tqma8mpql",
> > "fsl,imx8mp"; +
> > +	chosen {
> > +		stdout-path = &uart4;
> > +	};
> > +
> > +	iio-hwmon {
> > +		compatible = "iio-hwmon";
> > +		io-channels = <&adc 0>, <&adc 1>;
> > +	};
> > +
> > +	aliases {
> > +		mmc0 = &usdhc3;
> > +		mmc1 = &usdhc2;
> > +		mmc2 = &usdhc1;
> > +		rtc0 = &pcf85063;
> > +		rtc1 = &snvs_rtc;
> > +		spi0 = &flexspi;
> > +		spi1 = &ecspi1;
> > +		spi2 = &ecspi2;
> > +		spi3 = &ecspi3;
> > +	};
> > +
> > +	backlight_lvds: backlight-lvds {
> > +		compatible = "pwm-backlight";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_bllvds>;
> > +		pwms = <&pwm2 0 5000000 0>;
> > +		power-supply = <&reg_vcc_12v0>;
> > +		enable-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
> > +		status = "disabled";
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_gpiobutton>;
> > +		autorepeat;
> > +
> > +		switch1 {
> 
> swtich-1

Thanks, will change this.

> > +			label = "S12";
> > +			linux,code = <BTN_0>;
> > +			gpios = <&gpio5 26 GPIO_ACTIVE_LOW>;
> > +		};
> > +
> > +		switch2 {
> 
> switch-2

Dito.

> 
> > +			label = "S13";
> > +			linux,code = <BTN_1>;
> > +			gpios = <&gpio5 27 GPIO_ACTIVE_LOW>;
> > +		};
> > +	};
> > +
> > +	gpio-leds {
> > +		compatible = "gpio-leds";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_gpioled>;
> > +
> > +		led0 {
> 
> led-0

Will change this.

> > +			label = "led0";
> 
> Your label should be more useful... or just replace it with function and
> color.

I wasn't aware that function and color is preferred to label nowadays. I will 
use that instead.

> > +			gpios = <&gpio5 5 GPIO_ACTIVE_HIGH>;
> > +			linux,default-trigger = "default-on";
> > +		};
> > +
> > +		led1 {
> 
> led-1

Will change this.

> > +			label = "led1";
> > +			gpios = <&gpio5 4 GPIO_ACTIVE_HIGH>;
> > +			linux,default-trigger = "heartbeat";
> > +		};
> > +
> > +		led2 {
> 
> led-2

Will change this.

> > +			label = "led2";
> > +			gpios = <&gpio5 3 GPIO_ACTIVE_HIGH>;
> > +		};
> > +	};
> > +
> > +	lvds_panel: display {
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_lvdspanel>;
> > +		enable-gpios = <&gpio3 20 GPIO_ACTIVE_HIGH>;
> > +		backlight = <&backlight_lvds>;
> > +		status = "disabled";
> 
> What's this node about? There is no compatible.

As mentioned in the cover letter, some nodes are not used yet. In this case 
the intention is to apply an overlay for enabling the display later on. As the 
MBa8MPxL is a starter kit mainboard there is no fixed display attached. So a 
user might use our supported display or chose his own. But the backlight and 
display are fixed, so anything related to that can be part of the general 
mainboad DTS.
The display specific overlay will then have (among other things) the following 
snippet:

> &display0 {
> 	compatible = "tianma,tm070jvhg33";
> 	status = "okay";
> };

Power supply and enable GPIO are fixed, so there the can be part of the 
mainboard DTS while the actual compatible for the display might change.

I hope this makes this approach a bit more clear and this approach is 
acceptable.

Best regards,
Alexander