[PATCH v13 03/14] drm/mediatek: Add DSI sub driver

[Philipp Zabel]

Hi Daniel,

Am Mittwoch, den 09.03.2016, 22:07 +0800 schrieb Daniel Kurtz:
> Hi Philipp, CK,
> 
> Some small comments.
> Nothing that couldn't be addressed after merging, if you prefer.
> 
> On Tue, Mar 8, 2016 at 9:27 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
[...]
> > +static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > +{
> > +       struct device *dev = dsi->dev;
> > +       int ret;
> > +
> > +       if (++dsi->refcount != 1)
> > +               return 0;
> 
> What is the point of this refcount?
> I believe dsi->enabled already ensures poweron/poweroff calls are paired.

mtk_dsi_poweron() is called from both mtk_dsi_encoder_enable() and
mtk_dsi_ddp_start() and enables just enough of the DSI to provide power
and the pixel clock. The reason is that the DSI also provides the pixel
clock to the rest of the pipeline elements.

dsi->enabled only tracks the whole DSI being active after the call of
mtk_dsi_encoder_enable().

[...]
> > +static int mtk_dsi_create_conn_enc(struct drm_device *drm, struct mtk_dsi *dsi)
> > +{
> > +       int ret;
> > +
> > +       ret = drm_encoder_init(drm, &dsi->encoder, &mtk_dsi_encoder_funcs,
> > +                              DRM_MODE_ENCODER_DSI, NULL);
> > +       if (ret) {
> > +               DRM_ERROR("Failed to encoder init to drm\n");
> > +               return ret;
> > +       }
> > +       drm_encoder_helper_add(&dsi->encoder, &mtk_dsi_encoder_helper_funcs);
> > +
> > +       /*
> > +        * Currently display data paths are statically assigned to a crtc each.
> > +        * crtc 0 is OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0
> > +        */
> > +       dsi->encoder.possible_crtcs = 1;
> > +
> > +       /* Pre-empt DP connector creation if there's a bridge */
> > +       ret = mtk_drm_attach_bridge(dsi->bridge, &dsi->encoder);
> > +       if (!ret)
> > +               return 0;
> 
> nit: the above valid early termination of this function here is a bit unusual.
> It might be more clear if the "connector init" part below was split out into its
> own helper function.

Good point, will do that.

[...]
> > +static int mtk_dsi_remove(struct platform_device *pdev)
> > +{
> > +       struct mtk_dsi *dsi = platform_get_drvdata(pdev);
> > +
> > +       mtk_output_dsi_disable(dsi);
> 
> This one looks unmatched.

It is, indeed we should let the drm core disable the encoder before the
driver is removed.

[...]
> > +static int mtk_dsi_resume(struct device *dev)
> > +{
> > +       struct mtk_dsi *dsi;
> > +
> > +       dsi = dev_get_drvdata(dev);
> > +
> > +       mtk_output_dsi_enable(dsi);
> > +       DRM_DEBUG_DRIVER("dsi resume success!\n");
> > +
> > +       return 0;
> > +}
> > +#endif
> > +static SIMPLE_DEV_PM_OPS(mtk_dsi_pm_ops, mtk_dsi_suspend, mtk_dsi_resume);
> 
> I don't think we want PM ops.
> The MTK DRM driver disables/enables encoders during suspend/resume.

Yes, and that will also allow to merge mtk_output_dsi_disable() into
mtk_dsi_encoder_disable(), too.

[...]
> > +static unsigned long mtk_mipi_tx_pll_recalc_rate(struct clk_hw *hw,
> > +                                                unsigned long parent_rate)
> > +{
> > +       struct mtk_mipi_tx *mipi_tx = container_of(hw, struct mtk_mipi_tx,
> > +                                                  pll_hw);
> 
> An inline function / macro would help here.

Ok.

[...]
> > +static void mtk_mipi_tx_power_off_signal(struct phy *phy)
> > +{
> > +       struct mtk_mipi_tx *mipi_tx = phy_get_drvdata(phy);
> > +
> > +       mtk_mipi_tx_mask(mipi_tx, MIPITX_DSI_TOP_CON, RG_DSI_PAD_TIE_LOW_EN,
> > +                        RG_DSI_PAD_TIE_LOW_EN);
> 
> As mentioned in the HDMI review, _set_bits() / _clr_bits() is more readable than
> _mask() if we are just setting/clearing groups of bits.

I don't think
	mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_TOP_CON,
	                     RG_DSI_PAD_TIE_LOW_EN);
is that much easier to read. How about calling the function
mtk_mipi_tx_update_bits instead?

regards
Philipp