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

[Daniel Kurtz]

On Tue, Mar 15, 2016 at 7:49 PM, Philipp Zabel <p.zabel at pengutronix.de> wrote:
>
> 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?

Actually, the important part here was to remove the redundant 'value'
parameter, and instead have dedicated clear / set functions that
operate on just the mask.



>
> regards
> Philipp
>