* Tomi Valkeinen <[email protected]> [190206 09:13]:
> On 05/02/2019 19:58, Tony Lindgren wrote:
> > * Tomi Valkeinen <[email protected]> [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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
