Received-SPF: pass (google.com: domain of linux-kernel+bounces-88129-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 1 Mar 2024 09:58:16 +0100
From: Marco Felsch <m.felsch@pengutronix.de>
To: Adam Ford <aford173@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org, Rob Herring <robh@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Fabio Estevam <festevam@gmail.com>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	devicetree@vger.kernel.org, Sascha Hauer <s.hauer@pengutronix.de>,
	aford@beaconembedded.com, linux-kernel@vger.kernel.org,
	imx@lists.linux.dev,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Shawn Guo <shawnguo@kernel.org>
Subject: Re: [PATCH 2/2] arm64: dts: imx8mp-beacon: Enable LVDS-1
Message-ID: <20240301085816.bam4ph43w7tikhea@pengutronix.de>
References: <20240229233556.116944-1-aford173@gmail.com>
 <20240229233556.116944-2-aford173@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240229233556.116944-2-aford173@gmail.com>

Hi Adam,

On 24-02-29, Adam Ford wrote:
> Beacon has an LVDS display that can connect to one of the
> LVDS ports on the baseboard.  The display requires a 30MHz
> clock to display properly, and the LDB needs to run at 7x that.
> With the audio CODEC now moved to the AUDIO_PLL1, the AUDIO_PLL2
> is now available to source the LDB at 210MHz and the DISP_PIX2.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> index 1f827ef38e36..731ee2667060 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts
> @@ -19,6 +19,26 @@ aliases {
>  		ethernet1 = &fec;
>  	};
>  
> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +		pwms = <&pwm2 0 100000 0>;
> +		power-supply = <&reg_lcd1_reset>;
> +		status = "okay";

status is not required here.

> +
> +		brightness-levels = < 0  1  2  3  4  5  6  7  8  9
> +				     10 11 12 13 14 15 16 17 18 19
> +				     20 21 22 23 24 25 26 27 28 29
> +				     30 31 32 33 34 35 36 37 38 39
> +				     40 41 42 43 44 45 46 47 48 49
> +				     50 51 52 53 54 55 56 57 58 59
> +				     60 61 62 63 64 65 66 67 68 69
> +				     70 71 72 73 74 75 76 77 78 79
> +				     80 81 82 83 84 85 86 87 88 89
> +				     90 91 92 93 94 95 96 97 98 99
> +				    100>;

Are you aware of: 'num-interpolated-steps' to avoid such arrays?

> +		default-brightness-level = <80>;
> +	};
> +
>  	chosen {
>  		stdout-path = &uart2;
>  	};
> @@ -135,6 +155,38 @@ led-3 {
>  		};
>  	};
>  
> +	lvds-1 {
> +		compatible = "panel-lvds";
> +		power-supply = <&reg_lcd1>;
> +		width-mm = <223>;
> +		height-mm = <125>;
> +		backlight = <&backlight>;
> +		data-mapping = "vesa-24";
> +
> +		panel-timing {
> +			/* 800x480@60Hz */
> +			clock-frequency = <30000000>;
> +			hactive = <800>;
> +			vactive = <480>;
> +			hsync-len = <48>;
> +			hfront-porch = <40>;
> +			hback-porch = <40>;
> +			vfront-porch = <13>;
> +			vback-porch = <29>;
> +			vsync-len = <1>;
> +			hsync-active = <1>;
> +			vsync-active = <3>;
> +			de-active = <1>;
> +			pixelclk-active = <0>;
> +		};

I would like to have a proper panel-simple.c entry but that's just my
POV of adding panels.

> +
> +		port {
> +			panel1_in: endpoint {
> +				remote-endpoint = <&ldb_lvds_ch1>;
> +			};
> +		};
> +	};
> +
>  	reg_audio: regulator-wm8962 {
>  		compatible = "regulator-fixed";
>  		regulator-name = "3v3_aud";
> @@ -144,6 +196,25 @@ reg_audio: regulator-wm8962 {
>  		enable-active-high;
>  	};
>  
> +	reg_lcd1_reset: regulator-lcd1-reset {
> +		compatible = "regulator-fixed";
> +		regulator-name = "LVDS-1 reset";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&pca6416_3 13 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		vin-supply = <&reg_lcd1>;
> +	};

This reset "regulator" seems more like a workaround, why don't you use
the reset-gpios property from "panel-lvds"?

Regards,
  Marco

> +
> +	reg_lcd1: regulator-lcd1 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "lvds-1 power";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&pca6416_3 14 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
>  	reg_usdhc2_vmmc: regulator-usdhc2 {
>  		compatible = "regulator-fixed";
>  		regulator-name = "VSD_3V3";
> @@ -457,6 +528,38 @@ &lcdif1 {
>  	status = "okay";
>  };
>  
> +&lcdif2 {
> +	status = "okay";
> +};
> +
> +&lvds_bridge {
> +	assigned-clocks = <&clk IMX8MP_CLK_MEDIA_LDB>, <&clk IMX8MP_AUDIO_PLL2_OUT>;
> +	assigned-clock-parents = <&clk IMX8MP_AUDIO_PLL2_OUT>;
> +	assigned-clock-rates = <210000000>, <210000000>;
> +	status = "okay";
> +
> +	ports {
> +		port@2 {
> +			ldb_lvds_ch1: endpoint {
> +				remote-endpoint = <&panel1_in>;
> +			};
> +		};
> +	};
> +};
> +
> +&media_blk_ctrl {
> +	assigned-clocks = <&clk IMX8MP_CLK_MEDIA_AXI>,
> +			  <&clk IMX8MP_CLK_MEDIA_APB>,
> +			  <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
> +			  <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
> +			  <&clk IMX8MP_VIDEO_PLL1>;
> +	assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
> +				 <&clk IMX8MP_SYS_PLL1_800M>,
> +				 <&clk IMX8MP_VIDEO_PLL1_OUT>,
> +				 <&clk IMX8MP_AUDIO_PLL2_OUT>;
> +	assigned-clock-rates = <500000000>, <200000000>, <0>, <0>, <1039500000>;
> +};
> +
>  &micfil {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_pdm>;
> @@ -496,6 +599,12 @@ &pcie_phy {
>  	status = "okay";
>  };
>  
> +&pwm2 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_pwm2>;
> +};
> +
>  &sai3 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&pinctrl_sai3>;
> @@ -596,6 +705,13 @@ &usdhc2 {
>  };
>  
>  &iomuxc {
> +
> +	pinctrl_pwm2: pwm2grp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_GPIO1_IO09__PWM2_OUT	0x116
> +		>;
> +	};
> +
>  	pinctrl_ecspi2: ecspi2grp {
>  		fsl,pins = <
>  			MX8MP_IOMUXC_ECSPI2_SCLK__ECSPI2_SCLK	0x82
> -- 
> 2.43.0
> 
> 
>