Hi Archit,
Thanks a lot for the patch.
On Saturday 09 January 2016 22:20:25 Archit Taneja wrote:
> We don't want to use the old i2c slave encoder interface anymore.
I happily agree with that :-)
> Remove that and make the i2c driver create a drm_bridge entity instead.
> Converting to bridges helps because the kms drivers don't need to
> exract encoder slave ops from this driver and use it within their
> own encoder/connector ops.
>
> The driver now creates its own connector when a kms driver attaches
> itself to the bridge. Therefore, kms drivers don't need to create
> their own connectors anymore.
That, however, I don't agree with. There's nothing that guarantees that the
output of a bridge is a connector. The bridge output could be connected to the
input of another bridge. Quoting drm_bridge.c,
* A bridge is always attached to a single &drm_encoder at a time, but can be
* either connected to it directly, or through an intermediate bridge:
*
* encoder ---> bridge B ---> bridge A
Creating the connector should be done by the DRM driver, as that's the only
driver that should have knowledge of the full hardware topology.
Apart from that the patch looks good to me, please see below for a couple of
comments.
As creating the connector outside of the bridge driver will significantly
impact "[RFC] drm: rcar-du: Remove i2c slave encoder interface for hdmi
encoder" I'll postpone reviewing that patch until we reach an agreement on
this one if that's fine with you. Please let me know if there are items in the
rcar-du patch that you would like me to already look at.
> The old encoder slave ops are now used by the new bridge and connector
> entities.
>
> Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i2c/adv7511.c | 234 ++++++++++++++++++++++++++-------------
> 1 file changed, 160 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index 00416f2..3fda10f 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -16,7 +16,8 @@
> #include <drm/drmP.h>
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_edid.h>
> -#include <drm/drm_encoder_slave.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
Alphabetical order please :-)
>
> #include "adv7511.h"
>
> @@ -36,7 +37,8 @@ struct adv7511 {
> bool edid_read;
>
> wait_queue_head_t wq;
> - struct drm_encoder *encoder;
> + struct drm_bridge bridge;
> + struct drm_connector connector;
>
> bool embedded_sync;
> enum adv7511_sync_polarity vsync_polarity;
> @@ -48,11 +50,6 @@ struct adv7511 {
> struct gpio_desc *gpio_pd;
> };
>
> -static struct adv7511 *encoder_to_adv7511(struct drm_encoder *encoder)
> -{
> - return to_encoder_slave(encoder)->slave_priv;
> -}
> -
> /* ADI recommended values for proper operation. */
> static const struct reg_sequence adv7511_fixed_registers[] = {
> { 0x98, 0x03 },
> @@ -438,8 +435,8 @@ static int adv7511_irq_process(struct adv7511 *adv7511)
> regmap_write(adv7511->regmap, ADV7511_REG_INT(0), irq0);
> regmap_write(adv7511->regmap, ADV7511_REG_INT(1), irq1);
>
> - if (irq0 & ADV7511_INT0_HDP && adv7511->encoder)
> - drm_helper_hpd_irq_event(adv7511->encoder->dev);
> + if (irq0 & ADV7511_INT0_HDP && adv7511->bridge.encoder)
> + drm_helper_hpd_irq_event(adv7511->connector.dev);
>
> if (irq0 & ADV7511_INT0_EDID_READY || irq1 & ADV7511_INT1_DDC_ERROR) {
> adv7511->edid_read = true;
> @@ -555,13 +552,12 @@ static int adv7511_get_edid_block(void *data, u8 *buf,
> unsigned int block, }
>
> /* ------------------------------------------------------------------------
> - * Encoder operations
> + * ADV75xx helpers
> */
>
> -static int adv7511_get_modes(struct drm_encoder *encoder,
> +static int adv7511_get_modes(struct adv7511 *adv7511,
> struct drm_connector *connector)
> {
> - struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> struct edid *edid;
> unsigned int count;
>
> @@ -596,21 +592,9 @@ static int adv7511_get_modes(struct drm_encoder
> *encoder, return count;
> }
>
> -static void adv7511_encoder_dpms(struct drm_encoder *encoder, int mode)
> -{
> - struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> -
> - if (mode == DRM_MODE_DPMS_ON)
> - adv7511_power_on(adv7511);
> - else
> - adv7511_power_off(adv7511);
> -}
> -
> static enum drm_connector_status
> -adv7511_encoder_detect(struct drm_encoder *encoder,
> - struct drm_connector *connector)
> +adv7511_detect(struct adv7511 *adv7511, struct drm_connector *connector)
> {
> - struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> enum drm_connector_status status;
> unsigned int val;
> bool hpd;
> @@ -634,7 +618,7 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
> if (status == connector_status_connected && hpd && adv7511->powered) {
> regcache_mark_dirty(adv7511->regmap);
> adv7511_power_on(adv7511);
> - adv7511_get_modes(encoder, connector);
> + adv7511_get_modes(adv7511, connector);
> if (adv7511->status == connector_status_connected)
> status = connector_status_disconnected;
> } else {
> @@ -648,8 +632,8 @@ adv7511_encoder_detect(struct drm_encoder *encoder,
> return status;
> }
>
> -static int adv7511_encoder_mode_valid(struct drm_encoder *encoder,
> - struct drm_display_mode *mode)
> +static int adv7511_mode_valid(struct adv7511 *adv7511,
> + struct drm_display_mode *mode)
> {
> if (mode->clock > 165000)
> return MODE_CLOCK_HIGH;
> @@ -657,11 +641,10 @@ static int adv7511_encoder_mode_valid(struct
> drm_encoder *encoder, return MODE_OK;
> }
>
> -static void adv7511_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adj_mode)
> +static void adv7511_mode_set(struct adv7511 *adv7511,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adj_mode)
> {
> - struct adv7511 *adv7511 = encoder_to_adv7511(encoder);
> unsigned int low_refresh_rate;
> unsigned int hsync_polarity = 0;
> unsigned int vsync_polarity = 0;
> @@ -752,12 +735,132 @@ static void adv7511_encoder_mode_set(struct
> drm_encoder *encoder, adv7511->f_tmds = mode->clock;
> }
>
> -static struct drm_encoder_slave_funcs adv7511_encoder_funcs = {
> - .dpms = adv7511_encoder_dpms,
> - .mode_valid = adv7511_encoder_mode_valid,
> - .mode_set = adv7511_encoder_mode_set,
> - .detect = adv7511_encoder_detect,
> - .get_modes = adv7511_get_modes,
> +/* Connector funcs */
> +static struct adv7511 *connector_to_adv7511(struct drm_connector
> *connector) +{
> + return container_of(connector, struct adv7511, connector);
> +}
> +
> +static int adv7511_connector_get_modes(struct drm_connector *connector)
> +{
> + struct adv7511 *adv = connector_to_adv7511(connector);
> +
> + return adv7511_get_modes(adv, connector);
> +}
> +
> +static struct drm_encoder *
> +adv7511_connector_best_encoder(struct drm_connector *connector)
> +{
> + struct adv7511 *adv = connector_to_adv7511(connector);
> +
> + return adv->bridge.encoder;
> +}
> +
> +static enum drm_mode_status
> +adv7511_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + struct adv7511 *adv = connector_to_adv7511(connector);
> +
> + return adv7511_mode_valid(adv, mode);
> +}
> +
> +static struct drm_connector_helper_funcs adv7511_connector_helper_funcs = {
> + .get_modes = adv7511_connector_get_modes,
> + .best_encoder = adv7511_connector_best_encoder,
> + .mode_valid = adv7511_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +adv7511_connector_detect(struct drm_connector *connector, bool force)
> +{
> + struct adv7511 *adv = connector_to_adv7511(connector);
> +
> + return adv7511_detect(adv, connector);
> +}
> +
> +static struct drm_connector_funcs adv7511_connector_funcs = {
> + .dpms = drm_atomic_helper_connector_dpms,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .detect = adv7511_connector_detect,
> + .destroy = drm_connector_cleanup,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +/* Bridge funcs */
> +static struct adv7511 *bridge_to_adv7511(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct adv7511, bridge);
> +}
> +
> +static void adv7511_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct adv7511 *adv = bridge_to_adv7511(bridge);
> +
> + adv7511_power_on(adv);
> +}
> +
> +static void adv7511_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + struct adv7511 *adv = bridge_to_adv7511(bridge);
> +
> + adv7511_power_off(adv);
> +}
> +
> +static void adv7511_bridge_enable(struct drm_bridge *bridge)
> +{
> +}
> +
> +static void adv7511_bridge_disable(struct drm_bridge *bridge)
> +{
> +}
You could define a single function called adv7511_bridge_noop(), or, better,
make the callbacks optionals in drm_bridge.c.
By the way, what's the reason you use the pre_enable/post_disable callbacks
instead of enable/disable ?
> +static void adv7511_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adj_mode)
> +{
> + struct adv7511 *adv = bridge_to_adv7511(bridge);
> +
> + adv7511_mode_set(adv, mode, adj_mode);
> +}
> +
> +static int adv7511_bridge_attach(struct drm_bridge *bridge)
> +{
> + struct adv7511 *adv = bridge_to_adv7511(bridge);
> + int ret;
> +
> + if (!bridge->encoder) {
> + DRM_ERROR("Parent encoder object not found");
> + return -ENODEV;
> + }
> +
> + adv->connector.polled = DRM_CONNECTOR_POLL_HPD;
> +
> + ret = drm_connector_init(bridge->dev, &adv->connector,
> + &adv7511_connector_funcs, DRM_MODE_CONNECTOR_HDMIA);
> + if (ret) {
> + DRM_ERROR("Failed to initialize connector with drm\n");
> + return ret;
> + }
> + drm_connector_helper_add(&adv->connector,
> + &adv7511_connector_helper_funcs);
> + drm_connector_register(&adv->connector);
> + drm_mode_connector_attach_encoder(&adv->connector, bridge->encoder);
> +
> + drm_helper_hpd_irq_event(adv->connector.dev);
> +
> + return ret;
> +}
> +
> +static struct drm_bridge_funcs adv7511_bridge_funcs = {
> + .pre_enable = adv7511_bridge_pre_enable,
> + .enable = adv7511_bridge_enable,
> + .disable = adv7511_bridge_disable,
> + .post_disable = adv7511_bridge_post_disable,
> + .mode_set = adv7511_bridge_mode_set,
> + .attach = adv7511_bridge_attach,
> };
>
> /*
> ---------------------------------------------------------------------------
> -- @@ -934,6 +1037,15 @@ static int adv7511_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
>
> adv7511_set_link_config(adv7511, &link_config);
>
> + adv7511->bridge.funcs = &adv7511_bridge_funcs;
> + adv7511->bridge.of_node = dev->of_node;
> +
> + ret = drm_bridge_add(&adv7511->bridge);
> + if (ret) {
> + dev_err(dev, "failed to add adv7511 bridge\n");
> + goto err_i2c_unregister_device;
> + }
> +
> return 0;
>
> err_i2c_unregister_device:
> @@ -946,6 +1058,8 @@ static int adv7511_remove(struct i2c_client *i2c)
> {
> struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
>
> + drm_bridge_remove(&adv7511->bridge);
> +
> i2c_unregister_device(adv7511->i2c_edid);
>
> kfree(adv7511->edid);
> @@ -953,20 +1067,6 @@ static int adv7511_remove(struct i2c_client *i2c)
> return 0;
> }
>
> -static int adv7511_encoder_init(struct i2c_client *i2c, struct drm_device
> *dev, - struct drm_encoder_slave *encoder)
> -{
> -
> - struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> -
> - encoder->slave_priv = adv7511;
> - encoder->slave_funcs = &adv7511_encoder_funcs;
> -
> - adv7511->encoder = &encoder->base;
> -
> - return 0;
> -}
> -
> static const struct i2c_device_id adv7511_i2c_ids[] = {
> { "adv7511", 0 },
> { "adv7511w", 0 },
> @@ -983,31 +1083,17 @@ static const struct of_device_id adv7511_of_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, adv7511_of_ids);
>
> -static struct drm_i2c_encoder_driver adv7511_driver = {
> - .i2c_driver = {
> - .driver = {
> - .name = "adv7511",
> - .of_match_table = adv7511_of_ids,
> - },
> - .id_table = adv7511_i2c_ids,
> - .probe = adv7511_probe,
> - .remove = adv7511_remove,
> +static struct i2c_driver adv7511_driver = {
> + .driver = {
> + .name = "adv7511",
> + .of_match_table = adv7511_of_ids,
> },
> -
> - .encoder_init = adv7511_encoder_init,
> + .id_table = adv7511_i2c_ids,
> + .probe = adv7511_probe,
> + .remove = adv7511_remove,
> };
>
> -static int __init adv7511_init(void)
> -{
> - return drm_i2c_encoder_register(THIS_MODULE, &adv7511_driver);
> -}
> -module_init(adv7511_init);
> -
> -static void __exit adv7511_exit(void)
> -{
> - drm_i2c_encoder_unregister(&adv7511_driver);
> -}
> -module_exit(adv7511_exit);
> +module_i2c_driver(adv7511_driver);
>
> MODULE_AUTHOR("Lars-Peter Clausen <lars@xxxxxxxxxx>");
> MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver");
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
