From: Joonyoung Shim <jy0922.shim@samsung.com>
To: Rahul Sharma <rahul.sharma@samsung.com>
Cc: s.shirish@samsung.com, l.krishna@samsung.com, joshi@samsung.com,
dri-devel@lists.freedesktop.org, kyungmin.park@samsung.com,
fahad.k@samsung.com, prashanth.g@samsung.com
Subject: Re: [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver
Date: Thu, 13 Sep 2012 10:23:25 +0900 [thread overview]
Message-ID: <5051358D.3040402@samsung.com> (raw)
In-Reply-To: <1347451727-13386-3-git-send-email-rahul.sharma@samsung.com>
Hi, Rahul.
Overall, i think this patch causes messy codes.
On 09/12/2012 09:08 PM, Rahul Sharma wrote:
> Added support for exynos5 to hdmi driver. Resource init
> is splitted for exynos5 and exynos4. Exynos5 hdmi driver
> is dt based while exynos4 hdmi driver is not.
>
> Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> Signed-off-by: Fahad Kunnathadi <fahad.k@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_hdmi.c | 300 ++++++++++++++++++++++++++++++----
> 1 files changed, 269 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index a6aea6f..5236256 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -32,6 +32,9 @@
> #include <linux/pm_runtime.h>
> #include <linux/clk.h>
> #include <linux/regulator/consumer.h>
> +#include <linux/io.h>
> +#include <linux/of_gpio.h>
> +#include <plat/gpio-cfg.h>
>
> #include <drm/exynos_drm.h>
>
> @@ -61,11 +64,13 @@ struct hdmi_context {
> bool powered;
> bool is_v13;
> bool dvi_mode;
> + bool is_soc_exynos5;
> struct mutex hdmi_mutex;
>
> void __iomem *regs;
> - unsigned int external_irq;
> - unsigned int internal_irq;
> + int external_irq;
> + int internal_irq;
> + int hpd_gpio;
>
> struct i2c_client *ddc_port;
> struct i2c_client *hdmiphy_port;
> @@ -953,6 +958,23 @@ static inline void hdmi_reg_writemask(struct hdmi_context *hdata,
> writel(value, hdata->regs + reg_id);
> }
>
> +static void hdmi_cfg_hpd(struct hdmi_context *hdata, bool internal)
> +{
> + if (!internal) {
> + s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(0xf));
> + s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_DOWN);
> + } else {
> + s3c_gpio_cfgpin(hdata->hpd_gpio, S3C_GPIO_SFN(3));
> + s3c_gpio_setpull(hdata->hpd_gpio, S3C_GPIO_PULL_NONE);
> + }
> +}
> +
Don't use SoC specific functions in the driver.
> +static int hdmi_get_hpd(struct hdmi_context *hdata)
> +{
> + int gpio_stat = gpio_get_value(hdata->hpd_gpio);
> + return gpio_stat;
> +}
> +
Actually, above two functions should come from platform data, but these
will remove soon.
> static void hdmi_v13_regs_dump(struct hdmi_context *hdata, char *prefix)
> {
> #define DUMPREG(reg_id) \
> @@ -2026,6 +2048,9 @@ static void hdmi_poweron(struct hdmi_context *hdata)
>
> if (hdata->cfg_hpd)
> hdata->cfg_hpd(true);
> + else
> + hdmi_cfg_hpd(hdata, true);
> +
> mutex_unlock(&hdata->hdmi_mutex);
>
> pm_runtime_get_sync(hdata->dev);
> @@ -2063,6 +2088,8 @@ static void hdmi_poweroff(struct hdmi_context *hdata)
> mutex_lock(&hdata->hdmi_mutex);
> if (hdata->cfg_hpd)
> hdata->cfg_hpd(false);
> + else
> + hdmi_cfg_hpd(hdata, false);
>
> hdata->powered = false;
>
> @@ -2110,17 +2137,16 @@ static irqreturn_t hdmi_external_irq_thread(int irq, void *arg)
> struct exynos_drm_hdmi_context *ctx = arg;
> struct hdmi_context *hdata = ctx->ctx;
>
> - if (!hdata->get_hpd)
> - goto out;
> -
> mutex_lock(&hdata->hdmi_mutex);
> - hdata->hpd = hdata->get_hpd();
> + if (hdata->get_hpd)
> + hdata->hpd = hdata->get_hpd();
> + else
> + hdata->hpd = hdmi_get_hpd(hdata);
> mutex_unlock(&hdata->hdmi_mutex);
>
> if (ctx->drm_dev)
> drm_helper_hpd_irq_event(ctx->drm_dev);
>
> -out:
> return IRQ_HANDLED;
> }
>
> @@ -2203,23 +2229,25 @@ static int __devinit hdmi_resources_init(struct hdmi_context *hdata)
>
> clk_set_parent(res->sclk_hdmi, res->sclk_pixel);
>
> - res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
> - sizeof(res->regul_bulk[0]), GFP_KERNEL);
> - if (!res->regul_bulk) {
> - DRM_ERROR("failed to get memory for regulators\n");
> - goto fail;
> - }
> - for (i = 0; i < ARRAY_SIZE(supply); ++i) {
> - res->regul_bulk[i].supply = supply[i];
> - res->regul_bulk[i].consumer = NULL;
> - }
> - ret = regulator_bulk_get(dev, ARRAY_SIZE(supply), res->regul_bulk);
> - if (ret) {
> - DRM_ERROR("failed to get regulators\n");
> - goto fail;
> + if (!hdata->is_soc_exynos5) {
> + res->regul_bulk = kzalloc(ARRAY_SIZE(supply) *
> + sizeof(res->regul_bulk[0]), GFP_KERNEL);
> + if (!res->regul_bulk) {
> + DRM_ERROR("failed to get memory for regulators\n");
> + goto fail;
> + }
> + for (i = 0; i < ARRAY_SIZE(supply); ++i) {
> + res->regul_bulk[i].supply = supply[i];
> + res->regul_bulk[i].consumer = NULL;
> + }
> + ret = regulator_bulk_get(dev, ARRAY_SIZE(supply),
> + res->regul_bulk);
> + if (ret) {
> + DRM_ERROR("failed to get regulators\n");
> + goto fail;
> + }
> + res->regul_count = ARRAY_SIZE(supply);
> }
> - res->regul_count = ARRAY_SIZE(supply);
> -
> return 0;
> fail:
> DRM_ERROR("HDMI resource init - failed\n");
> @@ -2262,7 +2290,8 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
> hdmi_hdmiphy = hdmiphy;
> }
>
> -static int __devinit hdmi_probe(struct platform_device *pdev)
> +static int __devinit hdmi_resources_init_exynos4(
> + struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> @@ -2271,7 +2300,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
> struct resource *res;
> int ret;
>
> - DRM_DEBUG_KMS("[%d]\n", __LINE__);
> + DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
>
> pdata = pdev->dev.platform_data;
> if (!pdata) {
> @@ -2304,14 +2333,21 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
> hdata->cfg_hpd = pdata->cfg_hpd;
> hdata->get_hpd = pdata->get_hpd;
> hdata->dev = dev;
> + hdata->is_soc_exynos5 = false;
>
> ret = hdmi_resources_init(hdata);
> if (ret) {
> ret = -EINVAL;
> + DRM_ERROR("hdmi_resources_init failed\n");
> goto err_data;
> }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + DRM_ERROR("failed to find registers\n");
> + ret = -ENOENT;
> + goto err_resource;
> + }
>
> hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
> if (!hdata->regs) {
> @@ -2340,7 +2376,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
>
> hdata->external_irq = platform_get_irq_byname(pdev, "external_irq");
> if (hdata->external_irq < 0) {
> - DRM_ERROR("failed to get platform irq\n");
> + DRM_ERROR("failed to get platform external irq\n");
> ret = hdata->external_irq;
> goto err_hdmiphy;
> }
> @@ -2357,7 +2393,7 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
> IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> "hdmi_external", drm_hdmi_ctx);
> if (ret) {
> - DRM_ERROR("failed to register hdmi internal interrupt\n");
> + DRM_ERROR("failed to register hdmi external interrupt\n");
> goto err_hdmiphy;
> }
>
> @@ -2372,10 +2408,157 @@ static int __devinit hdmi_probe(struct platform_device *pdev)
> goto err_free_irq;
> }
>
> - /* register specific callbacks to common hdmi. */
> - exynos_hdmi_ops_register(&hdmi_ops);
> + return 0;
>
> - pm_runtime_enable(dev);
> +err_free_irq:
> + free_irq(hdata->external_irq, drm_hdmi_ctx);
> +err_hdmiphy:
> + i2c_del_driver(&hdmiphy_driver);
> +err_ddc:
> + i2c_del_driver(&ddc_driver);
> +err_resource:
> + hdmi_resources_cleanup(hdata);
> +err_data:
> + return ret;
> +}
> +
> +static int __devinit hdmi_resources_init_exynos5(
> + struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> + struct hdmi_context *hdata;
> + struct resource *res;
> + unsigned int value;
> + int ret;
> + enum of_gpio_flags flags;
> +
> + DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
> +
> + drm_hdmi_ctx = devm_kzalloc(&pdev->dev, sizeof(*drm_hdmi_ctx),
> + GFP_KERNEL);
> + if (!drm_hdmi_ctx) {
> + DRM_ERROR("failed to allocate common hdmi context.\n");
> + return -ENOMEM;
> + }
> +
> + hdata = devm_kzalloc(&pdev->dev, sizeof(struct hdmi_context),
> + GFP_KERNEL);
> + if (!hdata) {
> + DRM_ERROR("out of memory\n");
> + return -ENOMEM;
> + }
> +
> + mutex_init(&hdata->hdmi_mutex);
> +
> + drm_hdmi_ctx->ctx = (void *)hdata;
> + hdata->parent_ctx = (void *)drm_hdmi_ctx;
> +
> + platform_set_drvdata(pdev, drm_hdmi_ctx);
> +
> + if (!of_property_read_u32(pdev->dev.of_node,
> + "v13_support", &value)) {
> + hdata->is_v13 = (value == 0) ? false : true;
> + } else {
> + DRM_ERROR("no hdmi version property found\n");
> + ret = -EINVAL;
> + goto err_data;
> + }
> +
> + if (!of_find_property(pdev->dev.of_node,
> + "hpd-gpio", &value)){
> + DRM_ERROR("no hpd gpio property found\n");
> + ret = -EINVAL;
> + goto err_data;
> + }
> +
> + hdata->hpd_gpio = of_get_named_gpio_flags(pdev->dev.of_node,
> + "hpd-gpio", 0, &flags);
> +
> + if (!gpio_is_valid(hdata->hpd_gpio)) {
> + DRM_ERROR("failed to get hpd gpio.");
> + ret = -EINVAL;
> + goto err_data;
> + }
> +
> + hdata->cfg_hpd = NULL;
> + hdata->get_hpd = NULL;
> + hdata->dev = dev;
> + hdata->is_soc_exynos5 = true;
> +
> + ret = hdmi_resources_init(hdata);
> + if (ret) {
> + ret = -EINVAL;
> + DRM_ERROR("failed hdmi_resources_init.");
> + goto err_data;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + DRM_ERROR("failed to find registers\n");
> + ret = -ENOENT;
> + goto err_resource;
> + }
> +
> + hdata->regs = devm_request_and_ioremap(&pdev->dev, res);
> + if (!hdata->regs) {
> + DRM_ERROR("failed to map registers\n");
> + ret = -ENXIO;
> + goto err_resource;
> + }
> +
> + /* DDC i2c driver */
> + if (i2c_add_driver(&ddc_driver)) {
> + DRM_ERROR("failed to register ddc i2c driver\n");
> + ret = -ENOENT;
> + goto err_resource;
> + }
> +
> + 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->external_irq = gpio_to_irq(hdata->hpd_gpio);
> +
> + if (hdata->external_irq < 0) {
> + DRM_ERROR("failed to get platform external irq\n");
> + ret = hdata->external_irq;
> + goto err_hdmiphy;
> + }
> +
> + hdata->internal_irq = platform_get_irq(pdev, 0);
> +
> + if (hdata->internal_irq < 0) {
> + DRM_ERROR("failed to get platform internal irq\n");
> + ret = hdata->internal_irq;
> + goto err_hdmiphy;
> + }
> +
> + ret = request_threaded_irq(hdata->external_irq, NULL,
> + hdmi_external_irq_thread, IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + "hdmi_external", drm_hdmi_ctx);
> + if (ret) {
> + DRM_ERROR("failed to register hdmi external interrupt\n");
> + goto err_hdmiphy;
> + }
> +
> + hdmi_cfg_hpd(hdata, false);
> +
> + ret = request_threaded_irq(hdata->internal_irq, NULL,
> + hdmi_internal_irq_thread, IRQF_ONESHOT,
> + "hdmi_internal", drm_hdmi_ctx);
> + if (ret) {
> + DRM_ERROR("failed to register hdmi internal interrupt\n");
> + goto err_free_irq;
> + }
>
> return 0;
>
> @@ -2391,6 +2574,41 @@ err_data:
> return ret;
> }
>
> +static int __devinit hdmi_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct exynos_drm_hdmi_context *drm_hdmi_ctx;
> + bool is_soc_exynos5 = false;
> + int ret;
> +
> + DRM_DEBUG_KMS("[%d][%s]\n", __LINE__, __func__);
> +
> + if (pdev->dev.of_node &&
> + of_device_is_compatible(pdev->dev.of_node,
> + "samsung,exynos5-hdmi")) {
> + is_soc_exynos5 = true;
> + }
> +
> + /* acquire resources: regs, irqs, clocks */
> + if (is_soc_exynos5)
> + ret = hdmi_resources_init_exynos5(pdev);
> + else
> + ret = hdmi_resources_init_exynos4(pdev);
I think it isn't good to split using is_soc_exynos5 because the exynos5
hdmi is almost same that of exynos4x12.
> + if (ret)
> + goto err_data;
> +
> + drm_hdmi_ctx = platform_get_drvdata(pdev);
> +
> + /* register specific callbacks to common hdmi. */
> + exynos_hdmi_ops_register(&hdmi_ops);
> +
> + pm_runtime_enable(dev);
> +
> + return 0;
> +
> +err_data: return ret;
> +}
> +
> static int __devexit hdmi_remove(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -2444,12 +2662,32 @@ static int hdmi_resume(struct device *dev)
>
> static SIMPLE_DEV_PM_OPS(hdmi_pm_ops, hdmi_suspend, hdmi_resume);
>
> +static struct platform_device_id hdmi_driver_types[] = {
> + {
> + .name = "exynos5-hdmi",
> + }, {
> + .name = "exynos4-hdmi",
> + }, {
> + /* end node */
> + }
> +};
> +
> +static struct of_device_id hdmi_match_types[] = {
> + {
> + .compatible = "samsung,exynos5-hdmi",
> + }, {
> + /* end node */
> + }
> +};
> +
> struct platform_driver hdmi_driver = {
> .probe = hdmi_probe,
> .remove = __devexit_p(hdmi_remove),
> + .id_table = hdmi_driver_types,
> .driver = {
> - .name = "exynos4-hdmi",
> + .name = "exynos-hdmi",
> .owner = THIS_MODULE,
> .pm = &hdmi_pm_ops,
> + .of_match_table = hdmi_match_types,
> },
> };
Thanks.
next prev parent reply other threads:[~2012-09-13 1:23 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-12 12:08 [PATCH 0/3] drm: exynos: hdmi: add exynos5 support for drm hdmi Rahul Sharma
2012-09-12 12:08 ` [PATCH 1/3] drm: exynos: hdmi: add exynos5 support to mixer driver Rahul Sharma
2012-09-13 1:43 ` Joonyoung Shim
2012-09-13 2:53 ` Inki Dae
2012-09-13 3:34 ` Joonyoung Shim
2012-09-13 4:37 ` Inki Dae
2012-09-12 12:08 ` [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver Rahul Sharma
2012-09-13 1:23 ` Joonyoung Shim [this message]
2012-09-12 12:08 ` [PATCH 3/3] drm: exynos: hdmi: clean dependency on plf data for mixer, hdmi context Rahul Sharma
2012-09-13 2:12 ` Joonyoung Shim
-- strict thread matches above, loose matches on Subject: below --
2012-09-13 6:41 [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver RAHUL SHARMA
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5051358D.3040402@samsung.com \
--to=jy0922.shim@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=fahad.k@samsung.com \
--cc=joshi@samsung.com \
--cc=kyungmin.park@samsung.com \
--cc=l.krishna@samsung.com \
--cc=prashanth.g@samsung.com \
--cc=rahul.sharma@samsung.com \
--cc=s.shirish@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.