* Tomi Valkeinen <tomi.valkeinen@xxxxxx> [190206 09:13]:
> On 05/02/2019 19:58, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@xxxxxx> [190205 11:07]:
> >> Yep... So there's the DSI internal code which needs to deal with ulps
> >> and disconnect_lanes, and then the external interface to the DSI PLL (so
> >> that DPI can use DSI PLL) without ulps/disconnect.
> >>
> >> I think your patch breaks this latter one, as disconnect_lanes is zero
> >> in that case and would leave the regulator enabled. This would probably
> >> be visible on e.g. Pandaboard, which uses DSI PLLs for the TFP410 DVI
> >> output, if I recall right.
> >
> > Sorry I don't quite follow what happens there with dvi
> > calling into dsi.. Care to describe a bit more?
>
> We have the DSI PLL, which is "owned" by the DSI driver. Its main purpose is to clock the DSI. However, the output clock from the DSI PLL can also be muxed to go to the DPI (parallel video output, used for DVI on Panda). So for this use, the DPI driver enables, disables and configures the DSI PLL.
>
> When using DSI PLL from DPI, we don't care about this "disconnect_lanes", we always want to fully disable everything. But when using DSI PLL from DSI, we sometimes want to keep the lanes enabled using disconnect_lanes. So the functions these two users use are a bit different.
>
> That said, and looking at the code and trying to remember what all this is about... I think the disconnect_lanes is misplaced, and not related to the DSI PLL. The DSI code has degraded during the years quite a bit...
>
> I think the code should always enable the regulator in pll_enable, and always disable it in pll_disable. The disconnect_lanes should be handled separately from the PLL code, as it's not really related.
>
> Here's a quick edit about what I'm thing about, not tested:
OK I'll give it a try. Based on a quick glance, we need to still
check for enabled regulator to avoid unpaired calls.
> static int dsi_dump_dsi_clocks(struct seq_file *s, void *p)
> @@ -4108,6 +4094,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
>
> DSSDBG("PLL OK\n");
>
> + // XXX enable the regulator for the lanes
> + regulator_enable(dsi->vdds_dsi_reg);
> + dsi->vdds_dsi_enabled = true;
> +
So the above should only be done if !dsi->vdds_dsi_enabled?
> r = dsi_cio_init(dsi);
> if (r)
> goto err2;
> @@ -4136,6 +4126,10 @@ static int dsi_display_init_dsi(struct dsi_data *dsi)
> err3:
> dsi_cio_uninit(dsi);
> err2:
> + // XXX disable the regulator for the lanes
> + regulator_disable(dsi->vdds_dsi_reg);
> + dsi->vdds_dsi_enabled = false;
> +
And here only if dsi->vdds_dsi_enabled?
> @@ -4158,7 +4152,12 @@ static void dsi_display_uninit_dsi(struct dsi_data *dsi, bool disconnect_lanes,
>
> dss_select_dsi_clk_source(dsi->dss, dsi->module_id, DSS_CLK_SRC_FCK);
> dsi_cio_uninit(dsi);
> - dsi_pll_uninit(dsi, disconnect_lanes);
> + dss_pll_disable(&dsi->pll);
> +
> + if (disconnect_lanes) {
> + regulator_disable(dsi->vdds_dsi_reg);
> + dsi->vdds_dsi_enabled = false;
> + }
> }
Since they would be paired with the conditional handling
here?
Regards,
Tony
