Re: [PATCH] ARM: dts: am335x-evm: Use drm simple-panel instead of tilcdc-panel

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jyri,

Thank you for the patch.

On Mon, Dec 02, 2019 at 08:57:29PM +0200, Jyri Sarha wrote:
> Move to use the new drm panel support in tilcdc together with added
> "tfc,s9700rtwv43tr-01b"-panel support in drm panel-simple.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> "tfc,s9700rtwv43tr-01b" in panel-simple has been in for some time now
> so it is about time to get this in too.
> 
>  arch/arm/boot/dts/am335x-evm.dts | 44 +++++++++++++-------------------
>  1 file changed, 18 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index a00145705c9b..0e286a1c9cc5 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -113,7 +113,7 @@
>  		};
>  	};
>  
> -	backlight {
> +	backlight: backlight {
>  		compatible = "pwm-backlight";
>  		pwms = <&ecap0 0 50000 0>;
>  		brightness-levels = <0 51 53 56 62 75 101 152 255>;
> @@ -121,35 +121,19 @@
>  	};
>  
>  	panel {
> -		compatible = "ti,tilcdc,panel";
> +		compatible = "tfc,s9700rtwv43tr-01b";
> +
>  		status = "okay";

I think this can be dropped as it's implicit.

> +
>  		pinctrl-names = "default";
>  		pinctrl-0 = <&lcd_pins_s0>;
> -		panel-info {
> -			ac-bias           = <255>;
> -			ac-bias-intrpt    = <0>;
> -			dma-burst-sz      = <16>;
> -			bpp               = <32>;
> -			fdd               = <0x80>;
> -			sync-edge         = <0>;
> -			sync-ctrl         = <1>;
> -			raster-order      = <0>;
> -			fifo-th           = <0>;
> -		};
>  
> -		display-timings {
> -			800x480p62 {
> -				clock-frequency = <30000000>;
> -				hactive = <800>;
> -				vactive = <480>;
> -				hfront-porch = <39>;
> -				hback-porch = <39>;
> -				hsync-len = <47>;
> -				vback-porch = <29>;
> -				vfront-porch = <13>;
> -				vsync-len = <2>;
> -				hsync-active = <1>;
> -				vsync-active = <1>;
> +		backlight = <&backlight>;

I would add a blank line here.

> +		ports {
> +			port {
> +				panel_0: endpoint@0 {
> +					remote-endpoint = <&lcdc_0>;
> +				};
>  			};
>  		};

The outter ports node isn't required if there's a single port, up to
you.

>  	};
> @@ -525,6 +509,14 @@
>  	status = "okay";
>  
>  	blue-and-red-wiring = "crossed";
> +
> +	ports {
> +		port {
> +			lcdc_0: endpoint@0 {
> +				remote-endpoint = <&panel_0>;
> +			};
> +		};
> +	};

Same here.

With the above issues addressed as applicable,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
>  
>  &elm {

-- 
Regards,

Laurent Pinchart