On Tue, Mar 15, 2016 at 7:49 PM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html