[PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
From: Sean Paul <hidden>
Date: 2018-08-08 19:09:30
Also in:
dri-devel
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 <redacted>
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.
quoted hunk ↗ jump to hunk
--- 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.
quoted hunk ↗ jump to hunk
+} + +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/
quoted hunk ↗ jump to hunk
+/* 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.
quoted hunk ↗ jump to hunk
}; -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 at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-- Sean Paul, Software Engineer, Google / Chromium OS