Re: [RFC PATCH] ARM: dts: imx53-qsb: Add MCIMX-LVDS1 display module support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dmitry,

On 07/27/2024, Dmitry Baryshkov wrote:
> On Fri, Jul 26, 2024 at 02:50:12PM GMT, Liu Ying wrote:
>> MCIMX-LVDS1[1] display module integrates a HannStar HSD100PXN1 LVDS
>> display panel and a touch IC.  Add an overlay to support the LVDS
>> panel on i.MX53 QSB / QSRB platforms.
>>
>> [1] https://www.nxp.com/part/MCIMX-LVDS1
>>
>> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
>> ---
>> I mark RFC in patch subject prefix because if the DT overlay is used, both ldb
>> and panel devices end up as devices deferred.  However, if the DT overlay is
>> not used and the devices are defined in imx53-qsb-common.dtsi, then they can be
>> probed ok.
>>
>> With a dev_err_probe() added to imx_ldb_probe() in imx-ldb.c, devices_deferred
>> indicates 53fa8008.ldb and panel-lvds kind of depend on each other.
>>
>> root@imx53qsb:~# cat /sys/kernel/debug/devices_deferred
>> 53fa8008.ldb    imx-ldb: failed to find panel or bridge for channel0
>> panel-lvds      platform: wait for supplier /soc/bus@50000000/ldb@53fa8008/lvds-channel@0
>>
>> It looks like the issue is related to fw_devlink, because if "fw_devlink=off"
>> is added to kernel bootup command line, then the issue doesn't happen.
> 
> Could you please fdtdump /sys/firmware/fdt (or just generated DTB files)
> in both cases and compare the dumps for sensible differences?

I fdtdump imx53-qsrb-mcimx-lvds1.dtb and imx53-qsrb.dtb.

I see three sensible differences.
1) panel-lvds node position.
   For imx53-qsrb-mcimx-lvds1.dtb, it comes very early and is next to
   'compatible = "fsl,imx53-qsrb", "fsl,imx53";'.
   For imx53-qsrb.dtb, it comes later and is next to panel node in '/' node.

2) properties order in panel-lvds node.
   For imx53-qsrb-mcimx-lvds1.dtb, it shows
   panel-lvds {                                                                 
        power-supply = <0x0000001c>;                                             
        backlight = <0x00000030>;                                                
        compatible = "hannstar,hsd100pxn1";                                      
        port {                                                                   
            endpoint {                                                           
                phandle = <0x0000007d>;                                          
                remote-endpoint = <0x0000007c>;                                  
            };                                                                   
        };                                                                       
    };
    For imx53-qsrb.dtb, it shows
    panel-lvds {                                                                 
        compatible = "hannstar,hsd100pxn1";                                      
        backlight = <0x00000031>;                                                
        power-supply = <0x0000001d>;                                             
        port {                                                                   
            endpoint {                                                           
                remote-endpoint = <0x00000033>;                                      
                phandle = <0x00000017>;                                              
            };                                                                   
        };                                                                       
    };         

3) No 'lvds0_out' and 'panel_lvds_in' in __symbols__ node for
   imx53-qsrb-mcimx-lvds1.dtb, but for imx53-qsrb.dtb they are in it.
lvds0_out = "/soc/bus@50000000/ldb@53fa8008/lvds-channel@0/port@2/endpoint";
panel_lvds_in = "/panel-lvds/port/endpoint";

BTW, reverting Saravana's commits
7cb50f6c9fba ("of: property: fw_devlink: Fix stupid bug in remote-endpoint parsing")
and/or
7fddac12c382 ("driver core: Fix device_link_flag_is_sync_state_only()")
avoids the issue from happening.

