Re: [PATCH v3 4/7] drm/i2c: adv7511: Create a MIPI DSI device

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Archit,

Thank you for the patch.

On Wednesday 09 Mar 2016 16:27:15 Archit Taneja wrote:
> In order to pass DSI specific parameters to the DSI host, we need the
> driver to create a mipi_dsi_device DSI device that attaches to the
> host.
> 
> Use of_graph helpers to get the DSI host DT node. Create a MIPI DSI
> device using this host. Finally, attach this device to the DSI host.
> 
> Populate DT parameters (number of data lanes for now) that are required
> for DSI RX to work correctly. Hardcode few other parameters (rgb,
> embedded_sync) for now.
> 
> Signed-off-by: Archit Taneja <architt@codeaurora.org>

This adds a hard dependency on CONFIG_DRM_MIPI_DSI, otherwise the kernel won't 
link. As the ADV7511 doesn't require DSI, could you make it optional with 
conditional compilation to avoid pulling in dead code ?

> ---
>  drivers/gpu/drm/i2c/adv7511.c | 130 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 123 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index 75be17c..6c2a89a 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -11,6 +11,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
> 
> @@ -19,6 +20,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_mipi_dsi.h>
> 
>  #include "adv7511.h"
> 
> @@ -56,6 +58,11 @@ struct adv7511 {
> 
>  	struct gpio_desc *gpio_pd;
> 
> +	/* ADV7533 DSI RX related params */
> +	struct device_node *host_node;
> +	struct mipi_dsi_device *dsi;
> +	u8 num_dsi_lanes;
> +
>  	enum adv7511_type type;
>  };
> 
> @@ -392,8 +399,10 @@ static void adv7511_set_link_config(struct adv7511
> *adv7511,
> 
>  static void adv7533_dsi_power_on(struct adv7511 *adv)
>  {
> -	/* set number of dsi lanes (hardcoded to 4 for now) */
> -	regmap_write(adv->regmap_cec, 0x1c, 0x4 << 4);
> +	struct mipi_dsi_device *dsi = adv->dsi;
> +
> +	/* set number of dsi lanes */
> +	regmap_write(adv->regmap_cec, 0x1c, dsi->lanes << 4);
>  	/* disable internal timing generator */
>  	regmap_write(adv->regmap_cec, 0x27, 0x0b);
>  	/* enable hdmi */
> @@ -889,6 +898,51 @@ static void adv7511_bridge_mode_set(struct drm_bridge
> *bridge, adv7511_mode_set(adv, mode, adj_mode);
>  }
> 
> +static int adv7533_attach_dsi(struct adv7511 *adv)
> +{
> +	struct device *dev = &adv->i2c_main->dev;
> +	struct mipi_dsi_host *host;
> +	struct mipi_dsi_device *dsi;
> +	int ret = 0;
> +	const struct mipi_dsi_device_info info = { .type = "adv7533",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	host = of_find_mipi_dsi_host_by_node(adv->host_node);
> +	if (!host) {
> +		dev_err(dev, "failed to find dsi host\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi)) {
> +		dev_err(dev, "failed to create dsi device\n");
> +		ret = PTR_ERR(dsi);
> +		goto err_dsi_device;
> +	}
> +
> +	adv->dsi = dsi;
> +
> +	dsi->lanes = adv->num_dsi_lanes;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to attach dsi to host\n");
> +		goto err_dsi_attach;
> +	}
> +
> +	return 0;
> +
> +err_dsi_attach:
> +	mipi_dsi_device_unregister(dsi);
> +err_dsi_device:
> +	return ret;
> +}
> +
>  static int adv7511_bridge_attach(struct drm_bridge *bridge)
>  {
>  	struct adv7511 *adv = bridge_to_adv7511(bridge);
> @@ -912,6 +966,9 @@ static int adv7511_bridge_attach(struct drm_bridge
> *bridge) &adv7511_connector_helper_funcs);
>  	drm_mode_connector_attach_encoder(&adv->connector, bridge->encoder);
> 
> +	if (adv->type == ADV7533)
> +		ret = adv7533_attach_dsi(adv);
> +
>  	return ret;
>  }
> 
> @@ -1009,6 +1066,41 @@ static int adv7511_parse_dt(struct device_node *np,
>  	return 0;
>  }
> 
> +static int adv7533_parse_dt(struct device_node *np, struct adv7511 *adv)
> +{
> +	u32 num_lanes;
> +	struct device_node *endpoint;
> +
> +	of_property_read_u32(np, "adi,dsi-lanes", &num_lanes);
> +
> +	if (num_lanes < 1 || num_lanes > 4)
> +		return -EINVAL;
> +
> +	adv->num_dsi_lanes = num_lanes;
> +
> +	endpoint = of_graph_get_next_endpoint(np, NULL);
> +	if (!endpoint) {
> +		DRM_ERROR("ADV7533 DSI input endpoint not found\n");
> +		return -ENODEV;
> +	}
> +
> +	adv->host_node = of_graph_get_remote_port_parent(endpoint);
> +	if (!adv->host_node) {
> +		DRM_ERROR("DSI host node not found\n");
> +		of_node_put(endpoint);
> +		return -ENODEV;
> +	}
> +
> +	of_node_put(endpoint);
> +	of_node_put(adv->host_node);
> +
> +	/* TODO: Check if these need to be parsed by DT or not */
> +	adv->rgb = true;
> +	adv->embedded_sync = false;
> +
> +	return 0;
> +}
> +
>  static const int edid_i2c_addr = 0x7e;
>  static const int packet_i2c_addr = 0x70;
>  static const int cec_i2c_addr = 0x78;
> @@ -1038,11 +1130,12 @@ static int adv7511_probe(struct i2c_client *i2c,
> const struct i2c_device_id *id)
> 
>  	memset(&link_config, 0, sizeof(link_config));
> 
> -	if (adv7511->type == ADV7511) {
> +	if (adv7511->type == ADV7511)
>  		ret = adv7511_parse_dt(dev->of_node, &link_config);
> -		if (ret)
> -			return ret;
> -	}
> +	else
> +		ret = adv7533_parse_dt(dev->of_node, adv7511);
> +	if (ret)
> +		return ret;
> 
>  	/*
>  	 * The power down GPIO is optional. If present, toggle it from active to
> @@ -1155,6 +1248,11 @@ static int adv7511_remove(struct i2c_client *i2c)
>  {
>  	struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
> 
> +	if (adv7511->type == ADV7533) {
> +		mipi_dsi_detach(adv7511->dsi);
> +		mipi_dsi_device_unregister(adv7511->dsi);
> +	}
> +
>  	drm_bridge_remove(&adv7511->bridge);
> 
>  	i2c_unregister_device(adv7511->i2c_cec);
> @@ -1183,6 +1281,10 @@ static const struct of_device_id adv7511_of_ids[] = {
> };
>  MODULE_DEVICE_TABLE(of, adv7511_of_ids);
> 
> +static struct mipi_dsi_driver adv7533_dsi_driver = {
> +	.driver.name = "adv7533",
> +};
> +
>  static struct i2c_driver adv7511_driver = {
>  	.driver = {
>  		.name = "adv7511",
> @@ -1193,7 +1295,21 @@ static struct i2c_driver adv7511_driver = {
>  	.remove = adv7511_remove,
>  };
> 
> -module_i2c_driver(adv7511_driver);
> +static int __init adv7511_init(void)
> +{
> +	mipi_dsi_driver_register(&adv7533_dsi_driver);
> +
> +	return i2c_add_driver(&adv7511_driver);
> +}
> +module_init(adv7511_init);
> +
> +static void __exit adv7511_exit(void)
> +{
> +	i2c_del_driver(&adv7511_driver);
> +
> +	mipi_dsi_driver_unregister(&adv7533_dsi_driver);
> +}
> +module_exit(adv7511_exit);
> 
>  MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
>  MODULE_DESCRIPTION("ADV7511 HDMI transmitter driver");

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel