Re: [PATCH 2/3] drm: exynos: hdmi: add exynos5 support to hdmi driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 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


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux