From: RAHUL SHARMA <rahul.sharma@samsung.com>
To: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: SHIRISH S <s.shirish@samsung.com>,
Leela Krishna Amudala <l.krishna@samsung.com>,
SUNIL JOSHI <joshi@samsung.com>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
Kyungmin Park <kyungmin.park@samsung.com>,
FAHAD KUNNATHADI <fahad.k@samsung.com>,
PRASHANTH GODREHAL <prashanth.g@samsung.com>
Subject: Re: [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver
Date: Thu, 13 Sep 2012 06:41:32 +0000 (GMT) [thread overview]
Message-ID: <26545844.824001347518491867.JavaMail.weblogic@epml02> (raw)
> 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.
>
I had no option to pass above function pointers through hdmi device tree node
in exynos5. I can still pass these pointers through plf data by searching the
DT node and initializing the platform data ptr in the Machine Init file (mach-ecynos5-dt.c)
which is again not recommended.
I preferred this approach since many drivers LCD, eeprom, nand drivers configuring, getting
GPIO this way. One or the other way we have to go off-track.
Please give me your opinion.
>
>> 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.
>
Again like mixer, is_soc_exynos5 is used because the way of getting resources is
different in for both. But as I proposed, I can use single function for both and using
pdev->dev.of_node to decide, how to get a given resource. (like GPIO)
Thanks
>
>> + 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 reply other threads:[~2012-09-13 6:41 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-13 6:41 RAHUL SHARMA [this message]
-- strict thread matches above, loose matches on Subject: below --
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 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver Rahul Sharma
2012-09-13 1:23 ` Joonyoung Shim
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=26545844.824001347518491867.JavaMail.weblogic@epml02 \
--to=rahul.sharma@samsung.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=fahad.k@samsung.com \
--cc=joshi@samsung.com \
--cc=jy0922.shim@samsung.com \
--cc=kyungmin.park@samsung.com \
--cc=l.krishna@samsung.com \
--cc=prashanth.g@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.