On Wed, Apr 23, 2025 at 03:54:13PM -0700, Abhinav Kumar wrote: > > > On 2/26/2025 6:25 PM, Dmitry Baryshkov wrote: > > The LVDS/LCDC controller uses pixel clock coming from the multimedia > > controller (mmcc) rather than using the PLL directly. Stop using LVDS > > PLL directly and register it as a clock provider. Use lcdc_clk as a > > pixel clock for the LCDC. > > > > Reviewed-by: Konrad Dybcio <[email protected]> > > Signed-off-by: Dmitry Baryshkov <[email protected]> > > --- > > drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h | 2 +- > > drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c | 8 +++++++- > > drivers/gpu/drm/msm/disp/mdp4/mdp4_lvds_pll.c | 22 > > +++++++--------------- > > 3 files changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > > index > > 142ccb68b435263f91ba1ab27676e426d43e5d84..b8bdc3712c73b14f3547dce3439a895e3d10f193 > > 100644 > > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.h > > @@ -207,6 +207,6 @@ static inline struct drm_encoder > > *mdp4_dsi_encoder_init(struct drm_device *dev) > > } > > #endif > > -struct clk *mpd4_lvds_pll_init(struct drm_device *dev); > > +int mpd4_lvds_pll_init(struct drm_device *dev); > > #endif /* __MDP4_KMS_H__ */ > > diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > > b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > > index > > 8bbc7fb881d599e7d309cc61bda83697fecd253a..db93795916cdaa87ac8e61d3b44c2dadac10fd9e > > 100644 > > --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c > > @@ -381,7 +381,13 @@ struct drm_encoder *mdp4_lcdc_encoder_init(struct > > drm_device *dev, > > drm_encoder_helper_add(encoder, &mdp4_lcdc_encoder_helper_funcs); > > /* TODO: do we need different pll in other cases? */ > > - mdp4_lcdc_encoder->lcdc_clk = mpd4_lvds_pll_init(dev); > > + ret = mpd4_lvds_pll_init(dev); > > + if (ret) { > > + DRM_DEV_ERROR(dev->dev, "failed to register LVDS PLL\n"); > > + return ERR_PTR(ret); > > + } > > + > > + mdp4_lcdc_encoder->lcdc_clk = devm_clk_get(dev->dev, "lcdc_clk"); > > if (IS_ERR(mdp4_lcdc_encoder->lcdc_clk)) { > > DRM_DEV_ERROR(dev->dev, "failed to get lvds_clk\n"); > > return ERR_CAST(mdp4_lcdc_encoder->lcdc_clk); > > Change seems fine to me, one question on the order of changes, DT change has > to be merged first otherwise it will fail here?
It is already semi-broken, as just enabling the PLL is not enough. The branch clocks in MMSS are to be toggled / manipulated. As such, it's questionable if we need to coordinate or not. > > Will that be managed by co-ordinating with the DT maintainer? > -- With best wishes Dmitry
