Hi Tony, On 31/01/2019 05:32, Tony Lindgren wrote: > Currently dsi_display_init_dsi() calls dss_pll_enable() but it is not > paired with dss_pll_disable() in dsi_display_uninit_dsi(). This leaves But it is paired with dsi_pll_uninit(). > the DSS clocks enabled when the display is blanked wasting about extra > 5mW of power while idle. Which clocks? I think all the clocks are disabled. But if disconnect_lanes == false, the regulator is left enabled. If some clocks are left enabled, then that's a bug, but this didn't seem to be the case after a brief review of the code. > Let's fix this by setting a dsi->disconnect_lanes flag and making > the current dsi_pll_uninit() into dsi_pll_disable(). This way we can > just call dss_pll_disable() from dsi_display_uninit_dsi() and the > code becomes a bit easier to follow. It's been a long time since I worked on these DSI features, but: - If the regulator is disabled, the DSI lanes are in undefined state - If the DSI lanes are in undefined state, the panel often sees that as errors (or, in theory, might even read it as some real event) in the DSI lanes - If the panel driver can handle lanes in undefined state, it can set disconnect_lanes to true - ULPS needs the lanes to be in defined state (high/low, I can't recall) I can't remember the details, but I think there was a reason why the panel-dsi-cm.c doesn't disconnect the lanes... I also faintly remember that in Nokia we had some hacky code to change the pinmux to keep the pins in defined states even if the regulator got disabled. But that was never upstreamed. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
