> 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@xxxxxxxxxxx> >> Signed-off-by: Shirish S <s.shirish@xxxxxxxxxxx> >> Signed-off-by: Fahad Kunnathadi <fahad.k@xxxxxxxxxxx> >> --- >> 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. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel