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