[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree

[Tomasz Figa]

Hi Sean, Daniel,

Please see my comments inline.

On Tuesday 29 of October 2013 12:12:59 Sean Paul wrote:
> From: Daniel Kurtz <djkurtz at chromium.org>
> 
> The i2c client was previously being passed into the hdmi driver via a
> dedicated i2c driver, and then a global variable. This patch removes all
> of that and just uses the device tree to get the i2c_client. This patch
> also properly references the client so we don't lose it before we're
> done with it.
> 
> Signed-off-by: Daniel Kurtz <djkurtz at chromium.org>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
> 
> Changes in v2:
> 	- Change include to linux/i2c.h instead of linux/of_i2c.h
> Changes in v3: None
> 
>  drivers/gpu/drm/exynos/Makefile         |  1 -
>  drivers/gpu/drm/exynos/exynos_ddc.c     | 63 --------------------------------
>  drivers/gpu/drm/exynos/exynos_hdmi.c    | 59 ++++++++++++++----------------
>  drivers/gpu/drm/exynos/exynos_hdmi.h    | 23 ------------
>  drivers/gpu/drm/exynos/exynos_hdmiphy.c | 65 ---------------------------------
>  5 files changed, 27 insertions(+), 184 deletions(-)
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_ddc.c
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmi.h
>  delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmiphy.c

I hope this patch does not alter the existing Exynos HDMI bindings.
This would have to be reflected in respective documentation otherwise
and of course done in a backwards compatible way.

Well, let's see...

> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index d35ab2a..e106309 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[snip]
> @@ -1957,21 +1943,30 @@ static int hdmi_probe(struct platform_device *pdev)
>  	}
>  
>  	/* DDC i2c driver */
> -	if (i2c_add_driver(&ddc_driver)) {
> -		DRM_ERROR("failed to register ddc i2c driver\n");
> -		return -ENOENT;
> +	ddc_node = of_find_node_by_name(NULL, "hdmiddc");

This is wrong. You shall not reference a device tree node by its name,
except some very specific well-defined cases, such as cpus or memory
nodes.

A solution closest to yours, but correct, would be to use the same match
table as in the I2C driver you are removing and call
of_find_matching_node().

> +	if (!ddc_node) {
> +		DRM_ERROR("Failed to find ddc node in device tree\n");
> +		return -ENODEV;
> +	}
> +	hdata->ddc_port = of_find_i2c_device_by_node(ddc_node);
> +	if (!hdata->ddc_port) {
> +		DRM_ERROR("Failed to get ddc i2c client by node\n");
> +		return -ENODEV;
>  	}

In general, this looks like a better approach, without using a fake
driver. However you should check how this will play together with runtime
PM of the I2C adapter, as I'm not sure about it.

Both this comment and the comment above apply as well to the hdmiphy
handling.

Best regards,
Tomasz