[PATCH 2/4] drm/exynos: hdmi: register hdmiphy driver from common drm hdmi
Sean Paul
seanpaul at chromium.org
Mon Apr 29 09:58:32 PDT 2013
On Mon, Apr 29, 2013 at 10:50 AM, Rahul Sharma <rahul.sharma at samsung.com> wrote:
> hdmiphy hardware block is a physically separate device. Hdmiphy driver
> is glued inside hdmi driver and acessed through hdmi callbacks. With
> increasing diversities in the hdmiphy mapping and configurations, hdmi
> driver is expanding with un-related changes.
>
> This patch registers hdmiphy as a independent driver, having own set
> of requried callbacks which are called by common drm hdmi, parallel to
> hdmi and mixer driver.
>
> Signed-off-by: Rahul Sharma <rahul.sharma at samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_hdmi.c | 61 +++++++++++++++++++++++++++---
> drivers/gpu/drm/exynos/exynos_drm_hdmi.h | 17 +++++++++
> drivers/gpu/drm/exynos/exynos_hdmi.c | 26 ++-----------
> drivers/gpu/drm/exynos/exynos_hdmi.h | 1 -
> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 13 ++++++-
> 5 files changed, 87 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> index 7ab5f9f..25fe012 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.c
> @@ -32,12 +32,14 @@
> /* platform device pointer for common drm hdmi device. */
> static struct platform_device *exynos_drm_hdmi_pdev;
>
> -/* Common hdmi subdrv needs to access the hdmi and mixer though context.
> -* These should be initialied by the repective drivers */
> +/* Common hdmi subdrv needs to access the hdmi, hdmiphy and mixer though
> +* context. These should be initialied by the repective drivers */
Doesn't conform with Documentation/CodingStyle &
s/initialied/initialized/ & s/repective/respective/
> +static struct exynos_drm_hdmi_context *hdmiphy_ctx;
> static struct exynos_drm_hdmi_context *hdmi_ctx;
> static struct exynos_drm_hdmi_context *mixer_ctx;
>
> /* these callback points shoud be set by specific drivers. */
> +static struct exynos_hdmiphy_ops *hdmiphy_ops;
> static struct exynos_hdmi_ops *hdmi_ops;
> static struct exynos_mixer_ops *mixer_ops;
>
> @@ -45,6 +47,7 @@ struct platform_driver exynos_drm_common_hdmi_driver;
>
> struct drm_hdmi_context {
> struct exynos_drm_subdrv subdrv;
> + struct exynos_drm_hdmi_context *hdmiphy_ctx;
> struct exynos_drm_hdmi_context *hdmi_ctx;
> struct exynos_drm_hdmi_context *mixer_ctx;
>
> @@ -59,6 +62,10 @@ int exynos_common_hdmi_register(void)
> if (exynos_drm_hdmi_pdev)
> return -EEXIST;
>
> + ret = exynos_hdmiphy_driver_register();
> + if (ret < 0)
> + goto out_hdmiphy;
This isn't consistent with your last patch. In that patch, you exposed
the driver directly through exynos_drm_hdmi.h and registered the
driver directly. Here, you expose a helper function to do it for you.
These should at least work the same way.
> +
> ret = platform_driver_register(&hdmi_driver);
> if (ret < 0)
> goto out_hdmi;
> @@ -88,6 +95,8 @@ out_common_hdmi:
> out_mixer:
> platform_driver_unregister(&hdmi_driver);
> out_hdmi:
> + exynos_hdmiphy_driver_unregister();
> +out_hdmiphy:
> return ret;
> }
>
> @@ -99,9 +108,16 @@ void exynos_common_hdmi_unregister(void)
> platform_driver_unregister(&exynos_drm_common_hdmi_driver);
> platform_driver_unregister(&mixer_driver);
> platform_driver_unregister(&hdmi_driver);
> + exynos_hdmiphy_driver_unregister();
> exynos_drm_hdmi_pdev = NULL;
> }
>
> +void exynos_hdmiphy_drv_attach(struct exynos_drm_hdmi_context *ctx)
> +{
> + if (ctx)
> + hdmiphy_ctx = ctx;
> +}
> +
I think I've said this before, but in case I haven't, here it goes. If
you didn't platform_driverize all of this stuff, and instead
encapsulated the various functionality in callbacks central to one drm
driver, you wouldn't need to do this kind of thing.
> void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx)
> {
> if (ctx)
> @@ -114,6 +130,14 @@ void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx)
> mixer_ctx = ctx;
> }
>
> +void exynos_hdmiphy_ops_register(struct exynos_hdmiphy_ops *ops)
> +{
> + DRM_DEBUG_KMS("%s\n", __FILE__);
> +
> + if (ops)
> + hdmiphy_ops = ops;
> +}
> +
> void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops)
> {
> DRM_DEBUG_KMS("%s\n", __FILE__);
> @@ -164,8 +188,8 @@ static int drm_hdmi_check_mode(struct device *dev,
> DRM_DEBUG_KMS("%s\n", __FILE__);
>
> /*
> - * Both, mixer and hdmi should be able to handle the requested mode.
> - * If any of the two fails, return mode as BAD.
> + * Mixer, hdmi and hdmiphy should be able to handle the requested mode.
> + * If any of the them fails, return mode as BAD.
> */
>
> if (mixer_ops && mixer_ops->check_mode)
> @@ -175,7 +199,13 @@ static int drm_hdmi_check_mode(struct device *dev,
> return ret;
>
> if (hdmi_ops && hdmi_ops->check_mode)
> - return hdmi_ops->check_mode(ctx->hdmi_ctx->ctx, mode);
> + ret = hdmi_ops->check_mode(ctx->hdmi_ctx->ctx, mode);
> +
> + if (ret)
> + return ret;
> +
> + if (hdmiphy_ops && hdmiphy_ops->check_mode)
> + return hdmiphy_ops->check_mode(ctx->hdmiphy_ctx->ctx, mode);
>
> return 0;
> }
> @@ -289,6 +319,9 @@ static void drm_hdmi_mode_set(struct device *subdrv_dev, void *mode)
>
> if (hdmi_ops && hdmi_ops->mode_set)
> hdmi_ops->mode_set(ctx->hdmi_ctx->ctx, mode);
> +
> + if (hdmiphy_ops && hdmiphy_ops->mode_set)
> + hdmiphy_ops->mode_set(ctx->hdmiphy_ctx->ctx, mode);
> }
>
> static void drm_hdmi_get_max_resol(struct device *subdrv_dev,
> @@ -308,6 +341,15 @@ static void drm_hdmi_commit(struct device *subdrv_dev)
>
> DRM_DEBUG_KMS("%s\n", __FILE__);
>
> + if (hdmiphy_ops && hdmiphy_ops->prepare)
> + hdmiphy_ops->prepare(ctx->hdmiphy_ctx->ctx);
> +
> + if (hdmi_ops && hdmi_ops->prepare)
> + hdmi_ops->prepare(ctx->hdmi_ctx->ctx);
> +
This is a hack. You have a stubbed out exynos_drm_encoder_prepare
function in exynos_drm_connector.c that exists entirely for this
purpose.
> + if (hdmiphy_ops && hdmiphy_ops->config_apply)
> + hdmiphy_ops->config_apply(ctx->hdmiphy_ctx->ctx);
Why name it config_apply instead of commit?
> +
> if (hdmi_ops && hdmi_ops->commit)
> hdmi_ops->commit(ctx->hdmi_ctx->ctx);
> }
> @@ -323,6 +365,9 @@ static void drm_hdmi_dpms(struct device *subdrv_dev, int mode)
>
> if (hdmi_ops && hdmi_ops->dpms)
> hdmi_ops->dpms(ctx->hdmi_ctx->ctx, mode);
> +
> + if (hdmiphy_ops && hdmiphy_ops->dpms)
> + hdmiphy_ops->dpms(ctx->hdmiphy_ctx->ctx, mode);
Shouldn't the order of this be dependent on dpms mode?
ie for DPMS_ON do hdmi->dpms then phy->dpms and for DPMS_OFF do
phy->dpms then hdmi->dpms
> }
>
> static void drm_hdmi_apply(struct device *subdrv_dev)
> @@ -428,6 +473,11 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
> return -EFAULT;
> }
>
> + if (!hdmiphy_ctx) {
> + DRM_ERROR("hdmiphy context not initialized.\n");
> + return -EFAULT;
EFAULT is the wrong error to return here. ENODEV would be more appropriate, IMO.
> + }
> +
> if (!mixer_ctx) {
> DRM_ERROR("mixer context not initialized.\n");
> return -EFAULT;
> @@ -441,6 +491,7 @@ static int hdmi_subdrv_probe(struct drm_device *drm_dev,
> }
>
> ctx->hdmi_ctx = hdmi_ctx;
> + ctx->hdmiphy_ctx = hdmiphy_ctx;
> ctx->mixer_ctx = mixer_ctx;
>
> ctx->hdmi_ctx->drm_dev = drm_dev;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> index 8861b90..8c8974f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_hdmi.h
> @@ -39,10 +39,19 @@ struct exynos_hdmi_ops {
> void (*mode_set)(void *ctx, struct drm_display_mode *mode);
> void (*get_max_resol)(void *ctx, unsigned int *width,
> unsigned int *height);
> + void (*prepare)(void *ctx);
> void (*commit)(void *ctx);
> void (*dpms)(void *ctx, int mode);
> };
>
> +struct exynos_hdmiphy_ops {
> + int (*check_mode)(void *ctx, struct drm_display_mode *mode);
> + void (*mode_set)(void *ctx, struct drm_display_mode *mode);
> + void (*prepare)(void *ctx);
> + int (*config_apply)(void *ctx);
> + void (*dpms)(void *ctx, int mode);
> +};
> +
> struct exynos_mixer_ops {
> /* manager */
> int (*iommu_on)(void *ctx, bool enable);
> @@ -63,8 +72,16 @@ struct exynos_mixer_ops {
> extern struct platform_driver hdmi_driver;
> extern struct platform_driver mixer_driver;
>
> +/*
> + * these functions registers/unregisters exynos drm hdmiphy driver.
> + */
I think this comment is kind of obvious and unneeded, but if you think
it's useful, it should only take up one line.
> +extern int exynos_hdmiphy_driver_register(void);
> +extern void exynos_hdmiphy_driver_unregister(void);
> +
> +void exynos_hdmiphy_drv_attach(struct exynos_drm_hdmi_context *ctx);
> void exynos_hdmi_drv_attach(struct exynos_drm_hdmi_context *ctx);
> void exynos_mixer_drv_attach(struct exynos_drm_hdmi_context *ctx);
> +void exynos_hdmiphy_ops_register(struct exynos_hdmiphy_ops *ops);
> void exynos_hdmi_ops_register(struct exynos_hdmi_ops *ops);
> void exynos_mixer_ops_register(struct exynos_mixer_ops *ops);
> #endif
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 6bcd7fc..520c4af 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -1856,7 +1856,7 @@ fail:
> return -ENODEV;
> }
>
> -static struct i2c_client *hdmi_ddc, *hdmi_hdmiphy;
> +static struct i2c_client *hdmi_ddc;
>
> void hdmi_attach_ddc_client(struct i2c_client *ddc)
> {
> @@ -1864,12 +1864,6 @@ void hdmi_attach_ddc_client(struct i2c_client *ddc)
> hdmi_ddc = ddc;
> }
>
> -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
> -{
> - if (hdmiphy)
> - hdmi_hdmiphy = hdmiphy;
> -}
> -
> #ifdef CONFIG_OF
> static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
> (struct device *dev)
> @@ -2027,20 +2021,11 @@ static int hdmi_probe(struct platform_device *pdev)
>
> hdata->ddc_port = hdmi_ddc;
>
> - /* hdmiphy i2c driver */
> - if (i2c_add_driver(&hdmiphy_driver)) {
> - DRM_ERROR("failed to register hdmiphy i2c driver\n");
> - ret = -ENOENT;
> - goto err_ddc;
> - }
> -
> - hdata->hdmiphy_port = hdmi_hdmiphy;
> -
> hdata->irq = gpio_to_irq(hdata->hpd_gpio);
> if (hdata->irq < 0) {
> DRM_ERROR("failed to get GPIO irq\n");
> ret = hdata->irq;
> - goto err_hdmiphy;
> + goto err_ddc;
> }
>
> hdata->hpd = gpio_get_value(hdata->hpd_gpio);
> @@ -2051,7 +2036,7 @@ static int hdmi_probe(struct platform_device *pdev)
> "hdmi", drm_hdmi_ctx);
> if (ret) {
> DRM_ERROR("failed to register hdmi interrupt\n");
> - goto err_hdmiphy;
> + goto err_ddc;
> }
>
> /* Attach HDMI Driver to common hdmi. */
> @@ -2064,8 +2049,6 @@ static int hdmi_probe(struct platform_device *pdev)
>
> return 0;
>
> -err_hdmiphy:
> - i2c_del_driver(&hdmiphy_driver);
> err_ddc:
> i2c_del_driver(&ddc_driver);
> return ret;
> @@ -2083,9 +2066,6 @@ static int hdmi_remove(struct platform_device *pdev)
>
> free_irq(hdata->irq, hdata);
>
> -
> - /* hdmiphy i2c driver */
> - i2c_del_driver(&hdmiphy_driver);
> /* DDC i2c driver */
> i2c_del_driver(&ddc_driver);
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.h b/drivers/gpu/drm/exynos/exynos_hdmi.h
> index 0ddf395..6595d7b 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.h
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.h
> @@ -15,7 +15,6 @@
> #define _EXYNOS_HDMI_H_
>
> void hdmi_attach_ddc_client(struct i2c_client *ddc);
> -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy);
>
> extern struct i2c_driver hdmiphy_driver;
This can be removed if you're using the register/unregister helpers.
> extern struct i2c_driver ddc_driver;
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmiphy.c b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> index ea49d13..eee2510 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmiphy.c
> @@ -24,8 +24,6 @@
> static int hdmiphy_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> - hdmi_attach_hdmiphy_client(client);
> -
> dev_info(&client->adapter->dev, "attached s5p_hdmiphy "
> "into i2c adapter successfully\n");
>
> @@ -67,4 +65,15 @@ struct i2c_driver hdmiphy_driver = {
> .remove = hdmiphy_remove,
> .command = NULL,
> };
> +
> +extern int exynos_hdmiphy_driver_register(void)
> +{
> + return i2c_add_driver(&hdmiphy_driver);
> +}
> +
> +extern void exynos_hdmiphy_driver_unregister(void)
> +{
> + i2c_del_driver(&hdmiphy_driver);
> +}
> +
> EXPORT_SYMBOL(hdmiphy_driver);
You don't need to export it if you're using these helpers.
> --
> 1.7.10.4
>
More information about the dri-devel
mailing list