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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
