On Wed, Oct 25, 2023 at 06:39:57PM +0800, Keith Zhao wrote: > +static int starfive_hdmi_setup(struct starfive_hdmi *hdmi, > + struct drm_display_mode *mode) > +{ > + hdmi_modb(hdmi, STARFIVE_BIAS_CONTROL, STARFIVE_BIAS_ENABLE, STARFIVE_BIAS_ENABLE); > + hdmi_writeb(hdmi, STARFIVE_RX_CONTROL, STARFIVE_RX_ENABLE); > + hdmi->hdmi_data.vic = drm_match_cea_mode(mode); You're never using that variable. > + hdmi->tmds_rate = mode->clock * 1000; > + starfive_hdmi_phy_clk_set_rate(hdmi); > + > + while (!(hdmi_readb(hdmi, STARFIVE_PRE_PLL_LOCK_STATUS) & 0x1)) > + continue; > + while (!(hdmi_readb(hdmi, STARFIVE_POST_PLL_LOCK_STATUS) & 0x1)) > + continue; > + > + /*turn on LDO*/ > + hdmi_writeb(hdmi, STARFIVE_LDO_CONTROL, STARFIVE_LDO_ENABLE); > + /*turn on serializer*/ > + hdmi_writeb(hdmi, STARFIVE_SERIALIER_CONTROL, STARFIVE_SERIALIER_ENABLE); > + > + starfive_hdmi_tx_phy_power_down(hdmi); > + starfive_hdmi_config_video_timing(hdmi, mode); > + starfive_hdmi_tx_phy_power_on(hdmi); > + > + starfive_hdmi_tmds_driver_on(hdmi); > + starfive_hdmi_sync_tmds(hdmi); > + > + return 0; > +} > + > +static void starfive_hdmi_encoder_mode_set(struct drm_encoder *encoder, > + struct drm_display_mode *mode, > + struct drm_display_mode *adj_mode) > +{ > + struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder); > + > + drm_mode_copy(&hdmi->previous_mode, adj_mode); > +} You're never using that field, and it's not the previous mode, but the current mode. > +static void starfive_hdmi_encoder_enable(struct drm_encoder *encoder) > +{ > + struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder); > + struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; > + int ret, idx; > + struct drm_device *drm = hdmi->connector.dev; > + > + if (drm && !drm_dev_enter(drm, &idx)) > + return; > + > + ret = pm_runtime_get_sync(hdmi->dev); > + if (ret < 0) > + return; pm_runtime_resume_and_get is what you want here > + mdelay(10); ??? Sprinkling delays here and there isn't great. What is the issue this is trying to workaround? > + starfive_hdmi_setup(hdmi, mode); > + > + if (drm) > + drm_dev_exit(idx); > +} > + > +static void starfive_hdmi_encoder_disable(struct drm_encoder *encoder) > +{ > + struct starfive_hdmi *hdmi = encoder_to_hdmi(encoder); > + > + int idx; > + struct drm_device *drm = hdmi->connector.dev; > + > + if (drm && !drm_dev_enter(drm, &idx)) > + return; > + > + pm_runtime_put(hdmi->dev); pm_runtime calls should be safe anytime. If you need to protect them through a drm_dev_enter call, that call should be in the pm_runtime hook itself. > + if (drm) > + drm_dev_exit(idx); > +} > + > +static int > +starfive_hdmi_encoder_atomic_check(struct drm_encoder *encoder, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + bool valid = false; > + struct drm_display_mode *mode = &crtc_state->adjusted_mode; > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc_state); > + > + vs_crtc_state->encoder_type = encoder->encoder_type; > + vs_crtc_state->output_fmt = MEDIA_BUS_FMT_RGB888_1X24; Ok, so those two will always be ENCODER_TMDS and RGB888. Drop them from your CRTC state and use the proper defines there. > + const struct pre_pll_config *cfg = pre_pll_cfg_table; > + int pclk = mode->clock * 1000; > + > + for (; cfg->pixclock != 0; cfg++) { > + if (pclk == cfg->pixclock) { > + if (pclk > 297000000) > + continue; > + > + valid = true; > + break; > + } > + } > + > + return (valid) ? 0 : -EINVAL; > +} > + > +static const struct drm_encoder_helper_funcs starfive_hdmi_encoder_helper_funcs = { > + .enable = starfive_hdmi_encoder_enable, > + .disable = starfive_hdmi_encoder_disable, > + .mode_set = starfive_hdmi_encoder_mode_set, > + .atomic_check = starfive_hdmi_encoder_atomic_check, > +}; > + > +static enum drm_connector_status > +starfive_hdmi_connector_detect(struct drm_connector *connector, bool force) > +{ > + struct starfive_hdmi *hdmi = connector_to_hdmi(connector); > + struct drm_device *drm = hdmi->connector.dev; > + int ret; > + int idx; > + > + if (drm && !drm_dev_enter(drm, &idx)) > + return connector_status_disconnected; > + > + ret = pm_runtime_get_sync(hdmi->dev); > + if (ret < 0) > + return ret; > + > + ret = (hdmi_readb(hdmi, HDMI_STATUS) & m_HOTPLUG) ? > + connector_status_connected : connector_status_disconnected; > + pm_runtime_put(hdmi->dev); > + > + if (drm) > + drm_dev_exit(idx); > + > + return ret; > +} > + > +static int starfive_hdmi_connector_get_modes(struct drm_connector *connector) > +{ > + struct starfive_hdmi *hdmi = connector_to_hdmi(connector); > + struct edid *edid; > + int ret = 0; > + > + if (!hdmi->ddc) > + return 0; > + ret = pm_runtime_get_sync(hdmi->dev); > + if (ret < 0) > + return ret; > + > + edid = drm_get_edid(connector, hdmi->ddc); > + if (edid) { > + hdmi->hdmi_data.sink_is_hdmi = drm_detect_hdmi_monitor(edid); You're not using that field anywhere, and it's available in drm_display_info already. > + hdmi->hdmi_data.sink_has_audio = drm_detect_monitor_audio(edid); Ditto > +struct hdmi_data_info { > + int vic; > + bool sink_is_hdmi; > + bool sink_has_audio; > + unsigned int enc_in_format; You're not using it > + unsigned int enc_out_format; Ditto > + unsigned int colorimetry; Ditto Which means that as things stands, you can get rid of that entire structure. > +}; > + > +struct starfive_hdmi { > + struct device *dev; > + struct drm_device *drm_dev; > + struct drm_encoder encoder; > + struct drm_connector connector; > + > + struct starfive_hdmi_i2c *i2c; > + > + int irq; > + struct clk *sys_clk; > + struct clk *mclk; > + struct clk *bclk; > + struct reset_control *tx_rst; > + void __iomem *regs; > + > + struct i2c_adapter *ddc; > + > + unsigned long tmds_rate; You're not using that either. Maxime
Attachment:
signature.asc
Description: PGP signature