Re: [PATCH 1/2] arm64: dts: mediatek: mt8395-nio-12l: Prepare MIPI DSI port

[Julien Massot]

Hi Angelo,

On Mon, 2025-03-03 at 12:32 +0100, AngeloGioacchino Del Regno wrote:
> Il 26/02/25 15:35, Julien Massot ha scritto:
> > This board can use a MIPI-DSI panel on the DSI0 connector: in
> > preparation for adding an overlay for the Radxa Display 8HD,
> > add a pipeline connecting VDOSYS0 components to DSI0.
> > 
> > Also add the backlight, and some pin definitions available
> > through the DSI0 port.
> > 
> > Signed-off-by: Julien Massot <julien.massot@xxxxxxxxxxxxx>
> > ---
> >   .../boot/dts/mediatek/mt8395-radxa-nio-12l.dts     | 60 ++++++++++++++++++++++
> >   1 file changed, 60 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8395-radxa-nio-12l.dts
> > b/arch/arm64/boot/dts/mediatek/mt8395-radxa-nio-12l.dts
> > index 7184dc99296c7f5d749c7e6d378722677970b3b7..65c77e43d1cd4913b6741e25130febd746ff753c 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8395-radxa-nio-12l.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8395-radxa-nio-12l.dts
> > @@ -48,6 +48,17 @@ memory@40000000 {
> >   		reg = <0 0x40000000 0x1 0x0>;
> >   	};
> >   
> > +	backlight: backlight {
> > +		compatible = "pwm-backlight";
> > +		brightness-levels = <0 1023>;
> > +		default-brightness-level = <576>;
> > +		enable-gpios = <&pio 107 GPIO_ACTIVE_HIGH>;
> > +		num-interpolated-steps = <1023>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&dsi0_backlight_pins>;
> > +		pwms = <&disp_pwm0 0 500000>;
> 
> This should be disabled, unless there is a display connected to the DSI connector.
> 
> If there's no display, there's no point in enabling any backlight, as that pin
> may be reused somehow (with hardware hacks, maybe, yes, but still configuring
> the PWM IP and the pin as PWM while unused is at least a waste of energy).
> 
Yes, I will keep it disabled in the board dts.

> > +	};
> > +
> >   	wifi_vreg: regulator-wifi-3v3-en {
> >   		compatible = "regulator-fixed";
> >   		regulator-name = "wifi_3v3_en";
> > @@ -499,9 +510,20 @@ &mt6359_vsram_others_ldo_reg {
> >   	regulator-max-microvolt = <750000>;
> >   };
> >   
> > +&ovl0_in {
> > +	remote-endpoint = <&vdosys0_ep_main>;
> > +};
> > +
> 
> This goes to the overlay that enables the DSI display.
Ok

> 
> > @@ -912,6 +960,18 @@ &ssusb2 {
> >   	status = "okay";
> >   };
> >   
> > +&vdosys0 {
> > +	port {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		vdosys0_ep_main: endpoint@0 {
> > +			reg = <0>;
> > +			remote-endpoint = <&ovl0_in>;
> > +		};
> > +	};
> > +};
> 
> If you enable this path, in the event that the DSI display overlay is not added,
> the mediatek-drm driver will fail probing: even if a second path is enabled and
> that will succeed regardless of the first one failing, I don't see a reason why
> whoever doesn't have a DSI display attached should see a miserable failure in the
> kernel log :-)
> 
> Besides, mediatek-drm failing will also slow down boot for no reason.... etc etc.
> 
> Please move the display path setup to the DSI display overlay.
Sure will move the display path setup to the display overlay.

Thank you,
Julien