As part of the discussion about converting tda998x to a bridge, here's some preparatory work for that, which includes a bunch of fixes. I'm sending this out _early_ as I'm not going to be working on any kernel stuff next week (it's likely I won't even be reading email.) So it may be a little rough around the edges.
Essentially, this is a series of cleanups, complexity removal, and avoiding races with the newly introduced audio support. Even without the bridge conversion, I think all these are still worthwhile to have.
This series of changes can also be found in my drm-tda998x-devel branch as an unstable series of commits (iow, I'm going to rebase/rework these at some point, so the commit IDs are not stable, so do not merge this.)
git://git.armlinux.org.uk/~rmk/linux-arm.git drm-tda998x-devel
drivers/gpu/drm/i2c/tda998x_drv.c | 782 +++++++++++++++++++------------------- 1 file changed, 394 insertions(+), 388 deletions(-)
As priv->audio_params can now be changed at run time, we need to be more careful about how we deal with a mode set. We must take the audio lock while checking if there's a valid audio configuration.
However, it's slightly worse than that - during mode set, we mute the audio, and it must not be unmuted until we have finished the mode set. It is possible that the audio side may start while a mode set is in progress, so take the audio_mutex lock around the whole mode setting procedure.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 9798d400d817..d72bc30a3bce 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1074,13 +1074,12 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
- if (priv->audio_params.format != AFMT_UNUSED) { - mutex_lock(&priv->audio_mutex); + mutex_lock(&priv->audio_mutex); + if (priv->audio_params.format != AFMT_UNUSED) tda998x_configure_audio(priv, &priv->audio_params, adjusted_mode->clock); - mutex_unlock(&priv->audio_mutex); - } + mutex_unlock(&priv->audio_mutex); } }
We must not configure the audio path when the sink is a DVI device, as DVI has no capability to receive HDMI audio. HDMI audio is a HDMI only feature, requiring HDMI infoframes.
There's a question concerning how to handle a DVI connected device when the audio device is opened - we save the audio configuration, so that if a HDMI device is hotplugged, audio will then work. This seems a reasonable expectation.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index d72bc30a3bce..495ee3fed661 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -44,6 +44,7 @@ struct tda998x_priv { u8 current_page; int dpms; bool is_hdmi_sink; + bool is_hdmi_config; u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; @@ -971,6 +972,8 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, div = 3; }
+ mutex_lock(&priv->audio_mutex); + /* mute the audio FIFO: */ reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -1074,13 +1077,14 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
- mutex_lock(&priv->audio_mutex); if (priv->audio_params.format != AFMT_UNUSED) tda998x_configure_audio(priv, &priv->audio_params, adjusted_mode->clock); - mutex_unlock(&priv->audio_mutex); } + + priv->is_hdmi_config = priv->is_hdmi_sink; + mutex_unlock(&priv->audio_mutex); }
static enum drm_connector_status @@ -1264,9 +1268,12 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, }
mutex_lock(&priv->audio_mutex); - ret = tda998x_configure_audio(priv, - &audio, - priv->encoder.crtc->hwmode.clock); + /* We must not program the TDA998x for audio if the sink is DVI. */ + if (priv->is_hdmi_config) + ret = tda998x_configure_audio(priv, &audio, + priv->encoder.crtc->hwmode.clock); + else + ret = 0;
if (ret == 0) priv->audio_params = audio;
Avoid a racy access to the mode clock by storing the current mode clock during a mode set under the audio mutex. This allows us to access it from the audio path in a safe way.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 495ee3fed661..858237f7f4d6 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -48,6 +48,7 @@ struct tda998x_priv { u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; + unsigned long tdms_clock; struct tda998x_audio_params audio_params;
struct platform_device *audio_pdev; @@ -714,8 +715,7 @@ static void tda998x_audio_mute(struct tda998x_priv *priv, bool on)
static int tda998x_configure_audio(struct tda998x_priv *priv, - struct tda998x_audio_params *params, - unsigned mode_clock) + struct tda998x_audio_params *params) { u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv; u32 n; @@ -772,7 +772,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, * assume 100MHz requires larger divider. */ adiv = AUDIO_DIV_SERCLK_8; - if (mode_clock > 100000) + if (priv->tdms_clock > 100000) adiv++; /* AUDIO_DIV_SERCLK_16 */
/* S/PDIF asks for a larger divider */ @@ -1077,10 +1077,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
+ priv->tdms_clock = adjusted_mode->clock; + if (priv->audio_params.format != AFMT_UNUSED) - tda998x_configure_audio(priv, - &priv->audio_params, - adjusted_mode->clock); + tda998x_configure_audio(priv, &priv->audio_params); }
priv->is_hdmi_config = priv->is_hdmi_sink; @@ -1230,9 +1230,6 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, .cea = params->cea, };
- if (!priv->encoder.crtc) - return -ENODEV; - memcpy(audio.status, params->iec.status, min(sizeof(audio.status), sizeof(params->iec.status)));
@@ -1270,8 +1267,7 @@ static int tda998x_audio_hw_params(struct device *dev, void *data, mutex_lock(&priv->audio_mutex); /* We must not program the TDA998x for audio if the sink is DVI. */ if (priv->is_hdmi_config) - ret = tda998x_configure_audio(priv, &audio, - priv->encoder.crtc->hwmode.clock); + ret = tda998x_configure_audio(priv, &audio); else ret = 0;
Correct two references to tda998x_connector_get_modes() which were incorrectly referring to tda998x_encoder_get_modes().
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 858237f7f4d6..134cd3f26a07 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -581,9 +581,9 @@ tda998x_reset(struct tda998x_priv *priv) * HPD assertion: it needs a delay of 100ms to avoid timing out while * trying to read EDID data. * - * However, tda998x_encoder_get_modes() may be called at any moment + * However, tda998x_connector_get_modes() may be called at any moment * after tda998x_connector_detect() indicates that we are connected, so - * we need to delay probing modes in tda998x_encoder_get_modes() after + * we need to delay probing modes in tda998x_connector_get_modes() after * we have seen a HPD inactive->active transition. This code implements * that delay. */
The naming of tda998x_encoder_set_config() is a left-over from when TDA998x was a slave encoder. Since this is part of the initialisation, drop the _encoder from the name, and move it near tda998x_bind().
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 40 +++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 134cd3f26a07..d3951aee2b09 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -822,25 +822,6 @@ tda998x_configure_audio(struct tda998x_priv *priv,
/* DRM encoder functions */
-static void tda998x_encoder_set_config(struct tda998x_priv *priv, - const struct tda998x_encoder_params *p) -{ - priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | - (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | - VIP_CNTRL_0_SWAP_B(p->swap_b) | - (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0); - priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) | - (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) | - VIP_CNTRL_1_SWAP_D(p->swap_d) | - (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0); - priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) | - (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) | - VIP_CNTRL_2_SWAP_F(p->swap_f) | - (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); - - priv->audio_params = p->audio_params; -} - static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) { struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); @@ -1608,6 +1589,25 @@ static const struct drm_connector_funcs tda998x_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
+static void tda998x_set_config(struct tda998x_priv *priv, + const struct tda998x_encoder_params *p) +{ + priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) | + (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) | + VIP_CNTRL_0_SWAP_B(p->swap_b) | + (p->mirr_b ? VIP_CNTRL_0_MIRR_B : 0); + priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(p->swap_c) | + (p->mirr_c ? VIP_CNTRL_1_MIRR_C : 0) | + VIP_CNTRL_1_SWAP_D(p->swap_d) | + (p->mirr_d ? VIP_CNTRL_1_MIRR_D : 0); + priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(p->swap_e) | + (p->mirr_e ? VIP_CNTRL_2_MIRR_E : 0) | + VIP_CNTRL_2_SWAP_F(p->swap_f) | + (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0); + + priv->audio_params = p->audio_params; +} + static int tda998x_bind(struct device *dev, struct device *master, void *data) { struct tda998x_encoder_params *params = dev->platform_data; @@ -1640,7 +1640,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) return ret;
if (!dev->of_node && params) - tda998x_encoder_set_config(priv, params); + tda998x_set_config(priv, params);
tda998x_encoder_set_polling(priv, &priv->connector);
Group the TDA998x connector functions and funcs structures together before the encoder support, rather than scattered amongst the rest of the file. This keeps like code together.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 316 +++++++++++++++++++------------------- 1 file changed, 159 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index d3951aee2b09..bb5389fbd059 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -820,6 +820,165 @@ tda998x_configure_audio(struct tda998x_priv *priv, return tda998x_write_aif(priv, ¶ms->cea); }
+/* DRM connector functions */ + +static int tda998x_connector_dpms(struct drm_connector *connector, int mode) +{ + if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC)) + return drm_atomic_helper_connector_dpms(connector, mode); + else + return drm_helper_connector_dpms(connector, mode); +} + +static enum drm_connector_status +tda998x_connector_detect(struct drm_connector *connector, bool force) +{ + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); + u8 val = cec_read(priv, REG_CEC_RXSHPDLEV); + + return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected : + connector_status_disconnected; +} + +static void tda998x_connector_destroy(struct drm_connector *connector) +{ + drm_connector_unregister(connector); + drm_connector_cleanup(connector); +} + +static const struct drm_connector_funcs tda998x_connector_funcs = { + .dpms = tda998x_connector_dpms, + .reset = drm_atomic_helper_connector_reset, + .fill_modes = drm_helper_probe_single_connector_modes, + .detect = tda998x_connector_detect, + .destroy = tda998x_connector_destroy, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length) +{ + struct tda998x_priv *priv = data; + u8 offset, segptr; + int ret, i; + + offset = (blk & 1) ? 128 : 0; + segptr = blk / 2; + + reg_write(priv, REG_DDC_ADDR, 0xa0); + reg_write(priv, REG_DDC_OFFS, offset); + reg_write(priv, REG_DDC_SEGM_ADDR, 0x60); + reg_write(priv, REG_DDC_SEGM, segptr); + + /* enable reading EDID: */ + priv->wq_edid_wait = 1; + reg_write(priv, REG_EDID_CTRL, 0x1); + + /* flag must be cleared by sw: */ + reg_write(priv, REG_EDID_CTRL, 0x0); + + /* wait for block read to complete: */ + if (priv->hdmi->irq) { + i = wait_event_timeout(priv->wq_edid, + !priv->wq_edid_wait, + msecs_to_jiffies(100)); + if (i < 0) { + dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i); + return i; + } + } else { + for (i = 100; i > 0; i--) { + msleep(1); + ret = reg_read(priv, REG_INT_FLAGS_2); + if (ret < 0) + return ret; + if (ret & INT_FLAGS_2_EDID_BLK_RD) + break; + } + } + + if (i == 0) { + dev_err(&priv->hdmi->dev, "read edid timeout\n"); + return -ETIMEDOUT; + } + + ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length); + if (ret != length) { + dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n", + blk, ret); + return ret; + } + + return 0; +} + +static int tda998x_connector_get_modes(struct drm_connector *connector) +{ + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); + struct edid *edid; + int n; + + /* + * If we get killed while waiting for the HPD timeout, return + * no modes found: we are not in a restartable path, so we + * can't handle signals gracefully. + */ + if (tda998x_edid_delay_wait(priv)) + return 0; + + if (priv->rev == TDA19988) + reg_clear(priv, REG_TX4, TX4_PD_RAM); + + edid = drm_do_get_edid(connector, read_edid_block, priv); + + if (priv->rev == TDA19988) + reg_set(priv, REG_TX4, TX4_PD_RAM); + + if (!edid) { + dev_warn(&priv->hdmi->dev, "failed to read EDID\n"); + return 0; + } + + drm_mode_connector_update_edid_property(connector, edid); + n = drm_add_edid_modes(connector, edid); + priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + drm_edid_to_eld(connector, edid); + + kfree(edid); + + return n; +} + +static int tda998x_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + /* TDA19988 dotclock can go up to 165MHz */ + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); + + if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000)) + return MODE_CLOCK_HIGH; + if (mode->htotal >= BIT(13)) + return MODE_BAD_HVALUE; + if (mode->vtotal >= BIT(11)) + return MODE_BAD_VVALUE; + return MODE_OK; +} + +static struct drm_encoder * +tda998x_connector_best_encoder(struct drm_connector *connector) +{ + struct tda998x_priv *priv = conn_to_tda998x_priv(connector); + + return &priv->encoder; +} + +static +const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = { + .get_modes = tda998x_connector_get_modes, + .mode_valid = tda998x_connector_mode_valid, + .best_encoder = tda998x_connector_best_encoder, +}; + /* DRM encoder functions */
static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) @@ -855,21 +1014,6 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) priv->dpms = mode; }
-static int tda998x_connector_mode_valid(struct drm_connector *connector, - struct drm_display_mode *mode) -{ - /* TDA19988 dotclock can go up to 165MHz */ - struct tda998x_priv *priv = conn_to_tda998x_priv(connector); - - if (mode->clock > ((priv->rev == TDA19988) ? 165000 : 150000)) - return MODE_CLOCK_HIGH; - if (mode->htotal >= BIT(13)) - return MODE_BAD_HVALUE; - if (mode->vtotal >= BIT(11)) - return MODE_BAD_VVALUE; - return MODE_OK; -} - static void tda998x_encoder_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, @@ -1068,109 +1212,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
-static enum drm_connector_status -tda998x_connector_detect(struct drm_connector *connector, bool force) -{ - struct tda998x_priv *priv = conn_to_tda998x_priv(connector); - u8 val = cec_read(priv, REG_CEC_RXSHPDLEV); - - return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected : - connector_status_disconnected; -} - -static int read_edid_block(void *data, u8 *buf, unsigned int blk, size_t length) -{ - struct tda998x_priv *priv = data; - u8 offset, segptr; - int ret, i; - - offset = (blk & 1) ? 128 : 0; - segptr = blk / 2; - - reg_write(priv, REG_DDC_ADDR, 0xa0); - reg_write(priv, REG_DDC_OFFS, offset); - reg_write(priv, REG_DDC_SEGM_ADDR, 0x60); - reg_write(priv, REG_DDC_SEGM, segptr); - - /* enable reading EDID: */ - priv->wq_edid_wait = 1; - reg_write(priv, REG_EDID_CTRL, 0x1); - - /* flag must be cleared by sw: */ - reg_write(priv, REG_EDID_CTRL, 0x0); - - /* wait for block read to complete: */ - if (priv->hdmi->irq) { - i = wait_event_timeout(priv->wq_edid, - !priv->wq_edid_wait, - msecs_to_jiffies(100)); - if (i < 0) { - dev_err(&priv->hdmi->dev, "read edid wait err %d\n", i); - return i; - } - } else { - for (i = 100; i > 0; i--) { - msleep(1); - ret = reg_read(priv, REG_INT_FLAGS_2); - if (ret < 0) - return ret; - if (ret & INT_FLAGS_2_EDID_BLK_RD) - break; - } - } - - if (i == 0) { - dev_err(&priv->hdmi->dev, "read edid timeout\n"); - return -ETIMEDOUT; - } - - ret = reg_read_range(priv, REG_EDID_DATA_0, buf, length); - if (ret != length) { - dev_err(&priv->hdmi->dev, "failed to read edid block %d: %d\n", - blk, ret); - return ret; - } - - return 0; -} - -static int tda998x_connector_get_modes(struct drm_connector *connector) -{ - struct tda998x_priv *priv = conn_to_tda998x_priv(connector); - struct edid *edid; - int n; - - /* - * If we get killed while waiting for the HPD timeout, return - * no modes found: we are not in a restartable path, so we - * can't handle signals gracefully. - */ - if (tda998x_edid_delay_wait(priv)) - return 0; - - if (priv->rev == TDA19988) - reg_clear(priv, REG_TX4, TX4_PD_RAM); - - edid = drm_do_get_edid(connector, read_edid_block, priv); - - if (priv->rev == TDA19988) - reg_set(priv, REG_TX4, TX4_PD_RAM); - - if (!edid) { - dev_warn(&priv->hdmi->dev, "failed to read EDID\n"); - return 0; - } - - drm_mode_connector_update_edid_property(connector, edid); - n = drm_add_edid_modes(connector, edid); - priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); - drm_edid_to_eld(connector, edid); - - kfree(edid); - - return n; -} - static void tda998x_encoder_set_polling(struct tda998x_priv *priv, struct drm_connector *connector) { @@ -1550,45 +1591,6 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy, };
-static struct drm_encoder * -tda998x_connector_best_encoder(struct drm_connector *connector) -{ - struct tda998x_priv *priv = conn_to_tda998x_priv(connector); - - return &priv->encoder; -} - -static -const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = { - .get_modes = tda998x_connector_get_modes, - .mode_valid = tda998x_connector_mode_valid, - .best_encoder = tda998x_connector_best_encoder, -}; - -static void tda998x_connector_destroy(struct drm_connector *connector) -{ - drm_connector_unregister(connector); - drm_connector_cleanup(connector); -} - -static int tda998x_connector_dpms(struct drm_connector *connector, int mode) -{ - if (drm_core_check_feature(connector->dev, DRIVER_ATOMIC)) - return drm_atomic_helper_connector_dpms(connector, mode); - else - return drm_helper_connector_dpms(connector, mode); -} - -static const struct drm_connector_funcs tda998x_connector_funcs = { - .dpms = tda998x_connector_dpms, - .reset = drm_atomic_helper_connector_reset, - .fill_modes = drm_helper_probe_single_connector_modes, - .detect = tda998x_connector_detect, - .destroy = tda998x_connector_destroy, - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, -}; - static void tda998x_set_config(struct tda998x_priv *priv, const struct tda998x_encoder_params *p) {
Separate out the connector initialisation from the rest of the drivers initialisation.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 58 +++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index bb5389fbd059..4379c6aa1c48 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -979,6 +979,37 @@ const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = { .best_encoder = tda998x_connector_best_encoder, };
+static int tda998x_connector_init(struct tda998x_priv *priv, + struct drm_device *drm) +{ + struct drm_connector *connector = &priv->connector; + int ret; + + connector->interlace_allowed = 1; + + if (priv->hdmi->irq) + connector->polled = DRM_CONNECTOR_POLL_HPD; + else + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; + + drm_connector_helper_add(connector, &tda998x_connector_helper_funcs); + ret = drm_connector_init(drm, connector, &tda998x_connector_funcs, + DRM_MODE_CONNECTOR_HDMIA); + if (ret) + return ret; + + ret = drm_connector_register(&priv->connector); + if (ret) { + drm_connector_cleanup(&priv->connector); + return ret; + } + + drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); + + return 0; +} + /* DRM encoder functions */
static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) @@ -1212,16 +1243,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
-static void tda998x_encoder_set_polling(struct tda998x_priv *priv, - struct drm_connector *connector) -{ - if (priv->hdmi->irq) - connector->polled = DRM_CONNECTOR_POLL_HPD; - else - connector->polled = DRM_CONNECTOR_POLL_CONNECT | - DRM_CONNECTOR_POLL_DISCONNECT; -} - static void tda998x_destroy(struct tda998x_priv *priv) { /* disable all IRQs and free the IRQ handler */ @@ -1634,7 +1655,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) crtcs = 1 << 0; }
- priv->connector.interlace_allowed = 1; priv->encoder.possible_crtcs = crtcs;
ret = tda998x_create(client, priv); @@ -1644,32 +1664,18 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) if (!dev->of_node && params) tda998x_set_config(priv, params);
- tda998x_encoder_set_polling(priv, &priv->connector); - drm_encoder_helper_add(&priv->encoder, &tda998x_encoder_helper_funcs); ret = drm_encoder_init(drm, &priv->encoder, &tda998x_encoder_funcs, DRM_MODE_ENCODER_TMDS, NULL); if (ret) goto err_encoder;
- drm_connector_helper_add(&priv->connector, - &tda998x_connector_helper_funcs); - ret = drm_connector_init(drm, &priv->connector, - &tda998x_connector_funcs, - DRM_MODE_CONNECTOR_HDMIA); + ret = tda998x_connector_init(priv, drm); if (ret) goto err_connector;
- ret = drm_connector_register(&priv->connector); - if (ret) - goto err_sysfs; - - drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); - return 0;
-err_sysfs: - drm_connector_cleanup(&priv->connector); err_connector: drm_encoder_cleanup(&priv->encoder); err_encoder:
Group the TDA998x audio functions together rather than split between two different locations in the file, keeping like code together.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 278 +++++++++++++++++++------------------- 1 file changed, 140 insertions(+), 138 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 4379c6aa1c48..6a7095b66a69 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -702,6 +702,8 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode) tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, &frame); }
+/* Audio support */ + static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) { if (on) { @@ -820,6 +822,144 @@ tda998x_configure_audio(struct tda998x_priv *priv, return tda998x_write_aif(priv, ¶ms->cea); }
+static int tda998x_audio_hw_params(struct device *dev, void *data, + struct hdmi_codec_daifmt *daifmt, + struct hdmi_codec_params *params) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + int i, ret; + struct tda998x_audio_params audio = { + .sample_width = params->sample_width, + .sample_rate = params->sample_rate, + .cea = params->cea, + }; + + memcpy(audio.status, params->iec.status, + min(sizeof(audio.status), sizeof(params->iec.status))); + + switch (daifmt->fmt) { + case HDMI_I2S: + if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || + daifmt->bit_clk_master || daifmt->frame_clk_master) { + dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, + daifmt->bit_clk_inv, daifmt->frame_clk_inv, + daifmt->bit_clk_master, + daifmt->frame_clk_master); + return -EINVAL; + } + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) + if (priv->audio_port[i].format == AFMT_I2S) + audio.config = priv->audio_port[i].config; + audio.format = AFMT_I2S; + break; + case HDMI_SPDIF: + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) + if (priv->audio_port[i].format == AFMT_SPDIF) + audio.config = priv->audio_port[i].config; + audio.format = AFMT_SPDIF; + break; + default: + dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt); + return -EINVAL; + } + + if (audio.config == 0) { + dev_err(dev, "%s: No audio configutation found\n", __func__); + return -EINVAL; + } + + mutex_lock(&priv->audio_mutex); + /* We must not program the TDA998x for audio if the sink is DVI. */ + if (priv->is_hdmi_config) + ret = tda998x_configure_audio(priv, &audio); + else + ret = 0; + + if (ret == 0) + priv->audio_params = audio; + mutex_unlock(&priv->audio_mutex); + + return ret; +} + +static void tda998x_audio_shutdown(struct device *dev, void *data) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + mutex_lock(&priv->audio_mutex); + + reg_write(priv, REG_ENA_AP, 0); + + priv->audio_params.format = AFMT_UNUSED; + + mutex_unlock(&priv->audio_mutex); +} + +int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + mutex_lock(&priv->audio_mutex); + + tda998x_audio_mute(priv, enable); + + mutex_unlock(&priv->audio_mutex); + return 0; +} + +static int tda998x_audio_get_eld(struct device *dev, void *data, + uint8_t *buf, size_t len) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + struct drm_mode_config *config = &priv->encoder.dev->mode_config; + struct drm_connector *connector; + int ret = -ENODEV; + + mutex_lock(&config->mutex); + list_for_each_entry(connector, &config->connector_list, head) { + if (&priv->encoder == connector->encoder) { + memcpy(buf, connector->eld, + min(sizeof(connector->eld), len)); + ret = 0; + } + } + mutex_unlock(&config->mutex); + + return ret; +} + +static const struct hdmi_codec_ops audio_codec_ops = { + .hw_params = tda998x_audio_hw_params, + .audio_shutdown = tda998x_audio_shutdown, + .digital_mute = tda998x_audio_digital_mute, + .get_eld = tda998x_audio_get_eld, +}; + +static int tda998x_audio_codec_init(struct tda998x_priv *priv, + struct device *dev) +{ + struct hdmi_codec_pdata codec_data = { + .ops = &audio_codec_ops, + .max_i2s_channels = 2, + }; + int i; + + for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) { + if (priv->audio_port[i].format == AFMT_I2S && + priv->audio_port[i].config != 0) + codec_data.i2s = 1; + if (priv->audio_port[i].format == AFMT_SPDIF && + priv->audio_port[i].config != 0) + codec_data.spdif = 1; + } + + priv->audio_pdev = platform_device_register_data( + dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO, + &codec_data, sizeof(codec_data)); + + return PTR_ERR_OR_ZERO(priv->audio_pdev); +} + /* DRM connector functions */
static int tda998x_connector_dpms(struct drm_connector *connector, int mode) @@ -1261,144 +1401,6 @@ static void tda998x_destroy(struct tda998x_priv *priv) i2c_unregister_device(priv->cec); }
-static int tda998x_audio_hw_params(struct device *dev, void *data, - struct hdmi_codec_daifmt *daifmt, - struct hdmi_codec_params *params) -{ - struct tda998x_priv *priv = dev_get_drvdata(dev); - int i, ret; - struct tda998x_audio_params audio = { - .sample_width = params->sample_width, - .sample_rate = params->sample_rate, - .cea = params->cea, - }; - - memcpy(audio.status, params->iec.status, - min(sizeof(audio.status), sizeof(params->iec.status))); - - switch (daifmt->fmt) { - case HDMI_I2S: - if (daifmt->bit_clk_inv || daifmt->frame_clk_inv || - daifmt->bit_clk_master || daifmt->frame_clk_master) { - dev_err(dev, "%s: Bad flags %d %d %d %d\n", __func__, - daifmt->bit_clk_inv, daifmt->frame_clk_inv, - daifmt->bit_clk_master, - daifmt->frame_clk_master); - return -EINVAL; - } - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) - if (priv->audio_port[i].format == AFMT_I2S) - audio.config = priv->audio_port[i].config; - audio.format = AFMT_I2S; - break; - case HDMI_SPDIF: - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) - if (priv->audio_port[i].format == AFMT_SPDIF) - audio.config = priv->audio_port[i].config; - audio.format = AFMT_SPDIF; - break; - default: - dev_err(dev, "%s: Invalid format %d\n", __func__, daifmt->fmt); - return -EINVAL; - } - - if (audio.config == 0) { - dev_err(dev, "%s: No audio configutation found\n", __func__); - return -EINVAL; - } - - mutex_lock(&priv->audio_mutex); - /* We must not program the TDA998x for audio if the sink is DVI. */ - if (priv->is_hdmi_config) - ret = tda998x_configure_audio(priv, &audio); - else - ret = 0; - - if (ret == 0) - priv->audio_params = audio; - mutex_unlock(&priv->audio_mutex); - - return ret; -} - -static void tda998x_audio_shutdown(struct device *dev, void *data) -{ - struct tda998x_priv *priv = dev_get_drvdata(dev); - - mutex_lock(&priv->audio_mutex); - - reg_write(priv, REG_ENA_AP, 0); - - priv->audio_params.format = AFMT_UNUSED; - - mutex_unlock(&priv->audio_mutex); -} - -int tda998x_audio_digital_mute(struct device *dev, void *data, bool enable) -{ - struct tda998x_priv *priv = dev_get_drvdata(dev); - - mutex_lock(&priv->audio_mutex); - - tda998x_audio_mute(priv, enable); - - mutex_unlock(&priv->audio_mutex); - return 0; -} - -static int tda998x_audio_get_eld(struct device *dev, void *data, - uint8_t *buf, size_t len) -{ - struct tda998x_priv *priv = dev_get_drvdata(dev); - struct drm_mode_config *config = &priv->encoder.dev->mode_config; - struct drm_connector *connector; - int ret = -ENODEV; - - mutex_lock(&config->mutex); - list_for_each_entry(connector, &config->connector_list, head) { - if (&priv->encoder == connector->encoder) { - memcpy(buf, connector->eld, - min(sizeof(connector->eld), len)); - ret = 0; - } - } - mutex_unlock(&config->mutex); - - return ret; -} - -static const struct hdmi_codec_ops audio_codec_ops = { - .hw_params = tda998x_audio_hw_params, - .audio_shutdown = tda998x_audio_shutdown, - .digital_mute = tda998x_audio_digital_mute, - .get_eld = tda998x_audio_get_eld, -}; - -static int tda998x_audio_codec_init(struct tda998x_priv *priv, - struct device *dev) -{ - struct hdmi_codec_pdata codec_data = { - .ops = &audio_codec_ops, - .max_i2s_channels = 2, - }; - int i; - - for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++) { - if (priv->audio_port[i].format == AFMT_I2S && - priv->audio_port[i].config != 0) - codec_data.i2s = 1; - if (priv->audio_port[i].format == AFMT_SPDIF && - priv->audio_port[i].config != 0) - codec_data.spdif = 1; - } - - priv->audio_pdev = platform_device_register_data( - dev, HDMI_CODEC_DRV_NAME, PLATFORM_DEVID_AUTO, - &codec_data, sizeof(codec_data)); - - return PTR_ERR_OR_ZERO(priv->audio_pdev); -} - /* I2C driver functions */
static int tda998x_get_audio_ports(struct tda998x_priv *priv,
tda998x_audio_get_eld() is needlessly complex - the connector associated with the encoder is always our own priv->connector. Remove this complexity, but ensure that there are no races when copying out the ELD.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 6a7095b66a69..547cf99ac32d 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -911,21 +911,13 @@ static int tda998x_audio_get_eld(struct device *dev, void *data, uint8_t *buf, size_t len) { struct tda998x_priv *priv = dev_get_drvdata(dev); - struct drm_mode_config *config = &priv->encoder.dev->mode_config; - struct drm_connector *connector; - int ret = -ENODEV; - - mutex_lock(&config->mutex); - list_for_each_entry(connector, &config->connector_list, head) { - if (&priv->encoder == connector->encoder) { - memcpy(buf, connector->eld, - min(sizeof(connector->eld), len)); - ret = 0; - } - } - mutex_unlock(&config->mutex);
- return ret; + mutex_lock(&priv->audio_mutex); + memcpy(buf, priv->connector.eld, + min(sizeof(priv->connector.eld), len)); + mutex_unlock(&priv->audio_mutex); + + return 0; }
static const struct hdmi_codec_ops audio_codec_ops = { @@ -1082,7 +1074,10 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + + mutex_lock(&priv->audio_mutex); drm_edid_to_eld(connector, edid); + mutex_unlock(&priv->audio_mutex);
kfree(edid);
Rather than storing the DPMS mode (which will always be on or off) use a boolean to store this instead.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 547cf99ac32d..1f9a25fe17f3 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -42,7 +42,7 @@ struct tda998x_priv { struct mutex mutex; u16 rev; u8 current_page; - int dpms; + bool is_on; bool is_hdmi_sink; bool is_hdmi_config; u8 vip_cntrl_0; @@ -1150,16 +1150,15 @@ static int tda998x_connector_init(struct tda998x_priv *priv, static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) { struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); + bool on;
/* we only care about on or off: */ - if (mode != DRM_MODE_DPMS_ON) - mode = DRM_MODE_DPMS_OFF; + on = mode == DRM_MODE_DPMS_ON;
- if (mode == priv->dpms) + if (on == priv->is_on) return;
- switch (mode) { - case DRM_MODE_DPMS_ON: + if (on) { /* enable video ports, audio will be enabled later */ reg_write(priv, REG_ENA_VP_0, 0xff); reg_write(priv, REG_ENA_VP_1, 0xff); @@ -1168,16 +1167,16 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0); reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1); reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2); - break; - case DRM_MODE_DPMS_OFF: + + priv->is_on = true; + } else { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00); reg_write(priv, REG_ENA_VP_1, 0x00); reg_write(priv, REG_ENA_VP_2, 0x00); - break; - }
- priv->dpms = mode; + priv->is_on = false; + } }
static void
dri-devel@lists.freedesktop.org
-
Russell King -
Russell King - ARM Linux