> 
>>
>> Saravana, DT folks, any ideas?
>>
>> Thanks.
>>
>>  arch/arm/boot/dts/nxp/imx/Makefile            |  4 ++
>>  .../boot/dts/nxp/imx/imx53-qsb-common.dtsi    |  4 +-
>>  .../dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso    | 43 +++++++++++++++++++
>>  3 files changed, 49 insertions(+), 2 deletions(-)
>>  create mode 100644 arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso
>>
>> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile b/arch/arm/boot/dts/nxp/imx/Makefile
>> index 92e291603ea1..7116889e1515 100644
>> --- a/arch/arm/boot/dts/nxp/imx/Makefile
>> +++ b/arch/arm/boot/dts/nxp/imx/Makefile
>> @@ -46,8 +46,10 @@ dtb-$(CONFIG_SOC_IMX53) += \
>>  	imx53-ppd.dtb \
>>  	imx53-qsb.dtb \
>>  	imx53-qsb-hdmi.dtb \
>> +	imx53-qsb-mcimx-lvds1.dtb \
>>  	imx53-qsrb.dtb \
>>  	imx53-qsrb-hdmi.dtb \
>> +	imx53-qsrb-mcimx-lvds1.dtb \
>>  	imx53-sk-imx53.dtb \
>>  	imx53-sk-imx53-atm0700d4-lvds.dtb \
>>  	imx53-sk-imx53-atm0700d4-rgb.dtb \
>> @@ -57,7 +59,9 @@ dtb-$(CONFIG_SOC_IMX53) += \
>>  	imx53-usbarmory.dtb \
>>  	imx53-voipac-bsb.dtb
>>  imx53-qsb-hdmi-dtbs := imx53-qsb.dtb imx53-qsb-hdmi.dtbo
>> +imx53-qsb-mcimx-lvds1-dtbs := imx53-qsb.dtb imx53-qsb-mcimx-lvds1.dtbo
>>  imx53-qsrb-hdmi-dtbs := imx53-qsrb.dtb imx53-qsb-hdmi.dtbo
>> +imx53-qsrb-mcimx-lvds1-dtbs := imx53-qsrb.dtb imx53-qsb-mcimx-lvds1.dtbo
>>  dtb-$(CONFIG_SOC_IMX6Q) += \
>>  	imx6dl-alti6p.dtb \
>>  	imx6dl-apf6dev.dtb \
>> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi b/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi
>> index 05d7a462ea25..430792a91ccf 100644
>> --- a/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi
>> +++ b/arch/arm/boot/dts/nxp/imx/imx53-qsb-common.dtsi
>> @@ -16,7 +16,7 @@ memory@70000000 {
>>  		      <0xb0000000 0x20000000>;
>>  	};
>>  
>> -	backlight_parallel: backlight-parallel {
>> +	backlight: backlight {
> 
> Nit: this seems unrelated to the LVDS support

Do you suggest to do this in a separate patch?
If yes, is it worth adding a Fixes tag?

> 
>>  		compatible = "pwm-backlight";
>>  		pwms = <&pwm2 0 5000000 0>;
>>  		brightness-levels = <0 4 8 16 32 64 128 255>;
>> @@ -89,7 +89,7 @@ panel_dpi: panel {
>>  		compatible = "sii,43wvf1g";
>>  		pinctrl-names = "default";
>>  		pinctrl-0 = <&pinctrl_display_power>;
>> -		backlight = <&backlight_parallel>;
>> +		backlight = <&backlight>;
>>  		enable-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>;
>>  
>>  		port {
>> diff --git a/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso b/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso
>> new file mode 100644
>> index 000000000000..27f6bedf3d39
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/nxp/imx/imx53-qsb-mcimx-lvds1.dtso
>> @@ -0,0 +1,43 @@
>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> +/*
>> + * Copyright 2024 NXP
>> + */
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +&{/} {
>> +	panel-lvds {
> 
> Nit: Just 'panel' should be enough.

Nope.

'panel-lvds' is needed to differentiate it from 'panel' in
imx53-qsb-common.dtsi which is a DPI panel.

Using 'panel-lvds', procfs lists exactly the properties needed.
root@imx53qsb:~# ls /proc/device-tree/panel-lvds/
backlight     compatible    name          port          power-supply

Using 'panel', more are listed.
root@imx53qsb:~# ls /proc/device-tree/panel/
backlight      compatible     enable-gpios   name           phandle        pinctrl-0      pinctrl-names  port           power-supply

> 
>> +		compatible = "hannstar,hsd100pxn1";
>> +		backlight = <&backlight>;
>> +		power-supply = <&reg_3p2v>;
>> +
>> +		port {
>> +			panel_lvds_in: endpoint {
>> +				remote-endpoint = <&lvds0_out>;
>> +			};
>> +		};
>> +	};
>> +};
>> +
>> +&ldb {
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	status = "okay";
>> +
>> +	lvds-channel@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		fsl,data-mapping = "spwg";
>> +		fsl,data-width = <18>;
>> +		status = "okay";
>> +
>> +		port@2 {
>> +			reg = <2>;
>> +
>> +			lvds0_out: endpoint {
>> +				remote-endpoint = <&panel_lvds_in>;
>> +			};
>> +		};
>> +	};
>> +};
>> -- 
>> 2.34.1
>>
> 

-- 
Regards,
Liu Ying





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux