Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
This is independent of the component helper vs bridge safety issues.
drivers/gpu/drm/i2c/tda998x_drv.c | 288 ++++++++++++++++++++------------------ 1 file changed, 151 insertions(+), 137 deletions(-)
From: Peter Rosin peda@axentia.se
This prepares for being a drm_bridge which will not register the encoder. That makes the connector the better choice.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Peter Rosin peda@axentia.se Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index c3ebb9c4fc26..b05f54c8585b 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -753,7 +753,7 @@ static void tda998x_detect_work(struct work_struct *work) { struct tda998x_priv *priv = container_of(work, struct tda998x_priv, detect_work); - struct drm_device *dev = priv->encoder.dev; + struct drm_device *dev = priv->connector.dev;
if (dev) drm_kms_helper_hotplug_event(dev);
From: Peter Rosin peda@axentia.se
This fits better with the drm_bridge callbacks for when this driver becomes a drm_bridge.
Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Peter Rosin peda@axentia.se [edited by rmk to just split the tda998x_encoder_dpms() function and restore the double-disable protection we originally had, preserving original behaviour.] Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b05f54c8585b..1c2f5dac1886 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1299,18 +1299,9 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
/* DRM encoder functions */
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_enable(struct tda998x_priv *priv) { - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); - bool on; - - /* we only care about on or off: */ - on = mode == DRM_MODE_DPMS_ON; - - if (on == priv->is_on) - return; - - if (on) { + if (!priv->is_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); @@ -1321,7 +1312,12 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
priv->is_on = true; - } else { + } +} + +static void tda998x_disable(struct tda998x_priv *priv) +{ + if (!priv->is_on) { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00); reg_write(priv, REG_ENA_VP_1, 0x00); @@ -1331,6 +1327,23 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) } }
+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: */ + on = mode == DRM_MODE_DPMS_ON; + + if (on == priv->is_on) + return; + + if (on) + tda998x_enable(priv); + else + tda998x_disable(priv); +} + static void tda998x_encoder_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
On 2018-07-30 18:42, Russell King wrote:
From: Peter Rosin peda@axentia.se
This fits better with the drm_bridge callbacks for when this driver becomes a drm_bridge.
Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Peter Rosin peda@axentia.se [edited by rmk to just split the tda998x_encoder_dpms() function and restore the double-disable protection we originally had, preserving original behaviour.] Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
drivers/gpu/drm/i2c/tda998x_drv.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b05f54c8585b..1c2f5dac1886 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1299,18 +1299,9 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
/* DRM encoder functions */
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_enable(struct tda998x_priv *priv) {
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- bool on;
- /* we only care about on or off: */
- on = mode == DRM_MODE_DPMS_ON;
- if (on == priv->is_on)
return;- if (on) {
- if (!priv->is_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);
@@ -1321,7 +1312,12 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2);
priv->is_on = true;
- } else {
- }
+}
+static void tda998x_disable(struct tda998x_priv *priv) +{
- if (!priv->is_on) {
if (priv->is_on) {
I'm pretty sure that one's on you... :-)
Cheers, Peter
/* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00); reg_write(priv, REG_ENA_VP_1, 0x00);@@ -1331,6 +1327,23 @@ static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) } }
+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: */
- on = mode == DRM_MODE_DPMS_ON;
- if (on == priv->is_on)
return;- if (on)
tda998x_enable(priv);- else
tda998x_disable(priv);+}
static void tda998x_encoder_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
Move the non-DT configuration of the TDA998x into tda998x_create() so that we do all setup in one place.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 73 +++++++++++++++++++-------------------- 1 file changed, 35 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 1c2f5dac1886..843078e9fbf3 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1621,6 +1621,25 @@ static int tda998x_get_audio_ports(struct tda998x_priv *priv, return 0; }
+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_create(struct i2c_client *client, struct tda998x_priv *priv) { struct device_node *np = client->dev.of_node; @@ -1772,23 +1791,24 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) /* enable EDID read irq: */ reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
- if (!np) - return 0; /* non-DT */ + if (np) { + /* get the device tree parameters */ + ret = of_property_read_u32(np, "video-ports", &video); + if (ret == 0) { + priv->vip_cntrl_0 = video >> 16; + priv->vip_cntrl_1 = video >> 8; + priv->vip_cntrl_2 = video; + }
- /* get the device tree parameters */ - ret = of_property_read_u32(np, "video-ports", &video); - if (ret == 0) { - priv->vip_cntrl_0 = video >> 16; - priv->vip_cntrl_1 = video >> 8; - priv->vip_cntrl_2 = video; - } + ret = tda998x_get_audio_ports(priv, np); + if (ret) + goto fail;
- ret = tda998x_get_audio_ports(priv, np); - if (ret) - goto fail; - - if (priv->audio_port[0].format != AFMT_UNUSED) - tda998x_audio_codec_init(priv, &client->dev); + if (priv->audio_port[0].format != AFMT_UNUSED) + tda998x_audio_codec_init(priv, &client->dev); + } else if (client->dev.platform_data) { + tda998x_set_config(priv, client->dev.platform_data); + }
return 0;
@@ -1834,28 +1854,8 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy, };
-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; struct i2c_client *client = to_i2c_client(dev); struct drm_device *drm = data; struct tda998x_priv *priv; @@ -1883,9 +1883,6 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;
- if (!dev->of_node && params) - tda998x_set_config(priv, params); - 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);
Convert tda998x to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 154 +++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 843078e9fbf3..1ea62052f3e0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -69,6 +69,7 @@ struct tda998x_priv { bool edid_delay_active;
struct drm_encoder encoder; + struct drm_bridge bridge; struct drm_connector connector;
struct tda998x_audio_port audio_port[2]; @@ -79,9 +80,10 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, connector) - #define enc_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, encoder) +#define bridge_to_tda998x_priv(x) \ + container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify * things we encode the page # in upper bits of the register #. To read/ @@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector *connector) { struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return &priv->encoder; + return priv->bridge.encoder; }
static @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv *priv, if (ret) return ret;
- drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder); + drm_mode_connector_attach_encoder(&priv->connector, + priv->bridge.encoder);
return 0; }
-/* DRM encoder functions */ +/* DRM bridge functions */ + +static int tda998x_bridge_attach(struct drm_bridge *bridge) +{ + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + + return tda998x_connector_init(priv, bridge->dev); +} + +static void tda998x_bridge_detach(struct drm_bridge *bridge) +{ + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + + drm_connector_cleanup(&priv->connector); +}
-static void tda998x_enable(struct tda998x_priv *priv) +static void tda998x_bridge_enable(struct drm_bridge *bridge) { + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + if (!priv->is_on) { /* enable video ports, audio will be enabled later */ reg_write(priv, REG_ENA_VP_0, 0xff); @@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv) } }
-static void tda998x_disable(struct tda998x_priv *priv) +static void tda998x_bridge_disable(struct drm_bridge *bridge) { + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + if (!priv->is_on) { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00); @@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv) } }
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_bridge_mode_set(struct drm_bridge *bridge, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) { - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); - bool on; - - /* we only care about on or off: */ - on = mode == DRM_MODE_DPMS_ON; - - if (on == priv->is_on) - return; - - if (on) - tda998x_enable(priv); - else - tda998x_disable(priv); -} - -static void -tda998x_encoder_mode_set(struct drm_encoder *encoder, - struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) -{ - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); u16 ref_pix, ref_line, n_pix, n_line; u16 hs_pix_s, hs_pix_e; u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e; @@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
+static const struct drm_bridge_funcs tda998x_bridge_funcs = { + .attach = tda998x_bridge_attach, + .detach = tda998x_bridge_detach, + .disable = tda998x_bridge_disable, + .mode_set = tda998x_bridge_mode_set, + .enable = tda998x_bridge_enable, +}; + static void tda998x_destroy(struct tda998x_priv *priv) { + drm_bridge_remove(&priv->bridge); + /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); @@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex); + INIT_LIST_HEAD(&priv->bridge.list); init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); INIT_WORK(&priv->detect_work, tda998x_detect_work); @@ -1810,43 +1824,23 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) tda998x_set_config(priv, client->dev.platform_data); }
+ priv->bridge.funcs = &tda998x_bridge_funcs; + priv->bridge.of_node = dev->of_node; + + drm_bridge_add(&priv->bridge); + return 0;
fail: - /* if encoder_init fails, the encoder slave is never registered, - * so cleanup here: - */ - i2c_unregister_device(priv->cec); - if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); - if (client->irq) - free_irq(client->irq, priv); + tda998x_destroy(priv); err_irq: return ret; }
-static void tda998x_encoder_prepare(struct drm_encoder *encoder) -{ - tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF); -} - -static void tda998x_encoder_commit(struct drm_encoder *encoder) -{ - tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON); -} - -static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = { - .dpms = tda998x_encoder_dpms, - .prepare = tda998x_encoder_prepare, - .commit = tda998x_encoder_commit, - .mode_set = tda998x_encoder_mode_set, -}; +/* DRM encoder functions */
static void tda998x_encoder_destroy(struct drm_encoder *encoder) { - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder); - - tda998x_destroy(priv); drm_encoder_cleanup(encoder); }
@@ -1854,20 +1848,12 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy, };
-static int tda998x_bind(struct device *dev, struct device *master, void *data) +static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) { - struct i2c_client *client = to_i2c_client(dev); - struct drm_device *drm = data; - struct tda998x_priv *priv; + struct tda998x_priv *priv = dev_get_drvdata(dev); u32 crtcs = 0; int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; - - dev_set_drvdata(dev, priv); - if (dev->of_node) crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -1879,35 +1865,53 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
priv->encoder.possible_crtcs = crtcs;
- ret = tda998x_create(client, priv); - if (ret) - return ret; - - 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;
- ret = tda998x_connector_init(priv, drm); + ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL); if (ret) - goto err_connector; + goto err_bridge;
return 0;
-err_connector: +err_bridge: drm_encoder_cleanup(&priv->encoder); err_encoder: - tda998x_destroy(priv); return ret; }
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{ + struct i2c_client *client = to_i2c_client(dev); + struct drm_device *drm = data; + struct tda998x_priv *priv; + int ret; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + dev_set_drvdata(dev, priv); + + ret = tda998x_create(client, priv); + if (ret) + return ret; + + ret = tda998x_encoder_init(dev, drm); + if (ret) { + tda998x_destroy(priv); + return ret; + } + return 0; +} + static void tda998x_unbind(struct device *dev, struct device *master, void *data) { struct tda998x_priv *priv = dev_get_drvdata(dev);
- drm_connector_cleanup(&priv->connector); drm_encoder_cleanup(&priv->encoder); tda998x_destroy(priv); }
Hi!
This patch needs a refresh since commit cde4c44d8769 ("drm: drop _mode_ from drm_mode_connector_attach_encoder") interferes with hunk#4
On 2018-07-30 18:42, Russell King wrote:
Convert tda998x to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
drivers/gpu/drm/i2c/tda998x_drv.c | 154 +++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 843078e9fbf3..1ea62052f3e0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -69,6 +69,7 @@ struct tda998x_priv { bool edid_delay_active;
struct drm_encoder encoder;
struct drm_bridge bridge; struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
@@ -79,9 +80,10 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, connector)
#define enc_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, encoder) +#define bridge_to_tda998x_priv(x) \
- container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
- things we encode the page # in upper bits of the register #. To read/
@@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector *connector) { struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return &priv->encoder;
- return priv->bridge.encoder;
}
static @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv *priv, if (ret) return ret;
- drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
- drm_mode_connector_attach_encoder(&priv->connector,
priv->bridge.encoder);
cde4c44d8769 ("drm: drop _mode_ from drm_mode_connector_attach_encoder") conflicts with this hunk.
Cheers, Peter
return 0; }
-/* DRM encoder functions */ +/* DRM bridge functions */
+static int tda998x_bridge_attach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- return tda998x_connector_init(priv, bridge->dev);
+}
+static void tda998x_bridge_detach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- drm_connector_cleanup(&priv->connector);
+}
-static void tda998x_enable(struct tda998x_priv *priv) +static void tda998x_bridge_enable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* enable video ports, audio will be enabled later */ reg_write(priv, REG_ENA_VP_0, 0xff);
@@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv) } }
-static void tda998x_disable(struct tda998x_priv *priv) +static void tda998x_bridge_disable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00);
@@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv) } }
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode){
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- bool on;
- /* we only care about on or off: */
- on = mode == DRM_MODE_DPMS_ON;
- if (on == priv->is_on)
return;- if (on)
tda998x_enable(priv);- else
tda998x_disable(priv);-}
-static void -tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); u16 ref_pix, ref_line, n_pix, n_line; u16 hs_pix_s, hs_pix_e; u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
- .attach = tda998x_bridge_attach,
- .detach = tda998x_bridge_detach,
- .disable = tda998x_bridge_disable,
- .mode_set = tda998x_bridge_mode_set,
- .enable = tda998x_bridge_enable,
+};
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list); init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); INIT_WORK(&priv->detect_work, tda998x_detect_work);
@@ -1810,43 +1824,23 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) tda998x_set_config(priv, client->dev.platform_data); }
- priv->bridge.funcs = &tda998x_bridge_funcs;
- priv->bridge.of_node = dev->of_node;
- drm_bridge_add(&priv->bridge);
- return 0;
fail:
- /* if encoder_init fails, the encoder slave is never registered,
* so cleanup here:*/- i2c_unregister_device(priv->cec);
- if (priv->cec_notify)
cec_notifier_put(priv->cec_notify);- if (client->irq)
free_irq(client->irq, priv);
- tda998x_destroy(priv);
err_irq: return ret; }
-static void tda998x_encoder_prepare(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-static void tda998x_encoder_commit(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-}; +/* DRM encoder functions */
static void tda998x_encoder_destroy(struct drm_encoder *encoder) {
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- tda998x_destroy(priv); drm_encoder_cleanup(encoder);
}
@@ -1854,20 +1848,12 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy, };
-static int tda998x_bind(struct device *dev, struct device *master, void *data) +static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) {
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- struct tda998x_priv *priv = dev_get_drvdata(dev); u32 crtcs = 0; int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- if (dev->of_node) crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -1879,35 +1865,53 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
priv->encoder.possible_crtcs = crtcs;
ret = tda998x_create(client, priv);
if (ret)
return ret;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;
ret = tda998x_connector_init(priv, drm);
- ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL); if (ret)
goto err_connector;
goto err_bridge;return 0;
-err_connector: +err_bridge: drm_encoder_cleanup(&priv->encoder); err_encoder:
- tda998x_destroy(priv); return ret;
}
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
+}
static void tda998x_unbind(struct device *dev, struct device *master, void *data) { struct tda998x_priv *priv = dev_get_drvdata(dev);
- drm_connector_cleanup(&priv->connector); drm_encoder_cleanup(&priv->encoder); tda998x_destroy(priv);
}
On Mon, Jul 30, 2018 at 05:42:21PM +0100, Russell King wrote:
Convert tda998x to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
Hi Russell, Thanks for doing the bridge conversion, it certainly seems a better fit. I've cc'd the bridge maintainers/reviewer on this patch so that hopefully they will see it.
We should probably also move this driver into bridge/ once it's been reviewed.
drivers/gpu/drm/i2c/tda998x_drv.c | 154 +++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 843078e9fbf3..1ea62052f3e0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -69,6 +69,7 @@ struct tda998x_priv { bool edid_delay_active;
struct drm_encoder encoder;
struct drm_bridge bridge; struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
@@ -79,9 +80,10 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, connector)
#define enc_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, encoder) +#define bridge_to_tda998x_priv(x) \
- container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
- things we encode the page # in upper bits of the register #. To read/
@@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector *connector) { struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return &priv->encoder;
- return priv->bridge.encoder;
}
static @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv *priv, if (ret) return ret;
- drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
drm_mode_connector_attach_encoder(&priv->connector,
priv->bridge.encoder);return 0;
}
-/* DRM encoder functions */ +/* DRM bridge functions */
+static int tda998x_bridge_attach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- return tda998x_connector_init(priv, bridge->dev);
There doesn't seem to be any benefit to having tda998x_connector_init() as a separate function. I'd suggest just rolling that code in here.
+}
+static void tda998x_bridge_detach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- drm_connector_cleanup(&priv->connector);
+}
-static void tda998x_enable(struct tda998x_priv *priv) +static void tda998x_bridge_enable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* enable video ports, audio will be enabled later */ reg_write(priv, REG_ENA_VP_0, 0xff);
@@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv) } }
-static void tda998x_disable(struct tda998x_priv *priv) +static void tda998x_bridge_disable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00);
@@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv) } }
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode){
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- bool on;
- /* we only care about on or off: */
- on = mode == DRM_MODE_DPMS_ON;
- if (on == priv->is_on)
return;- if (on)
tda998x_enable(priv);- else
tda998x_disable(priv);-}
-static void -tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); u16 ref_pix, ref_line, n_pix, n_line; u16 hs_pix_s, hs_pix_e; u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
- .attach = tda998x_bridge_attach,
- .detach = tda998x_bridge_detach,
- .disable = tda998x_bridge_disable,
- .mode_set = tda998x_bridge_mode_set,
- .enable = tda998x_bridge_enable,
+};
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list); init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); INIT_WORK(&priv->detect_work, tda998x_detect_work);
@@ -1810,43 +1824,23 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) tda998x_set_config(priv, client->dev.platform_data); }
- priv->bridge.funcs = &tda998x_bridge_funcs;
- priv->bridge.of_node = dev->of_node;
- drm_bridge_add(&priv->bridge);
- return 0;
fail:
- /* if encoder_init fails, the encoder slave is never registered,
* so cleanup here:*/- i2c_unregister_device(priv->cec);
- if (priv->cec_notify)
cec_notifier_put(priv->cec_notify);- if (client->irq)
free_irq(client->irq, priv);
- tda998x_destroy(priv);
err_irq: return ret; }
-static void tda998x_encoder_prepare(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-static void tda998x_encoder_commit(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-};
Now that encoder is a stub, it should really be removed from here. The encoder should be instantiated elsewhere and attach the bridge to itself. There are a bunch of examples of this in bridge/
+/* DRM encoder functions */
static void tda998x_encoder_destroy(struct drm_encoder *encoder) {
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- tda998x_destroy(priv); drm_encoder_cleanup(encoder);
}
@@ -1854,20 +1848,12 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy,
Just use drm_encoder_cleanup directly.
};
-static int tda998x_bind(struct device *dev, struct device *master, void *data) +static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) {
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- struct tda998x_priv *priv = dev_get_drvdata(dev); u32 crtcs = 0; int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- if (dev->of_node) crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -1879,35 +1865,53 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
priv->encoder.possible_crtcs = crtcs;
ret = tda998x_create(client, priv);
if (ret)
return ret;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;
ret = tda998x_connector_init(priv, drm);
- ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL); if (ret)
goto err_connector;
goto err_bridge;return 0;
-err_connector: +err_bridge: drm_encoder_cleanup(&priv->encoder); err_encoder:
- tda998x_destroy(priv); return ret;
}
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
+}
static void tda998x_unbind(struct device *dev, struct device *master, void *data) { struct tda998x_priv *priv = dev_get_drvdata(dev);
- drm_connector_cleanup(&priv->connector); drm_encoder_cleanup(&priv->encoder); tda998x_destroy(priv);
}
2.7.4
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Aug 08, 2018 at 03:09:30PM -0400, Sean Paul wrote:
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-};
Now that encoder is a stub, it should really be removed from here. The encoder should be instantiated elsewhere and attach the bridge to itself. There are a bunch of examples of this in bridge/
That's not possible at present - this driver has to remain compatible with Armada and TI LCDC, both of which expect this driver to create the encoder.
In any case, bridges are buggy with unbinding/rebinding as I've pointed out several times in the past, but TDA998x used with Armada and TI LCDC as it currently stands are not. So, to do this as a full conversion to bridge and pushing the encoders into the DRM drivers results in a regression for these two DRM drivers. I'm not willing to accept such a regression, sorry.
Thanks for the other cleanup suggestions, they can be done with a later patch (these changes have already been been merged.)
On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote:
On Wed, Aug 08, 2018 at 03:09:30PM -0400, Sean Paul wrote:
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-};
Now that encoder is a stub, it should really be removed from here. The encoder should be instantiated elsewhere and attach the bridge to itself. There are a bunch of examples of this in bridge/
That's not possible at present - this driver has to remain compatible with Armada and TI LCDC, both of which expect this driver to create the encoder.
Hmm, yeah, I just read that thread. I suppose once bridges play nice with components the encoder can migrate into the drivers?
In any case, bridges are buggy with unbinding/rebinding as I've pointed out several times in the past, but TDA998x used with Armada and TI LCDC as it currently stands are not. So, to do this as a full conversion to bridge and pushing the encoders into the DRM drivers results in a regression for these two DRM drivers. I'm not willing to accept such a regression, sorry.
Thanks for the other cleanup suggestions, they can be done with a later patch (these changes have already been been merged.)
I still think you should get review from a bridge maintainer before it goes to drm-next.
Sean
-- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up
On Fri, Aug 10, 2018 at 12:11:05PM -0400, Sean Paul wrote:
On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote:
In any case, bridges are buggy with unbinding/rebinding as I've pointed out several times in the past, but TDA998x used with Armada and TI LCDC as it currently stands are not. So, to do this as a full conversion to bridge and pushing the encoders into the DRM drivers results in a regression for these two DRM drivers. I'm not willing to accept such a regression, sorry.
Thanks for the other cleanup suggestions, they can be done with a later patch (these changes have already been been merged.)
I still think you should get review from a bridge maintainer before it goes to drm-next.
They are free to review them any time they wish, all the patches are on dri-devel. However "before it goes to drm-next" has been impossible for a while now because David has already pulled it in.
Almost none of my DRM specific patches on dri-devel this time around received any feedback what so ever, even after myself and David chasing them up. Over the Armada changes, David Airlie eventually said to me "if it works for you, send it to me I suppose".
David also tried poking the tda998x/component discussion thread as well from the beginning of July (or so David told me), with seemingly the same result.
So, in summary, it's been almost impossible to get any feedback on any of the patches and discussions I've sent - I think extracting blood from a rock might be easier! ;)
I sent a patch about the "broadcast rgb" property - there are no replies to that email, and it was only in a completely different thread that I happened to notice a comment about that patch - it is not a sign of a healthy community to be providing feedback on patches by sending replies to other threads! Yet that seems to be happening on dri-devel!
I guess everyone could've gone to the beach (lucky them) because of the hot weather for the last couple of months, and now that it's cooled down in some parts, people are starting to re-engage...
If the bridge maintainers eventually get around to reviewing the changes, then, as I've said, any necessary changes can be made later, but what you ask is now impossible - they've been in drm-next since Tuesday evening, and by implication they've been in linux-next too - including last night's linux-next which is the final one before the merge window opens assuming it opens on Sunday.
On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote:
On Fri, Aug 10, 2018 at 12:11:05PM -0400, Sean Paul wrote:
On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote:
In any case, bridges are buggy with unbinding/rebinding as I've pointed out several times in the past, but TDA998x used with Armada and TI LCDC as it currently stands are not. So, to do this as a full conversion to bridge and pushing the encoders into the DRM drivers results in a regression for these two DRM drivers. I'm not willing to accept such a regression, sorry.
Thanks for the other cleanup suggestions, they can be done with a later patch (these changes have already been been merged.)
I still think you should get review from a bridge maintainer before it goes to drm-next.
They are free to review them any time they wish, all the patches are on dri-devel. However "before it goes to drm-next" has been impossible for a while now because David has already pulled it in.
Oh, I didn't realize it was already pulled into -next.
Almost none of my DRM specific patches on dri-devel this time around received any feedback what so ever, even after myself and David chasing them up. Over the Armada changes, David Airlie eventually said to me "if it works for you, send it to me I suppose".
David also tried poking the tda998x/component discussion thread as well from the beginning of July (or so David told me), with seemingly the same result.
So, in summary, it's been almost impossible to get any feedback on any of the patches and discussions I've sent - I think extracting blood from a rock might be easier! ;)
Yeah, I suspect a lot of people have blinders wrt tda given that it's off in i2c/. Feel free to ping me on irc in future if you're looking for a review. Once tda moves to bridge/ and drm-misc, it'll likely be much easier to get people engaged.
I sent a patch about the "broadcast rgb" property - there are no replies to that email, and it was only in a completely different thread that I happened to notice a comment about that patch - it is not a sign of a healthy community to be providing feedback on patches by sending replies to other threads! Yet that seems to be happening on dri-devel!
I guess everyone could've gone to the beach (lucky them) because of the hot weather for the last couple of months, and now that it's cooled down in some parts, people are starting to re-engage...
If the bridge maintainers eventually get around to reviewing the changes, then, as I've said, any necessary changes can be made later, but what you ask is now impossible - they've been in drm-next since Tuesday evening, and by implication they've been in linux-next too - including last night's linux-next which is the final one before the merge window opens assuming it opens on Sunday.
FYI, drm-misc (and by extension, bridge drivers) are under feature freeze after rc6 is cut. That soak time in linux-next has proven quite helpful for the stability of drm. Having the bridge conversion spend only a week in linux-next before merge window is not ideal. Again, nothing that can be done about that now, but keep that in mind for future.
Sean
-- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up
On Fri, Aug 10, 2018 at 01:02:29PM -0400, Sean Paul wrote:
On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote:
Almost none of my DRM specific patches on dri-devel this time around received any feedback what so ever, even after myself and David chasing them up. Over the Armada changes, David Airlie eventually said to me "if it works for you, send it to me I suppose".
David also tried poking the tda998x/component discussion thread as well from the beginning of July (or so David told me), with seemingly the same result.
So, in summary, it's been almost impossible to get any feedback on any of the patches and discussions I've sent - I think extracting blood from a rock might be easier! ;)
Yeah, I suspect a lot of people have blinders wrt tda given that it's off in i2c/. Feel free to ping me on irc in future if you're looking for a review. Once tda moves to bridge/ and drm-misc, it'll likely be much easier to get people engaged.
I suspect that those who actually use it don't have such blinders, yet they also have ignored the patches despite being copied with them. I can't see that the location of the driver would make any difference for them.
On Fri, Aug 10, 2018 at 06:16:30PM +0100, Russell King - ARM Linux wrote:
On Fri, Aug 10, 2018 at 01:02:29PM -0400, Sean Paul wrote:
On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote:
Almost none of my DRM specific patches on dri-devel this time around received any feedback what so ever, even after myself and David chasing them up. Over the Armada changes, David Airlie eventually said to me "if it works for you, send it to me I suppose".
David also tried poking the tda998x/component discussion thread as well from the beginning of July (or so David told me), with seemingly the same result.
So, in summary, it's been almost impossible to get any feedback on any of the patches and discussions I've sent - I think extracting blood from a rock might be easier! ;)
Yeah, I suspect a lot of people have blinders wrt tda given that it's off in i2c/. Feel free to ping me on irc in future if you're looking for a review. Once tda moves to bridge/ and drm-misc, it'll likely be much easier to get people engaged.
I suspect that those who actually use it don't have such blinders, yet they also have ignored the patches despite being copied with them. I can't see that the location of the driver would make any difference for them.
Expecting review to magically happen doesn't work. You need to actively solicit that, and often that involves a bit of reviewing trading. Especially anywhere where a part of the kernel is officially group-maintained like drm_bridge.
Given your past track record of handling other contributors I think it's entirely understandably that people do not choose to collaborate with you voluntarily. Fixing that is entirely up to you though.
I good first step here would imo be to move the tda driver under bridge, formally put it under the drm-misc group maintainership, actively solict and ack for that from other bridge maintainers and then also suggest other authors and contributors to the tda driver to join as committers and co-maintainers.
Thanks, Daniel
On Tue, Aug 14, 2018 at 12:42:42PM +0200, Daniel Vetter wrote:
Given your past track record of handling other contributors I think it's entirely understandably that people do not choose to collaborate with you voluntarily. Fixing that is entirely up to you though.
I do not work piecemeal. I work in blocks, so at some point, I'll switch to working on X, and then I'll work on X until that's complete before moving on to Y. It's more efficient use of my time, otherwise I have to constantly context switch - and that leads to mistakes.
I'm sorry that you don't think that's a good way of working, and you find that unacceptable, and you blame this on lack of collaboration.
On Tue, Aug 14, 2018 at 12:48 PM, Russell King - ARM Linux linux@armlinux.org.uk wrote:
On Tue, Aug 14, 2018 at 12:42:42PM +0200, Daniel Vetter wrote:
Given your past track record of handling other contributors I think it's entirely understandably that people do not choose to collaborate with you voluntarily. Fixing that is entirely up to you though.
I do not work piecemeal. I work in blocks, so at some point, I'll switch to working on X, and then I'll work on X until that's complete before moving on to Y. It's more efficient use of my time, otherwise I have to constantly context switch - and that leads to mistakes.
I'm sorry that you don't think that's a good way of working, and you find that unacceptable, and you blame this on lack of collaboration.
Maybe it's not clear, but I'm not referring to how you schedule your own time here. I've referred to your downright abuse behaviour in past years here on dri-devel, which only stopped once we've put a full formal Code of Conduct into place, including enforcement. Up to and including simply throwing existing maintainers out.
You blaming others for not reviewing your patches in this context is simply victim blaming. You do not get to blame others for the consequence of your past actions. It took you years of abuse emails to get there, it will take you years of hard constructive work and showing your best behaviour only, with no blaming others, to restore your reputation.
Thanks, Daniel
On 30.07.2018 18:42, Russell King wrote:
Convert tda998x to a bridge driver with built-in encoder support for compatibility with existing component drivers.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
drivers/gpu/drm/i2c/tda998x_drv.c | 154 +++++++++++++++++++------------------- 1 file changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 843078e9fbf3..1ea62052f3e0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -69,6 +69,7 @@ struct tda998x_priv { bool edid_delay_active;
struct drm_encoder encoder;
struct drm_bridge bridge; struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
@@ -79,9 +80,10 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, connector)
#define enc_to_tda998x_priv(x) \ container_of(x, struct tda998x_priv, encoder) +#define bridge_to_tda998x_priv(x) \
- container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
- things we encode the page # in upper bits of the register #. To read/
@@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector *connector) { struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return &priv->encoder;
- return priv->bridge.encoder;
}
static @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv *priv, if (ret) return ret;
- drm_mode_connector_attach_encoder(&priv->connector, &priv->encoder);
drm_mode_connector_attach_encoder(&priv->connector,
priv->bridge.encoder);return 0;
}
-/* DRM encoder functions */ +/* DRM bridge functions */
+static int tda998x_bridge_attach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- return tda998x_connector_init(priv, bridge->dev);
+}
+static void tda998x_bridge_detach(struct drm_bridge *bridge) +{
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- drm_connector_cleanup(&priv->connector);
+}
-static void tda998x_enable(struct tda998x_priv *priv) +static void tda998x_bridge_enable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* enable video ports, audio will be enabled later */ reg_write(priv, REG_ENA_VP_0, 0xff);
@@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv) } }
-static void tda998x_disable(struct tda998x_priv *priv) +static void tda998x_bridge_disable(struct drm_bridge *bridge) {
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- if (!priv->is_on) { /* disable video ports */ reg_write(priv, REG_ENA_VP_0, 0x00);
@@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv) } }
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode){
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- bool on;
- /* we only care about on or off: */
- on = mode == DRM_MODE_DPMS_ON;
- if (on == priv->is_on)
return;- if (on)
tda998x_enable(priv);- else
tda998x_disable(priv);-}
-static void -tda998x_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,struct drm_display_mode *adjusted_mode)-{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); u16 ref_pix, ref_line, n_pix, n_line; u16 hs_pix_s, hs_pix_e; u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, mutex_unlock(&priv->audio_mutex); }
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
- .attach = tda998x_bridge_attach,
- .detach = tda998x_bridge_detach,
- .disable = tda998x_bridge_disable,
- .mode_set = tda998x_bridge_mode_set,
- .enable = tda998x_bridge_enable,
+};
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list);
This line can be probably removed, unless there is a reason I am not aware of.
init_waitqueue_head(&priv->edid_delay_waitq); timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0); INIT_WORK(&priv->detect_work, tda998x_detect_work); @@ -1810,43 +1824,23 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) tda998x_set_config(priv, client->dev.platform_data); }
- priv->bridge.funcs = &tda998x_bridge_funcs;
- priv->bridge.of_node = dev->of_node;
- drm_bridge_add(&priv->bridge);
- return 0;
fail:
- /* if encoder_init fails, the encoder slave is never registered,
* so cleanup here:*/- i2c_unregister_device(priv->cec);
- if (priv->cec_notify)
cec_notifier_put(priv->cec_notify);- if (client->irq)
free_irq(client->irq, priv);
- tda998x_destroy(priv);
err_irq: return ret; }
-static void tda998x_encoder_prepare(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_OFF);
-}
-static void tda998x_encoder_commit(struct drm_encoder *encoder) -{
- tda998x_encoder_dpms(encoder, DRM_MODE_DPMS_ON);
-}
-static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
- .dpms = tda998x_encoder_dpms,
- .prepare = tda998x_encoder_prepare,
- .commit = tda998x_encoder_commit,
- .mode_set = tda998x_encoder_mode_set,
-}; +/* DRM encoder functions */
static void tda998x_encoder_destroy(struct drm_encoder *encoder) {
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- tda998x_destroy(priv); drm_encoder_cleanup(encoder);
}
@@ -1854,20 +1848,12 @@ static const struct drm_encoder_funcs tda998x_encoder_funcs = { .destroy = tda998x_encoder_destroy, };
-static int tda998x_bind(struct device *dev, struct device *master, void *data) +static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) {
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- struct tda998x_priv *priv = dev_get_drvdata(dev); u32 crtcs = 0; int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- if (dev->of_node) crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -1879,35 +1865,53 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
priv->encoder.possible_crtcs = crtcs;
ret = tda998x_create(client, priv);
if (ret)
return ret;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;
ret = tda998x_connector_init(priv, drm);
- ret = drm_bridge_attach(&priv->encoder, &priv->bridge, NULL); if (ret)
goto err_connector;
goto err_bridge;return 0;
-err_connector: +err_bridge: drm_encoder_cleanup(&priv->encoder); err_encoder:
- tda998x_destroy(priv); return ret;
}
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
It could be replaced by: Â Â Â ret = tda998x_encoder_init(dev, drm); Â Â Â if (ret) Â Â Â Â Â Â tda998x_destroy(priv); Â Â Â return ret;
but this is probably matter of taste.
Moreover I guess priv->is_on could be removed if enable/disable callbacks are called only by drm_core, but this is for another patch.
With removed initialization of bridge.list: Reviewed-by: Andrzej Hajda a.hajda@samsung.com
-- Regards Andrzej
+}
static void tda998x_unbind(struct device *dev, struct device *master, void *data) { struct tda998x_priv *priv = dev_get_drvdata(dev);
- drm_connector_cleanup(&priv->connector); drm_encoder_cleanup(&priv->encoder); tda998x_destroy(priv);
}
Hi Andrzej,
On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote:
On 30.07.2018 18:42, Russell King wrote:
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list);
This line can be probably removed, unless there is a reason I am not aware of.
The addition above of drm_bridge_remove() to tda998x_destroy() means that we end up calling this function in the error cleanup path. This avoids unnecessary complexity with lots of different gotos - tda998x has had a long history of not cleaning up stuff properly.
devm interfaces for bridge do not help avoid that - devm stuff only works if everything that is registered previously is cleaned up via devm mechanisms to ensure that a device's interface becomes unavailable before stuff (eg, edid timers, detect work) is started to be cleaned up. Otherwise, there's a chance of this stuff being triggered during tear-down.
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
It could be replaced by: Â Â Â ret = tda998x_encoder_init(dev, drm); Â Â Â if (ret) Â Â Â Â Â Â tda998x_destroy(priv); Â Â Â return ret;
but this is probably matter of taste.
It's not clear to me what "It" is - I think you're suggesting combining tda998x_create() and tda998x_encoder_init() ?
The code is structured this way to make the following patches easier - there is no point of combining things only to have to then break them apart again in a later patch. Please see patch 7, where tda998x_create() moves out of this function, where exactly this happens.
Moreover I guess priv->is_on could be removed if enable/disable callbacks are called only by drm_core, but this is for another patch.
Is it guaranteed that a bridge ->enable or ->disable callback won't be called twice, even for legacy drivers? I think atomic guarantees this but I don't think it's guaranteed for legacy drivers.
I'm guessing Rob had a reason why he added the check when he originally created the driver (encoder ->dpms can be called for the same dpms state multiple times?)
On 27.08.2018 19:59, Russell King - ARM Linux wrote:
Hi Andrzej,
On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote:
On 30.07.2018 18:42, Russell King wrote:
static void tda998x_destroy(struct tda998x_priv *priv) {
- drm_bridge_remove(&priv->bridge);
- /* disable all IRQs and free the IRQ handler */ cec_write(priv, REG_CEC_RXSHPDINTENA, 0); reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex);
- INIT_LIST_HEAD(&priv->bridge.list);
This line can be probably removed, unless there is a reason I am not aware of.
The addition above of drm_bridge_remove() to tda998x_destroy() means that we end up calling this function in the error cleanup path. This avoids unnecessary complexity with lots of different gotos - tda998x has had a long history of not cleaning up stuff properly.
1. bridge.list is/should be a private field of drm_bridge framework, so it's direct usage in driver looks like layer violation. 2. Calling drm_bridge_remove() without drm_bridge_add() is not strictly forbidden, but at least looks very suspicious. Even if current implementation tolerates it, it can change in the future.
Neither argument is a blocker IMO so if you prefer to stay with current solution please add a comment in the code explaining why do you initializes list field, the code at first sight looks suspicious.
devm interfaces for bridge do not help avoid that - devm stuff only works if everything that is registered previously is cleaned up via devm mechanisms to ensure that a device's interface becomes unavailable before stuff (eg, edid timers, detect work) is started to be cleaned up. Otherwise, there's a chance of this stuff being triggered during tear-down.
+static int tda998x_bind(struct device *dev, struct device *master, void *data) +{
- struct i2c_client *client = to_i2c_client(dev);
- struct drm_device *drm = data;
- struct tda998x_priv *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- dev_set_drvdata(dev, priv);
- ret = tda998x_create(client, priv);
- if (ret)
return ret;- ret = tda998x_encoder_init(dev, drm);
- if (ret) {
tda998x_destroy(priv);return ret;- }
- return 0;
It could be replaced by: Â Â Â ret = tda998x_encoder_init(dev, drm); Â Â Â if (ret) Â Â Â Â Â Â tda998x_destroy(priv); Â Â Â return ret;
but this is probably matter of taste.
It's not clear to me what "It" is - I think you're suggesting combining tda998x_create() and tda998x_encoder_init() ?
No, just simplifying error path.
The code is structured this way to make the following patches easier - there is no point of combining things only to have to then break them apart again in a later patch. Please see patch 7, where tda998x_create() moves out of this function, where exactly this happens.
OK. As I said: up to you.
Moreover I guess priv->is_on could be removed if enable/disable callbacks are called only by drm_core, but this is for another patch.
Is it guaranteed that a bridge ->enable or ->disable callback won't be called twice, even for legacy drivers? I think atomic guarantees this but I don't think it's guaranteed for legacy drivers.
I'm guessing Rob had a reason why he added the check when he originally created the driver (encoder ->dpms can be called for the same dpms state multiple times?)
OK, my guess was incorrect.
Regards
Andrzej
Move the tda998x_priv allocation inside tda998x_create() and simplify the tda998x_create()'s arguments. Pass the same to tda998x_destroy().
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 1ea62052f3e0..476161967742 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1567,8 +1567,10 @@ static const struct drm_bridge_funcs tda998x_bridge_funcs = { .enable = tda998x_bridge_enable, };
-static void tda998x_destroy(struct tda998x_priv *priv) +static void tda998x_destroy(struct device *dev) { + struct tda998x_priv *priv = dev_get_drvdata(dev); + drm_bridge_remove(&priv->bridge);
/* disable all IRQs and free the IRQ handler */ @@ -1653,13 +1655,21 @@ static void tda998x_set_config(struct tda998x_priv *priv, priv->audio_params = p->audio_params; }
-static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) +static int tda998x_create(struct device *dev) { + struct i2c_client *client = to_i2c_client(dev); struct device_node *np = client->dev.of_node; struct i2c_board_info cec_info; + struct tda998x_priv *priv; u32 video; int rev_lo, rev_hi, ret;
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + dev_set_drvdata(dev, priv); + mutex_init(&priv->mutex); /* protect the page access */ mutex_init(&priv->audio_mutex); /* protect access from audio thread */ mutex_init(&priv->edid_mutex); @@ -1832,7 +1842,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv) return 0;
fail: - tda998x_destroy(priv); + tda998x_destroy(dev); err_irq: return ret; } @@ -1884,24 +1894,16 @@ static int tda998x_encoder_init(struct device *dev, struct drm_device *drm)
static int tda998x_bind(struct device *dev, struct device *master, void *data) { - struct i2c_client *client = to_i2c_client(dev); struct drm_device *drm = data; - struct tda998x_priv *priv; int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); - if (!priv) - return -ENOMEM; - - dev_set_drvdata(dev, priv); - - ret = tda998x_create(client, priv); + ret = tda998x_create(dev); if (ret) return ret;
ret = tda998x_encoder_init(dev, drm); if (ret) { - tda998x_destroy(priv); + tda998x_destroy(dev); return ret; } return 0; @@ -1913,7 +1915,7 @@ static void tda998x_unbind(struct device *dev, struct device *master, struct tda998x_priv *priv = dev_get_drvdata(dev);
drm_encoder_cleanup(&priv->encoder); - tda998x_destroy(priv); + tda998x_destroy(dev); }
static const struct component_ops tda998x_ops = {
Cleanup the code a little from the effects of the previous changes: - Move tda998x_destroy() to be above tda998x_create() - Use 'dev' directly in tda998x_create() where appropriate.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 76 +++++++++++++++++++-------------------- 1 file changed, 37 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 476161967742..ea71602d1139 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1567,31 +1567,6 @@ static const struct drm_bridge_funcs tda998x_bridge_funcs = { .enable = tda998x_bridge_enable, };
-static void tda998x_destroy(struct device *dev) -{ - struct tda998x_priv *priv = dev_get_drvdata(dev); - - drm_bridge_remove(&priv->bridge); - - /* disable all IRQs and free the IRQ handler */ - cec_write(priv, REG_CEC_RXSHPDINTENA, 0); - reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); - - if (priv->audio_pdev) - platform_device_unregister(priv->audio_pdev); - - if (priv->hdmi->irq) - free_irq(priv->hdmi->irq, priv); - - del_timer_sync(&priv->edid_delay_timer); - cancel_work_sync(&priv->detect_work); - - i2c_unregister_device(priv->cec); - - if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); -} - /* I2C driver functions */
static int tda998x_get_audio_ports(struct tda998x_priv *priv, @@ -1655,6 +1630,31 @@ static void tda998x_set_config(struct tda998x_priv *priv, priv->audio_params = p->audio_params; }
+static void tda998x_destroy(struct device *dev) +{ + struct tda998x_priv *priv = dev_get_drvdata(dev); + + drm_bridge_remove(&priv->bridge); + + /* disable all IRQs and free the IRQ handler */ + cec_write(priv, REG_CEC_RXSHPDINTENA, 0); + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); + + if (priv->audio_pdev) + platform_device_unregister(priv->audio_pdev); + + if (priv->hdmi->irq) + free_irq(priv->hdmi->irq, priv); + + del_timer_sync(&priv->edid_delay_timer); + cancel_work_sync(&priv->detect_work); + + i2c_unregister_device(priv->cec); + + if (priv->cec_notify) + cec_notifier_put(priv->cec_notify); +} + static int tda998x_create(struct device *dev) { struct i2c_client *client = to_i2c_client(dev); @@ -1696,13 +1696,13 @@ static int tda998x_create(struct device *dev) /* read version: */ rev_lo = reg_read(priv, REG_VERSION_LSB); if (rev_lo < 0) { - dev_err(&client->dev, "failed to read version: %d\n", rev_lo); + dev_err(dev, "failed to read version: %d\n", rev_lo); return rev_lo; }
rev_hi = reg_read(priv, REG_VERSION_MSB); if (rev_hi < 0) { - dev_err(&client->dev, "failed to read version: %d\n", rev_hi); + dev_err(dev, "failed to read version: %d\n", rev_hi); return rev_hi; }
@@ -1713,20 +1713,19 @@ static int tda998x_create(struct device *dev)
switch (priv->rev) { case TDA9989N2: - dev_info(&client->dev, "found TDA9989 n2"); + dev_info(dev, "found TDA9989 n2"); break; case TDA19989: - dev_info(&client->dev, "found TDA19989"); + dev_info(dev, "found TDA19989"); break; case TDA19989N2: - dev_info(&client->dev, "found TDA19989 n2"); + dev_info(dev, "found TDA19989 n2"); break; case TDA19988: - dev_info(&client->dev, "found TDA19988"); + dev_info(dev, "found TDA19988"); break; default: - dev_err(&client->dev, "found unsupported device: %04x\n", - priv->rev); + dev_err(dev, "found unsupported device: %04x\n", priv->rev); return -ENXIO; }
@@ -1769,8 +1768,7 @@ static int tda998x_create(struct device *dev) tda998x_irq_thread, irq_flags, "tda998x", priv); if (ret) { - dev_err(&client->dev, - "failed to request IRQ#%u: %d\n", + dev_err(dev, "failed to request IRQ#%u: %d\n", client->irq, ret); goto err_irq; } @@ -1779,13 +1777,13 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); }
- priv->cec_notify = cec_notifier_get(&client->dev); + priv->cec_notify = cec_notifier_get(dev); if (!priv->cec_notify) { ret = -ENOMEM; goto fail; }
- priv->cec_glue.parent = &client->dev; + priv->cec_glue.parent = dev; priv->cec_glue.data = priv; priv->cec_glue.init = tda998x_cec_hook_init; priv->cec_glue.exit = tda998x_cec_hook_exit; @@ -1830,8 +1828,8 @@ static int tda998x_create(struct device *dev)
if (priv->audio_port[0].format != AFMT_UNUSED) tda998x_audio_codec_init(priv, &client->dev); - } else if (client->dev.platform_data) { - tda998x_set_config(priv, client->dev.platform_data); + } else if (dev->platform_data) { + tda998x_set_config(priv, dev->platform_data); }
priv->bridge.funcs = &tda998x_bridge_funcs;
Register the bridge outside of the component helper as we have drivers that wish to use the tda998x without its encoder.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index ea71602d1139..57a42269a7fb 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1893,18 +1893,8 @@ static int tda998x_encoder_init(struct device *dev, struct drm_device *drm) static int tda998x_bind(struct device *dev, struct device *master, void *data) { struct drm_device *drm = data; - int ret; - - ret = tda998x_create(dev); - if (ret) - return ret;
- ret = tda998x_encoder_init(dev, drm); - if (ret) { - tda998x_destroy(dev); - return ret; - } - return 0; + return tda998x_encoder_init(dev, drm); }
static void tda998x_unbind(struct device *dev, struct device *master, @@ -1913,7 +1903,6 @@ static void tda998x_unbind(struct device *dev, struct device *master, struct tda998x_priv *priv = dev_get_drvdata(dev);
drm_encoder_cleanup(&priv->encoder); - tda998x_destroy(dev); }
static const struct component_ops tda998x_ops = { @@ -1924,16 +1913,27 @@ static const struct component_ops tda998x_ops = { static int tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id) { + int ret; + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { dev_warn(&client->dev, "adapter does not support I2C\n"); return -EIO; } - return component_add(&client->dev, &tda998x_ops); + + ret = tda998x_create(&client->dev); + if (ret) + return ret; + + ret = component_add(&client->dev, &tda998x_ops); + if (ret) + tda998x_destroy(&client->dev); + return ret; }
static int tda998x_remove(struct i2c_client *client) { component_del(&client->dev, &tda998x_ops); + tda998x_destroy(&client->dev); return 0; }
On 30.07.2018 18:42, Russell King wrote:
Register the bridge outside of the component helper as we have drivers that wish to use the tda998x without its encoder.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
-- Regards Andrzej
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
Cheers, Peter
This is independent of the component helper vs bridge safety issues.
drivers/gpu/drm/i2c/tda998x_drv.c | 288 ++++++++++++++++++++------------------ 1 file changed, 151 insertions(+), 137 deletions(-)
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
On 2018-07-31 09:41, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
Right, meanwhile I massaged the patches into a combo of some local patches, next-20180730 and drm-misc-next, and tested that. The conflict was trivial once I had a closer look...
And it seems to work (with atmel-hlcdc), so you can add
Tested-by: Peter Rosin peda@axentia.se
Thank you for very much for picking this up!
Cheers, Peter
On Tue, Jul 31, 2018 at 09:53:57AM +0200, Peter Rosin wrote:
On 2018-07-31 09:41, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
Right, meanwhile I massaged the patches into a combo of some local patches, next-20180730 and drm-misc-next, and tested that. The conflict was trivial once I had a closer look...
And it seems to work (with atmel-hlcdc), so you can add
Tested-by: Peter Rosin peda@axentia.se
Thanks, I've added your attributation, and fixed patch 2 as you pointed out.
I have a four more tda998x patches if you're willing to test. These follow on from this set - I've tweaked patch 2 in this follow on set to cater for the removal of "_mode_" in drm_mode_connector_update_edid_property.
drivers/gpu/drm/i2c/tda998x_drv.c | 106 ++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 55 deletions(-)
Move the mode_valid() implementation to the bridge instead of the connector, as we're checking the bridge's capabilities.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 8ca5c9786bdf..58831b6a4722 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1244,21 +1244,6 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) 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) { @@ -1270,7 +1255,6 @@ tda998x_connector_best_encoder(struct drm_connector *connector) 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, };
@@ -1316,6 +1300,21 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) drm_connector_cleanup(&priv->connector); }
+static int tda998x_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_mode *mode) +{ + /* TDA19988 dotclock can go up to 165MHz */ + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + + 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_bridge_enable(struct drm_bridge *bridge) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); @@ -1562,6 +1561,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, static const struct drm_bridge_funcs tda998x_bridge_funcs = { .attach = tda998x_bridge_attach, .detach = tda998x_bridge_detach, + .mode_valid = tda998x_bridge_mode_valid, .disable = tda998x_bridge_disable, .mode_set = tda998x_bridge_mode_set, .enable = tda998x_bridge_enable,
On 31.07.2018 11:26, Russell King wrote:
Move the mode_valid() implementation to the bridge instead of the connector, as we're checking the bridge's capabilities.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
Reviewed-by: Andrzej Hajda a.hajda@samsung.com
-- Regards Andrzej
drivers/gpu/drm/i2c/tda998x_drv.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 8ca5c9786bdf..58831b6a4722 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1244,21 +1244,6 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) 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) { @@ -1270,7 +1255,6 @@ tda998x_connector_best_encoder(struct drm_connector *connector) 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,
};
@@ -1316,6 +1300,21 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) drm_connector_cleanup(&priv->connector); }
+static int tda998x_bridge_mode_valid(struct drm_bridge *bridge,
const struct drm_display_mode *mode)+{
- /* TDA19988 dotclock can go up to 165MHz */
- struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
- 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_bridge_enable(struct drm_bridge *bridge) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); @@ -1562,6 +1561,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, static const struct drm_bridge_funcs tda998x_bridge_funcs = { .attach = tda998x_bridge_attach, .detach = tda998x_bridge_detach,
- .mode_valid = tda998x_bridge_mode_valid, .disable = tda998x_bridge_disable, .mode_set = tda998x_bridge_mode_set, .enable = tda998x_bridge_enable,
We can achieve the same effect via the get_modes() method, rather than wrapping the fill_modes helper.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 58831b6a4722..98ec91a55a6a 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1097,29 +1097,6 @@ static int tda998x_audio_codec_init(struct tda998x_priv *priv,
/* DRM connector functions */
-static int tda998x_connector_fill_modes(struct drm_connector *connector, - uint32_t maxX, uint32_t maxY) -{ - struct tda998x_priv *priv = conn_to_tda998x_priv(connector); - int ret; - - mutex_lock(&priv->audio_mutex); - ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); - - if (connector->edid_blob_ptr) { - struct edid *edid = (void *)connector->edid_blob_ptr->data; - - cec_notifier_set_phys_addr_from_edid(priv->cec_notify, edid); - - priv->sink_has_audio = drm_detect_monitor_audio(edid); - } else { - priv->sink_has_audio = false; - } - mutex_unlock(&priv->audio_mutex); - - return ret; -} - static enum drm_connector_status tda998x_connector_detect(struct drm_connector *connector, bool force) { @@ -1138,7 +1115,7 @@ static void tda998x_connector_destroy(struct drm_connector *connector) static const struct drm_connector_funcs tda998x_connector_funcs = { .dpms = drm_helper_connector_dpms, .reset = drm_atomic_helper_connector_reset, - .fill_modes = tda998x_connector_fill_modes, + .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, @@ -1237,7 +1214,12 @@ static int tda998x_connector_get_modes(struct drm_connector *connector) }
drm_connector_update_edid_property(connector, edid); + cec_notifier_set_phys_addr_from_edid(priv->cec_notify, edid); + + mutex_lock(&priv->audio_mutex); n = drm_add_edid_modes(connector, edid); + priv->sink_has_audio = drm_detect_monitor_audio(edid); + mutex_unlock(&priv->audio_mutex);
kfree(edid);
The serializer PLL divider is a power-of-two divider, so our calculation which assumes that it's a numerical divider is incorrect. Replace it with one that results in a power-of-two divider value instead.
Tested with all supported modes with a Samsung S24C750.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 98ec91a55a6a..a31809ce30e2 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1334,6 +1334,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, struct drm_display_mode *adjusted_mode) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + unsigned long tmds_clock; u16 ref_pix, ref_line, n_pix, n_line; u16 hs_pix_s, hs_pix_e; u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e; @@ -1404,12 +1405,19 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, (mode->vsync_end - mode->vsync_start)/2; }
- div = 148500 / mode->clock; - if (div != 0) { - div--; - if (div > 3) - div = 3; - } + tmds_clock = mode->clock; + + /* + * The divisor is power-of-2. The TDA9983B datasheet gives + * this as ranges of Msample/s, which is 10x the TMDS clock: + * 0 - 800 to 1500 Msample/s + * 1 - 400 to 800 Msample/s + * 2 - 200 to 400 Msample/s + * 3 - as 2 above + */ + for (div = 0; div < 3; div++) + if (80000 >> div <= tmds_clock) + break;
mutex_lock(&priv->audio_mutex);
TDA998x has no support for pixel repeated modes, and the code notes this as a "TODO" item. The implementation appears to be relatively simple, so lets add it.
We need to calculate the serializer clock divisor based on the TMDS clock rate, set the repeat control, and set the serializer pixel repeat count. Since the audio code needs the actual TMDS clock, record that.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk --- drivers/gpu/drm/i2c/tda998x_drv.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a31809ce30e2..08ce28cbdec4 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -241,6 +241,7 @@ struct tda998x_priv { # define HVF_CNTRL_1_PAD(x) (((x) & 3) << 4) # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ +# define RPT_CNTRL_REPEAT(x) ((x) & 15) #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ # define I2S_FORMAT(x) (((x) & 3) << 0) #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ @@ -1342,7 +1343,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, u16 vwin1_line_s, vwin1_line_e; u16 vwin2_line_s, vwin2_line_e; u16 de_pix_s, de_pix_e; - u8 reg, div, rep; + u8 reg, div, rep, sel_clk;
/* * Internally TDA998x is using ITU-R BT.656 style sync but @@ -1405,7 +1406,16 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, (mode->vsync_end - mode->vsync_start)/2; }
- tmds_clock = mode->clock; + /* + * Select pixel repeat depending on the double-clock flag + * (which means we have to repeat each pixel once.) + */ + rep = mode->flags & DRM_MODE_FLAG_DBLCLK ? 1 : 0; + sel_clk = SEL_CLK_ENA_SC_CLK | SEL_CLK_SEL_CLK1 | + SEL_CLK_SEL_VRF_CLK(rep ? 2 : 0); + + /* the TMDS clock is scaled up by the pixel repeat */ + tmds_clock = mode->clock * (1 + rep);
/* * The divisor is power-of-2. The TDA9983B datasheet gives @@ -1421,6 +1431,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
mutex_lock(&priv->audio_mutex);
+ priv->tmds_clock = tmds_clock; + /* mute the audio FIFO: */ reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -1443,12 +1455,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, reg_write(priv, REG_SERIALIZER, 0); reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
- /* TODO enable pixel repeat for pixel rates less than 25Msamp/s */ - rep = 0; - reg_write(priv, REG_RPT_CNTRL, 0); - reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) | - SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK); - + reg_write(priv, REG_RPT_CNTRL, RPT_CNTRL_REPEAT(rep)); + reg_write(priv, REG_SEL_CLK, sel_clk); reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) | PLL_SERIAL_2_SRL_PR(rep));
@@ -1516,8 +1524,6 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, /* must be last register set: */ reg_write(priv, REG_TBG_CNTRL_0, 0);
- priv->tmds_clock = adjusted_mode->clock; - /* CEA-861B section 6 says that: * CEA version 1 (CEA-861) has no support for infoframes. * CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
On Tue, Jul 31, 2018 at 10:26:55AM +0100, Russell King wrote:
TDA998x has no support for pixel repeated modes, and the code notes this as a "TODO" item. The implementation appears to be relatively simple, so lets add it.
We need to calculate the serializer clock divisor based on the TMDS clock rate, set the repeat control, and set the serializer pixel repeat count. Since the audio code needs the actual TMDS clock, record that.
Note that this is an experimental patch - I haven't yet been able to test it beyond "does it still work with non-doubled modes". They tend to be low resolution modes, because the doubling is needed to maintain a minimum clock rate over HDMI.
The doubled modes are (eg):
VIC Name 6 720(1440)x480i@60Hz 7 720(1440)x480i@60Hz 8 720(1440)x240@60Hz 21 720(1440)x576i@50Hz 22 720(1440)x576i@50Hz 23 720(1440)x288@50Hz 24 720(1440)x288@50Hz
They are probably only found with TVs rather than monitors.
Signed-off-by: Russell King rmk+kernel@armlinux.org.uk
drivers/gpu/drm/i2c/tda998x_drv.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index a31809ce30e2..08ce28cbdec4 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -241,6 +241,7 @@ struct tda998x_priv { # define HVF_CNTRL_1_PAD(x) (((x) & 3) << 4) # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ +# define RPT_CNTRL_REPEAT(x) ((x) & 15) #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ # define I2S_FORMAT(x) (((x) & 3) << 0) #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ @@ -1342,7 +1343,7 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, u16 vwin1_line_s, vwin1_line_e; u16 vwin2_line_s, vwin2_line_e; u16 de_pix_s, de_pix_e;
- u8 reg, div, rep;
u8 reg, div, rep, sel_clk;
/*
- Internally TDA998x is using ITU-R BT.656 style sync but
@@ -1405,7 +1406,16 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, (mode->vsync_end - mode->vsync_start)/2; }
- tmds_clock = mode->clock;
/*
* Select pixel repeat depending on the double-clock flag* (which means we have to repeat each pixel once.)*/rep = mode->flags & DRM_MODE_FLAG_DBLCLK ? 1 : 0;
sel_clk = SEL_CLK_ENA_SC_CLK | SEL_CLK_SEL_CLK1 |
SEL_CLK_SEL_VRF_CLK(rep ? 2 : 0);/* the TMDS clock is scaled up by the pixel repeat */
tmds_clock = mode->clock * (1 + rep);
/*
- The divisor is power-of-2. The TDA9983B datasheet gives
@@ -1421,6 +1431,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
mutex_lock(&priv->audio_mutex);
- priv->tmds_clock = tmds_clock;
- /* mute the audio FIFO: */ reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
@@ -1443,12 +1455,8 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, reg_write(priv, REG_SERIALIZER, 0); reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
- /* TODO enable pixel repeat for pixel rates less than 25Msamp/s */
- rep = 0;
- reg_write(priv, REG_RPT_CNTRL, 0);
- reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) |
SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
- reg_write(priv, REG_RPT_CNTRL, RPT_CNTRL_REPEAT(rep));
- reg_write(priv, REG_SEL_CLK, sel_clk); reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) | PLL_SERIAL_2_SRL_PR(rep));
@@ -1516,8 +1524,6 @@ static void tda998x_bridge_mode_set(struct drm_bridge *bridge, /* must be last register set: */ reg_write(priv, REG_TBG_CNTRL_0, 0);
- priv->tmds_clock = adjusted_mode->clock;
- /* CEA-861B section 6 says that:
- CEA version 1 (CEA-861) has no support for infoframes.
- CEA version 2 (CEA-861A) supports version 1 AVI infoframes,
-- 2.7.4
On 2018-07-31 11:23, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 09:53:57AM +0200, Peter Rosin wrote:
On 2018-07-31 09:41, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
Right, meanwhile I massaged the patches into a combo of some local patches, next-20180730 and drm-misc-next, and tested that. The conflict was trivial once I had a closer look...
And it seems to work (with atmel-hlcdc), so you can add
Tested-by: Peter Rosin peda@axentia.se
Thanks, I've added your attributation, and fixed patch 2 as you pointed out.
I have a four more tda998x patches if you're willing to test. These follow on from this set - I've tweaked patch 2 in this follow on set to cater for the removal of "_mode_" in drm_mode_connector_update_edid_property.
Sure thing. Those patches work for me too. So, feel free to add
Tested-by: Peter Rosin peda@axentia.se
However, a couple of notes:
1) I never succeeded in reading the edid, so I'm using a built-in edid.
I noticed the patch adding the nxp,calib gpio and got my hopes up, but if I just add that to the DT I get
tda998x 2-0070: found TDA19988 gpio-21 (nxp,calib): gpiod_direction_output: tried to set a GPIO tied to an IRQ as output tda9950 2-0034: TDA9950 CEC interface, hardware version 3.3
Grepping the source, that comes from gpiod_direction_output, which fails with -EIO after that message, so that can't be working...
(Side note on that gpio; the way I read the patch I should specify the gpio as GPIO_ACTIVE_HIGH, but that seems backwards? Isn't the INT pin on the tda19988 active low?)
...and if I try to evade that by commenting out the interrupt stuff from the DT, I instead get
tda998x 2-0070: found TDA19988 tda9950 2-0034: driver requires an interrupt
Are we supposed to wire the INT pin to two different pins on the CPU, or what?
Is edid reading know to work on 19988? The code suggests so, but maybe it hasn't been tested? Or maybe it regressed?
2) The lowest resolution on my monitor is 800x600, so I suppose I can't really test 4/4?
Cheers, Peter
drivers/gpu/drm/i2c/tda998x_drv.c | 106 ++++++++++++++++++-------------------- 1 file changed, 51 insertions(+), 55 deletions(-)
On Tue, Jul 31, 2018 at 12:43:34PM +0200, Peter Rosin wrote:
On 2018-07-31 11:23, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 09:53:57AM +0200, Peter Rosin wrote:
On 2018-07-31 09:41, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote:
Hi,
This is a re-posting of the series I responded to Peter Rosin's May posting, with a few bugs fixed, and the bridge registered outside of the component helper. This should allow Peter to use the driver while maintaining armada drm and tilcdc support.
No comments (other than 0-day test results) were received on the previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
Right, meanwhile I massaged the patches into a combo of some local patches, next-20180730 and drm-misc-next, and tested that. The conflict was trivial once I had a closer look...
And it seems to work (with atmel-hlcdc), so you can add
Tested-by: Peter Rosin peda@axentia.se
Thanks, I've added your attributation, and fixed patch 2 as you pointed out.
I have a four more tda998x patches if you're willing to test. These follow on from this set - I've tweaked patch 2 in this follow on set to cater for the removal of "_mode_" in drm_mode_connector_update_edid_property.
Sure thing. Those patches work for me too. So, feel free to add
Tested-by: Peter Rosin peda@axentia.se
However, a couple of notes:
- I never succeeded in reading the edid, so I'm using a built-in edid.
I noticed the patch adding the nxp,calib gpio and got my hopes up, but if I just add that to the DT I get
tda998x 2-0070: found TDA19988 gpio-21 (nxp,calib): gpiod_direction_output: tried to set a GPIO tied to an IRQ as output tda9950 2-0034: TDA9950 CEC interface, hardware version 3.3
Grepping the source, that comes from gpiod_direction_output, which fails with -EIO after that message, so that can't be working...
(Side note on that gpio; the way I read the patch I should specify the gpio as GPIO_ACTIVE_HIGH, but that seems backwards? Isn't the INT pin on the tda19988 active low?)
See below...
...and if I try to evade that by commenting out the interrupt stuff from the DT, I instead get
tda998x 2-0070: found TDA19988 tda9950 2-0034: driver requires an interrupt
Are we supposed to wire the INT pin to two different pins on the CPU, or what?
It's a problem with how some GPIO/IRQ drivers in Linux model GPIOs used as interrupts. There is a school of thought that if an IRQ is requested, then the GPIO associated with it should be requested and forced as an input. Not every GPIO/IRQ driver implements this, and the Dove driver does not. So, TDA998x with CEC works there.
That's fine and dandy when you have "sensible" hardware, but the TDA998x uses the INT pin for more than just signalling an interrupt. It is also used as a calibration mechanism for the CEC controller's free-running clock. The CEC block requires various registers set, and then the INT pin being pulled low for exactly 10ms and then raised by the CPU. It uses this pulse to count the number of cycles of the internal FRO to calculate the divider necessary to get the CEC bus clock.
The GPIO is "active high" because we drive it in "true" sense:
local_irq_disable(); gpiod_set_value(calib, 0); mdelay(10); gpiod_set_value(calib, 1); local_irq_enable();
Note that the GPIO sense is independent of the IRQ trigger sense.
Some TDA998x seem to function without calibration, but it depends on the temperature and supply voltage to the device, and the margins on other devices on the CEC bus.
The TDA998x and CEC implement what's required for CEC to work, which requires a GPIO/IRQ driver that does not subscribe to the above mentioned school of thought.
However, this should only cause the CEC part to fail, and should not be an issue for reading the EDID.
EDID reading works in two stages with the TDA998x - there is no direct access to the EEPROM in the HDMI sink. Instead, the TDA998x is requested to read a block from the HDMI sink, and the received data then appears in the TDA998x registers. The interrupt _is_ optional for the TDA998x non-CEC part, and when present will be used to deal with hotplug and EDID reading. It is required for the CEC functionality though, so if there's no interrupt present in DT, the TDA9950 CEC controller driver will fail, but the TDA998x should continue to work.
If you have tried without an interrupt, and EDID reading still doesn't work, it suggests there's an electrical problem with the DDC bus between the TDA998x and the HDMI sink.
Is edid reading know to work on 19988? The code suggests so, but maybe it hasn't been tested? Or maybe it regressed?
I've been using the TDA998x pretty much constantly and EDID reading does work. I've not had any reports that it fails on Juno nor with the TI hardware either, and I'm sure that I would've had some reports if it didnt' work there.
There is an issue I'm aware of with the Synopsis DesignWare I2C controller - attempts to read more than 16 bytes result in the I2C master hardware crapping out, but in that case you'll get:
"transfer terminated early - interrupt latency too high"
in the kernel log - the problem there is that the hardware _automatically_ performs an I2C bus stop if the TX FIFO empties during a read, which means a high interrupt latency causes only the first TX FIFO-depth of data to be received. The driver _used_ to report success with garbage in the receive buffer after the first 16 bytes, now it gracefully fails and reports the condition to the kernel log.
So, failure to read the EDID can have multiple reasons - it'll need some investigation - is it the TDA998x failing to read the EDID from the sink, or is it a failure to correctly read the EDID out of the TDA998x due to some bug in the I2C driver interacting with a large I2C receive request?
- The lowest resolution on my monitor is 800x600, so I suppose I
can't really test 4/4?
No matter, it can stay as "experimental" for the time being. However, I should probably arrange for the mode validation to reject pixel doubled modes while we don't support them.
On 2018-07-31 13:15, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 12:43:34PM +0200, Peter Rosin wrote:
On 2018-07-31 11:23, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 09:53:57AM +0200, Peter Rosin wrote:
On 2018-07-31 09:41, Russell King - ARM Linux wrote:
On Tue, Jul 31, 2018 at 07:44:24AM +0200, Peter Rosin wrote:
On 2018-07-30 18:41, Russell King - ARM Linux wrote: > Hi, > > This is a re-posting of the series I responded to Peter Rosin's May > posting, with a few bugs fixed, and the bridge registered outside of > the component helper. This should allow Peter to use the driver > while maintaining armada drm and tilcdc support. > > No comments (other than 0-day test results) were received on the > previous posting.
I of course meant to comment on this! I didn't because there was a rush before vacation, then vacation, then a heap of important stuff that had piled up. This one simply had too low priority to make a significant bleep on the radar. Sorry for the silence...
However, now that I do try to test, I get conflicts as I try to apply the patches. I'm wondering what this was based on? I've tried next-20180730 drm-misc/drm-misc-next from some minutes ago.
They're based on 4.17.
Right, meanwhile I massaged the patches into a combo of some local patches, next-20180730 and drm-misc-next, and tested that. The conflict was trivial once I had a closer look...
And it seems to work (with atmel-hlcdc), so you can add
Tested-by: Peter Rosin peda@axentia.se
Thanks, I've added your attributation, and fixed patch 2 as you pointed out.
I have a four more tda998x patches if you're willing to test. These follow on from this set - I've tweaked patch 2 in this follow on set to cater for the removal of "_mode_" in drm_mode_connector_update_edid_property.
Sure thing. Those patches work for me too. So, feel free to add
Tested-by: Peter Rosin peda@axentia.se
However, a couple of notes:
- I never succeeded in reading the edid, so I'm using a built-in edid.
I noticed the patch adding the nxp,calib gpio and got my hopes up, but if I just add that to the DT I get
tda998x 2-0070: found TDA19988 gpio-21 (nxp,calib): gpiod_direction_output: tried to set a GPIO tied to an IRQ as output tda9950 2-0034: TDA9950 CEC interface, hardware version 3.3
Grepping the source, that comes from gpiod_direction_output, which fails with -EIO after that message, so that can't be working...
(Side note on that gpio; the way I read the patch I should specify the gpio as GPIO_ACTIVE_HIGH, but that seems backwards? Isn't the INT pin on the tda19988 active low?)
See below...
...and if I try to evade that by commenting out the interrupt stuff from the DT, I instead get
tda998x 2-0070: found TDA19988 tda9950 2-0034: driver requires an interrupt
Are we supposed to wire the INT pin to two different pins on the CPU, or what?
It's a problem with how some GPIO/IRQ drivers in Linux model GPIOs used as interrupts. There is a school of thought that if an IRQ is requested, then the GPIO associated with it should be requested and forced as an input. Not every GPIO/IRQ driver implements this, and the Dove driver does not. So, TDA998x with CEC works there.
I guess I might have to live without CEC, which BTW is not the end of the world...
That's fine and dandy when you have "sensible" hardware, but the TDA998x uses the INT pin for more than just signalling an interrupt. It is also used as a calibration mechanism for the CEC controller's free-running clock. The CEC block requires various registers set, and then the INT pin being pulled low for exactly 10ms and then raised by the CPU. It uses this pulse to count the number of cycles of the internal FRO to calculate the divider necessary to get the CEC bus clock.
The GPIO is "active high" because we drive it in "true" sense:
local_irq_disable(); gpiod_set_value(calib, 0); mdelay(10); gpiod_set_value(calib, 1); local_irq_enable();Note that the GPIO sense is independent of the IRQ trigger sense.
Yes, I noticed this, but just mentioned that I thought it backwards. You activate the special state by setting the pin low, which to me make it a prime candidate for using active-low (especially when the INT function of the pin is active-low, but that isn't really related). But of course, active-low or active-high ultimately depends on *what* it is that is active, so it will always be a matter of definition. Anyway, the ship has sailed...
Some TDA998x seem to function without calibration, but it depends on the temperature and supply voltage to the device, and the margins on other devices on the CEC bus.
The TDA998x and CEC implement what's required for CEC to work, which requires a GPIO/IRQ driver that does not subscribe to the above mentioned school of thought.
However, this should only cause the CEC part to fail, and should not be an issue for reading the EDID.
EDID reading works in two stages with the TDA998x - there is no direct access to the EEPROM in the HDMI sink. Instead, the TDA998x is requested to read a block from the HDMI sink, and the received data then appears in the TDA998x registers. The interrupt _is_ optional for the TDA998x non-CEC part, and when present will be used to deal with hotplug and EDID reading. It is required for the CEC functionality though, so if there's no interrupt present in DT, the TDA9950 CEC controller driver will fail, but the TDA998x should continue to work.
If you have tried without an interrupt, and EDID reading still doesn't work, it suggests there's an electrical problem with the DDC bus between the TDA998x and the HDMI sink.
Interrupt or not does not seem to matter.
And yes, my plan is to try to hook up a oscilloscope to the DDC bus, but it's a bit difficult since the HW designer didn't add any test points and covered the interesting part of the board with the CPU module. Sigh. And a microscope would be helpful, something which I don't have at this location. So, it might be a while before I can actually do it...
Is edid reading know to work on 19988? The code suggests so, but maybe it hasn't been tested? Or maybe it regressed?
I've been using the TDA998x pretty much constantly and EDID reading does work. I've not had any reports that it fails on Juno nor with the TI hardware either, and I'm sure that I would've had some reports if it didnt' work there.
There is an issue I'm aware of with the Synopsis DesignWare I2C controller - attempts to read more than 16 bytes result in the I2C master hardware crapping out, but in that case you'll get:
"transfer terminated early - interrupt latency too high"
in the kernel log - the problem there is that the hardware _automatically_ performs an I2C bus stop if the TX FIFO empties during a read, which means a high interrupt latency causes only the first TX FIFO-depth of data to be received. The driver _used_ to report success with garbage in the receive buffer after the first 16 bytes, now it gracefully fails and reports the condition to the kernel log.
So, failure to read the EDID can have multiple reasons - it'll need some investigation - is it the TDA998x failing to read the EDID from the sink, or is it a failure to correctly read the EDID out of the TDA998x due to some bug in the I2C driver interacting with a large I2C receive request?
I don't think it's a problem with the atmel I2C driver. IIRC, the tda998x driver issues the command a initiate the EDID read, but that times out. So it appears to be the TDA19988 that fails to read the EDID over the DDC bus? Which brings me to the double problem with the scopes mentioned above...
Cheers, Peter
- The lowest resolution on my monitor is 800x600, so I suppose I
can't really test 4/4?
No matter, it can stay as "experimental" for the time being. However, I should probably arrange for the mode validation to reject pixel doubled modes while we don't support them.
On Wed, Aug 01, 2018 at 11:01:12AM +0200, Peter Rosin wrote:
I don't think it's a problem with the atmel I2C driver. IIRC, the tda998x driver issues the command a initiate the EDID read, but that times out. So it appears to be the TDA19988 that fails to read the EDID over the DDC bus? Which brings me to the double problem with the scopes mentioned above...
It sounds like it.
It may be helpful to know that there are HDMI pass-through boards available that give access to all the HDMI signals:
https://elabbay.myshopify.com/collections/camera https://elabbay.myshopify.com/collections/camera/products/hdmi-af-af-v1a-hdm...
I've never bought from them, so please don't take this as a recommendation - the fact that there seems to be no company details on their site doesn't seem good, and as the whois for elabbay.com is obscured also doesn't give me any confidence to buy from them.
On 2018-08-01 11:35, Russell King - ARM Linux wrote:
On Wed, Aug 01, 2018 at 11:01:12AM +0200, Peter Rosin wrote:
I don't think it's a problem with the atmel I2C driver. IIRC, the tda998x driver issues the command a initiate the EDID read, but that times out. So it appears to be the TDA19988 that fails to read the EDID over the DDC bus? Which brings me to the double problem with the scopes mentioned above...
It sounds like it.
It may be helpful to know that there are HDMI pass-through boards available that give access to all the HDMI signals:
https://elabbay.myshopify.com/collections/camera https://elabbay.myshopify.com/collections/camera/products/hdmi-af-af-v1a-hdm...
I've never bought from them, so please don't take this as a recommendation - the fact that there seems to be no company details on their site doesn't seem good, and as the whois for elabbay.com is obscured also doesn't give me any confidence to buy from them.
I still will not be able to inspect the DDC bus between the TDA19988 and the buffer circuit (IP4786), but the gadget seems useful enough and it's not a shitload of money. We'll see how long it takes for it to get here...
Thanks for the pointer! Maybe :-)
Cheers, Peter
On 2018-08-02 08:06, Peter Rosin wrote:
On 2018-08-01 11:35, Russell King - ARM Linux wrote:
On Wed, Aug 01, 2018 at 11:01:12AM +0200, Peter Rosin wrote:
I don't think it's a problem with the atmel I2C driver. IIRC, the tda998x driver issues the command a initiate the EDID read, but that times out. So it appears to be the TDA19988 that fails to read the EDID over the DDC bus? Which brings me to the double problem with the scopes mentioned above...
It sounds like it.
It may be helpful to know that there are HDMI pass-through boards available that give access to all the HDMI signals:
https://elabbay.myshopify.com/collections/camera https://elabbay.myshopify.com/collections/camera/products/hdmi-af-af-v1a-hdm...
I've never bought from them, so please don't take this as a recommendation - the fact that there seems to be no company details on their site doesn't seem good, and as the whois for elabbay.com is obscured also doesn't give me any confidence to buy from them.
I still will not be able to inspect the DDC bus between the TDA19988 and the buffer circuit (IP4786), but the gadget seems useful enough and it's not a shitload of money. We'll see how long it takes for it to get here...
Thanks for the pointer! Maybe :-)
I got the pass-through board a while back, and that board works as expected and there was no problem with ordering etc. What I could see with that was that the TDA19988 was able to initiate a start condition (SDA -> low) but then nothing more happened.
Then last week, someone noticed that even though the TDA19988 is driven by 1.8V, it still needs the high signals of the DDC bus to be above 3V, which was unexpected and not catered for by the design. Changing VCC(SYS) of the buffer circuit in place (IP4786, pin 27) to 3.3V fixed the issue and EDID reading works, and this was confirmed earlier today.
So, the problem was that the TDA19988 only ever saw "low" DDC signals, and probably aborted when the bus appeared busy. Or something.
If anyone cares...
Cheers, Peter
On Mon, Nov 12, 2018 at 04:50:37PM +0000, Peter Rosin wrote:
On 2018-08-02 08:06, Peter Rosin wrote:
On 2018-08-01 11:35, Russell King - ARM Linux wrote:
On Wed, Aug 01, 2018 at 11:01:12AM +0200, Peter Rosin wrote:
I don't think it's a problem with the atmel I2C driver. IIRC, the tda998x driver issues the command a initiate the EDID read, but that times out. So it appears to be the TDA19988 that fails to read the EDID over the DDC bus? Which brings me to the double problem with the scopes mentioned above...
It sounds like it.
It may be helpful to know that there are HDMI pass-through boards available that give access to all the HDMI signals:
https://elabbay.myshopify.com/collections/camera https://elabbay.myshopify.com/collections/camera/products/hdmi-af-af-v1a-hdm...
I've never bought from them, so please don't take this as a recommendation - the fact that there seems to be no company details on their site doesn't seem good, and as the whois for elabbay.com is obscured also doesn't give me any confidence to buy from them.
I still will not be able to inspect the DDC bus between the TDA19988 and the buffer circuit (IP4786), but the gadget seems useful enough and it's not a shitload of money. We'll see how long it takes for it to get here...
Thanks for the pointer! Maybe :-)
I got the pass-through board a while back, and that board works as expected and there was no problem with ordering etc. What I could see with that was that the TDA19988 was able to initiate a start condition (SDA -> low) but then nothing more happened.
Then last week, someone noticed that even though the TDA19988 is driven by 1.8V, it still needs the high signals of the DDC bus to be above 3V, which was unexpected and not catered for by the design. Changing VCC(SYS) of the buffer circuit in place (IP4786, pin 27) to 3.3V fixed the issue and EDID reading works, and this was confirmed earlier today.
So, the problem was that the TDA19988 only ever saw "low" DDC signals, and probably aborted when the bus appeared busy. Or something.
Thanks for following up on this issue.
Normally, if a master device sees that the I2C clock is being held low, it assumes that a slave is "clock stretching" and it will wait until it is raised. I wonder if that's what is causing this issue here.
dri-devel@lists.freedesktop.org
-
Andrzej Hajda -
Daniel Vetter -
Peter Rosin -
Russell King -
Russell King - ARM Linux -
Sean Paul