[PATCH v6 0/1] Add display-timing node parsing to exynos drm fimd

[PATCH v6 0/1] Add display-timing node parsing to exynos drm fimd

[Vikas Sajjan]

Add display-timing node parsing to drm fimd and depends on
the display helper patchset at
http://lists.freedesktop.org/archives/dri-devel/2013-January/033998.html

It also adds pinctrl support for drm fimd.

changes since v5:
	- addressed comments from Inki Dae <inki.dae@samsung.com>,
	to remove the allocation of 'fbmode' and replaced
	'-1'in "of_get_fb_videomode(dev->of_node, fbmode, -1)" with
	OF_USE_NATIVE_MODE.

changes since v4:
	- addressed comments from Paul Menzel 
	<paulepanter@users.sourceforge.net>, to modify the commit message

changes since v3:
	- addressed comments from Sean Paul <seanpaul@chromium.org>, to modify
	the return values and print messages.

changes since v2:
	- moved 'devm_pinctrl_get_select_default' function call under
		'if (pdev->dev.of_node)', this makes NON-DT code unchanged.
		(reported by: Rahul Sharma <r.sh.open@gmail.com>)

changes since v1:
	- addressed comments from Sean Paul <seanpaul@chromium.org>

Vikas Sajjan (1):
  video: drm: exynos: Add display-timing node parsing using video
    helper function

 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37 ++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Vikas Sajjan]

Add support for parsing the display-timing node using video helper
function.

The DT node parsing and pinctrl selection is done only if 'dev.of_node'
exists and the NON-DT logic is still maintained under the 'else' part.

Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37 ++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 9537761..8b2c0ff 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -19,7 +19,9 @@
 #include <linux/clk.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pinctrl/consumer.h>
 
+#include <video/of_display_timing.h>
 #include <video/samsung_fimd.h>
 #include <drm/exynos_drm.h>
 
@@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device *pdev)
 	struct exynos_drm_subdrv *subdrv;
 	struct exynos_drm_fimd_pdata *pdata;
 	struct exynos_drm_panel_info *panel;
+	struct fb_videomode *fbmode;
+	struct pinctrl *pctrl;
 	struct resource *res;
 	int win;
 	int ret = -EINVAL;
 
 	DRM_DEBUG_KMS("%s\n", __FILE__);
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata) {
-		dev_err(dev, "no platform data specified\n");
-		return -EINVAL;
+	if (pdev->dev.of_node) {
+		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata) {
+			DRM_ERROR("memory allocation for pdata failed\n");
+			return -ENOMEM;
+		}
+
+		fbmode = &pdata->panel.timing;
+		ret = of_get_fb_videomode(dev->of_node, fbmode,
+					OF_USE_NATIVE_MODE);
+		if (ret) {
+			DRM_ERROR("failed: of_get_fb_videomode()\n"
+				"with return value: %d\n", ret);
+			return ret;
+		}
+
+		pctrl = devm_pinctrl_get_select_default(dev);
+		if (IS_ERR_OR_NULL(pctrl)) {
+			DRM_ERROR("failed: devm_pinctrl_get_select_default()\n"
+				"with return value: %d\n", PTR_RET(pctrl));
+			return PTR_RET(pctrl);
+		}
+
+	} else {
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			DRM_ERROR("no platform data specified\n");
+			return -EINVAL;
+		}
 	}
 
 	panel = &pdata->panel;
-- 
1.7.9.5

RE: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Inki Dae]

> -----Original Message-----
> From: Vikas Sajjan [mailto:vikas.sajjan@linaro.org]
> Sent: Friday, February 15, 2013 3:43 PM
> To: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org; kgene.kim@samsung.com;
> inki.dae@samsung.com; l.krishna@samsung.com; patches@linaro.org
> Subject: [PATCH v6 1/1] video: drm: exynos: Add display-timing node
> parsing using video helper function
> 
> Add support for parsing the display-timing node using video helper
> function.
> 
> The DT node parsing and pinctrl selection is done only if 'dev.of_node'
> exists and the NON-DT logic is still maintained under the 'else' part.
> 
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37
> ++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..8b2c0ff 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,7 +19,9 @@
>  #include <linux/clk.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> 
> +#include <video/of_display_timing.h>
>  #include <video/samsung_fimd.h>
>  #include <drm/exynos_drm.h>
> 
> @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device *pdev)
>  	struct exynos_drm_subdrv *subdrv;
>  	struct exynos_drm_fimd_pdata *pdata;
>  	struct exynos_drm_panel_info *panel;
> +	struct fb_videomode *fbmode;
> +	struct pinctrl *pctrl;
>  	struct resource *res;
>  	int win;
>  	int ret = -EINVAL;
> 
>  	DRM_DEBUG_KMS("%s\n", __FILE__);
> 
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata) {
> -		dev_err(dev, "no platform data specified\n");
> -		return -EINVAL;
> +	if (pdev->dev.of_node) {
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata) {
> +			DRM_ERROR("memory allocation for pdata failed\n");
> +			return -ENOMEM;
> +		}
> +
> +		fbmode = &pdata->panel.timing;
> +		ret = of_get_fb_videomode(dev->of_node, fbmode,
> +					OF_USE_NATIVE_MODE);
> +		if (ret) {
> +			DRM_ERROR("failed: of_get_fb_videomode()\n"
> +				"with return value: %d\n", ret);
> +			return ret;
> +		}
> +
> +		pctrl = devm_pinctrl_get_select_default(dev);

Why does it need pinctrl? and even though needed, I think this should be
separated into another one.

Thanks,
Inki Dae

> +		if (IS_ERR_OR_NULL(pctrl)) {
> +			DRM_ERROR("failed:
> devm_pinctrl_get_select_default()\n"
> +				"with return value: %d\n", PTR_RET(pctrl));
> +			return PTR_RET(pctrl);
> +		}
> +
> +	} else {
> +		pdata = pdev->dev.platform_data;
> +		if (!pdata) {
> +			DRM_ERROR("no platform data specified\n");
> +			return -EINVAL;
> +		}
>  	}
> 
>  	panel = &pdata->panel;
> --
> 1.7.9.5

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Vikas Sajjan]

Hi Mr. Inki Dae,

On 20 February 2013 16:45, Inki Dae <inki.dae@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: Vikas Sajjan [mailto:vikas.sajjan@linaro.org]
>> Sent: Friday, February 15, 2013 3:43 PM
>> To: dri-devel@lists.freedesktop.org
>> Cc: linux-media@vger.kernel.org; kgene.kim@samsung.com;
>> inki.dae@samsung.com; l.krishna@samsung.com; patches@linaro.org
>> Subject: [PATCH v6 1/1] video: drm: exynos: Add display-timing node
>> parsing using video helper function
>>
>> Add support for parsing the display-timing node using video helper
>> function.
>>
>> The DT node parsing and pinctrl selection is done only if 'dev.of_node'
>> exists and the NON-DT logic is still maintained under the 'else' part.
>>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37
>> ++++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 9537761..8b2c0ff 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -19,7 +19,9 @@
>>  #include <linux/clk.h>
>>  #include <linux/of_device.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>
>> +#include <video/of_display_timing.h>
>>  #include <video/samsung_fimd.h>
>>  #include <drm/exynos_drm.h>
>>
>> @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device *pdev)
>>       struct exynos_drm_subdrv *subdrv;
>>       struct exynos_drm_fimd_pdata *pdata;
>>       struct exynos_drm_panel_info *panel;
>> +     struct fb_videomode *fbmode;
>> +     struct pinctrl *pctrl;
>>       struct resource *res;
>>       int win;
>>       int ret = -EINVAL;
>>
>>       DRM_DEBUG_KMS("%s\n", __FILE__);
>>
>> -     pdata = pdev->dev.platform_data;
>> -     if (!pdata) {
>> -             dev_err(dev, "no platform data specified\n");
>> -             return -EINVAL;
>> +     if (pdev->dev.of_node) {
>> +             pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +             if (!pdata) {
>> +                     DRM_ERROR("memory allocation for pdata failed\n");
>> +                     return -ENOMEM;
>> +             }
>> +
>> +             fbmode = &pdata->panel.timing;
>> +             ret = of_get_fb_videomode(dev->of_node, fbmode,
>> +                                     OF_USE_NATIVE_MODE);
>> +             if (ret) {
>> +                     DRM_ERROR("failed: of_get_fb_videomode()\n"
>> +                             "with return value: %d\n", ret);
>> +                     return ret;
>> +             }
>> +
>> +             pctrl = devm_pinctrl_get_select_default(dev);
>
> Why does it need pinctrl? and even though needed, I think this should be
> separated into another one.
>
Will separate it out and send it in a separate patch.

> Thanks,
> Inki Dae
>
>> +             if (IS_ERR_OR_NULL(pctrl)) {
>> +                     DRM_ERROR("failed:
>> devm_pinctrl_get_select_default()\n"
>> +                             "with return value: %d\n", PTR_RET(pctrl));
>> +                     return PTR_RET(pctrl);
>> +             }
>> +
>> +     } else {
>> +             pdata = pdev->dev.platform_data;
>> +             if (!pdata) {
>> +                     DRM_ERROR("no platform data specified\n");
>> +                     return -EINVAL;
>> +             }
>>       }
>>
>>       panel = &pdata->panel;
>> --
>> 1.7.9.5
>



-- 
Thanks and Regards
 Vikas Sajjan

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Joonyoung Shim]

Hi,

On 02/15/2013 03:43 PM, Vikas Sajjan wrote:
> Add support for parsing the display-timing node using video helper
> function.
>
> The DT node parsing and pinctrl selection is done only if 'dev.of_node'
> exists and the NON-DT logic is still maintained under the 'else' part.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> ---
>   drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37 ++++++++++++++++++++++++++----
>   1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 9537761..8b2c0ff 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,7 +19,9 @@
>   #include <linux/clk.h>
>   #include <linux/of_device.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
>   
> +#include <video/of_display_timing.h>
>   #include <video/samsung_fimd.h>
>   #include <drm/exynos_drm.h>
>   
> @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device *pdev)
>   	struct exynos_drm_subdrv *subdrv;
>   	struct exynos_drm_fimd_pdata *pdata;
>   	struct exynos_drm_panel_info *panel;
> +	struct fb_videomode *fbmode;
> +	struct pinctrl *pctrl;
>   	struct resource *res;
>   	int win;
>   	int ret = -EINVAL;
>   
>   	DRM_DEBUG_KMS("%s\n", __FILE__);
>   
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata) {
> -		dev_err(dev, "no platform data specified\n");
> -		return -EINVAL;
> +	if (pdev->dev.of_node) {
> +		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata) {
> +			DRM_ERROR("memory allocation for pdata failed\n");
> +			return -ENOMEM;
> +		}
> +
> +		fbmode = &pdata->panel.timing;
> +		ret = of_get_fb_videomode(dev->of_node, fbmode,
> +					OF_USE_NATIVE_MODE);

fbmode variable can be substituted to &pdata->panel.timing directly then 
can remove fbmode variable.

> +		if (ret) {
> +			DRM_ERROR("failed: of_get_fb_videomode()\n"
> +				"with return value: %d\n", ret);
> +			return ret;
> +		}
> +
> +		pctrl = devm_pinctrl_get_select_default(dev);
> +		if (IS_ERR_OR_NULL(pctrl)) {

It's enough to "if (IS_ERR(pctrl)) {".

> +			DRM_ERROR("failed: devm_pinctrl_get_select_default()\n"
> +				"with return value: %d\n", PTR_RET(pctrl));
> +			return PTR_RET(pctrl);

It's enough to "return PTR_ERR(pctrl);"

> +		}

If this needs, parts for pinctrl should go to another patch.

> +
> +	} else {
> +		pdata = pdev->dev.platform_data;
> +		if (!pdata) {
> +			DRM_ERROR("no platform data specified\n");
> +			return -EINVAL;
> +		}
>   	}
>   
>   	panel = &pdata->panel;

Thanks.

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Vikas Sajjan]

Hi,

On 21 February 2013 12:25, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
> Hi,
>
>
> On 02/15/2013 03:43 PM, Vikas Sajjan wrote:
>>
>> Add support for parsing the display-timing node using video helper
>> function.
>>
>> The DT node parsing and pinctrl selection is done only if 'dev.of_node'
>> exists and the NON-DT logic is still maintained under the 'else' part.
>>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37
>> ++++++++++++++++++++++++++----
>>   1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 9537761..8b2c0ff 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -19,7 +19,9 @@
>>   #include <linux/clk.h>
>>   #include <linux/of_device.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/pinctrl/consumer.h>
>>   +#include <video/of_display_timing.h>
>>   #include <video/samsung_fimd.h>
>>   #include <drm/exynos_drm.h>
>>   @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device
>> *pdev)
>>         struct exynos_drm_subdrv *subdrv;
>>         struct exynos_drm_fimd_pdata *pdata;
>>         struct exynos_drm_panel_info *panel;
>> +       struct fb_videomode *fbmode;
>> +       struct pinctrl *pctrl;
>>         struct resource *res;
>>         int win;
>>         int ret = -EINVAL;
>>         DRM_DEBUG_KMS("%s\n", __FILE__);
>>   -     pdata = pdev->dev.platform_data;
>> -       if (!pdata) {
>> -               dev_err(dev, "no platform data specified\n");
>> -               return -EINVAL;
>> +       if (pdev->dev.of_node) {
>> +               pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +               if (!pdata) {
>> +                       DRM_ERROR("memory allocation for pdata failed\n");
>> +                       return -ENOMEM;
>> +               }
>> +
>> +               fbmode = &pdata->panel.timing;
>> +               ret = of_get_fb_videomode(dev->of_node, fbmode,
>> +                                       OF_USE_NATIVE_MODE);
>
>
> fbmode variable can be substituted to &pdata->panel.timing directly then can
> remove fbmode variable.
>
this is can be done.
>
>> +               if (ret) {
>> +                       DRM_ERROR("failed: of_get_fb_videomode()\n"
>> +                               "with return value: %d\n", ret);
>> +                       return ret;
>> +               }
>> +
>> +               pctrl = devm_pinctrl_get_select_default(dev);
>> +               if (IS_ERR_OR_NULL(pctrl)) {
>
>
> It's enough to "if (IS_ERR(pctrl)) {".
>
what if it returns NULL?
>
>> +                       DRM_ERROR("failed:
>> devm_pinctrl_get_select_default()\n"
>> +                               "with return value: %d\n",
>> PTR_RET(pctrl));
>> +                       return PTR_RET(pctrl);
>
>
> It's enough to "return PTR_ERR(pctrl);"
>
ok.

>> +               }
>
>
> If this needs, parts for pinctrl should go to another patch.
>
I have it as a separate patch already. [PATCH v7 2/2] video: drm:
exynos: Add pinctrl support to fimd
>
>> +
>> +       } else {
>> +               pdata = pdev->dev.platform_data;
>> +               if (!pdata) {
>> +                       DRM_ERROR("no platform data specified\n");
>> +                       return -EINVAL;
>> +               }
>>         }
>>         panel = &pdata->panel;
>
>
> Thanks.



-- 
Thanks and Regards
 Vikas Sajjan

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Joonyoung Shim]

On 02/21/2013 04:12 PM, Vikas Sajjan wrote:
> Hi,
>
> On 21 February 2013 12:25, Joonyoung Shim <jy0922.shim@samsung.com> wrote:
>> Hi,
>>
>>
>> On 02/15/2013 03:43 PM, Vikas Sajjan wrote:
>>> Add support for parsing the display-timing node using video helper
>>> function.
>>>
>>> The DT node parsing and pinctrl selection is done only if 'dev.of_node'
>>> exists and the NON-DT logic is still maintained under the 'else' part.
>>>
>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>>> ---
>>>    drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37
>>> ++++++++++++++++++++++++++----
>>>    1 file changed, 33 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> index 9537761..8b2c0ff 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>> @@ -19,7 +19,9 @@
>>>    #include <linux/clk.h>
>>>    #include <linux/of_device.h>
>>>    #include <linux/pm_runtime.h>
>>> +#include <linux/pinctrl/consumer.h>
>>>    +#include <video/of_display_timing.h>
>>>    #include <video/samsung_fimd.h>
>>>    #include <drm/exynos_drm.h>
>>>    @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device
>>> *pdev)
>>>          struct exynos_drm_subdrv *subdrv;
>>>          struct exynos_drm_fimd_pdata *pdata;
>>>          struct exynos_drm_panel_info *panel;
>>> +       struct fb_videomode *fbmode;
>>> +       struct pinctrl *pctrl;
>>>          struct resource *res;
>>>          int win;
>>>          int ret = -EINVAL;
>>>          DRM_DEBUG_KMS("%s\n", __FILE__);
>>>    -     pdata = pdev->dev.platform_data;
>>> -       if (!pdata) {
>>> -               dev_err(dev, "no platform data specified\n");
>>> -               return -EINVAL;
>>> +       if (pdev->dev.of_node) {
>>> +               pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> +               if (!pdata) {
>>> +                       DRM_ERROR("memory allocation for pdata failed\n");
>>> +                       return -ENOMEM;
>>> +               }
>>> +
>>> +               fbmode = &pdata->panel.timing;
>>> +               ret = of_get_fb_videomode(dev->of_node, fbmode,
>>> +                                       OF_USE_NATIVE_MODE);
>>
>> fbmode variable can be substituted to &pdata->panel.timing directly then can
>> remove fbmode variable.
>>
> this is can be done.
>>> +               if (ret) {
>>> +                       DRM_ERROR("failed: of_get_fb_videomode()\n"
>>> +                               "with return value: %d\n", ret);
>>> +                       return ret;
>>> +               }
>>> +
>>> +               pctrl = devm_pinctrl_get_select_default(dev);
>>> +               if (IS_ERR_OR_NULL(pctrl)) {
>>
>> It's enough to "if (IS_ERR(pctrl)) {".
>>
> what if it returns NULL?

devm_pinctrl_get_select_default() never return NULL.


>>> +                       DRM_ERROR("failed:
>>> devm_pinctrl_get_select_default()\n"
>>> +                               "with return value: %d\n",
>>> PTR_RET(pctrl));
>>> +                       return PTR_RET(pctrl);
>>
>> It's enough to "return PTR_ERR(pctrl);"
>>
> ok.
>
>>> +               }
>>
>> If this needs, parts for pinctrl should go to another patch.
>>
> I have it as a separate patch already. [PATCH v7 2/2] video: drm:
> exynos: Add pinctrl support to fimd
>>> +
>>> +       } else {
>>> +               pdata = pdev->dev.platform_data;
>>> +               if (!pdata) {
>>> +                       DRM_ERROR("no platform data specified\n");
>>> +                       return -EINVAL;
>>> +               }
>>>          }
>>>          panel = &pdata->panel;
>>
>> Thanks.
>
>

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Joonyoung Shim]

Hi,

On 02/21/2013 04:18 PM, Joonyoung Shim wrote:
> On 02/21/2013 04:12 PM, Vikas Sajjan wrote:
>> Hi,
>>
>> On 21 February 2013 12:25, Joonyoung Shim <jy0922.shim@samsung.com> 
>> wrote:
>>> Hi,
>>>
>>>
>>> On 02/15/2013 03:43 PM, Vikas Sajjan wrote:
>>>> Add support for parsing the display-timing node using video helper
>>>> function.
>>>>
>>>> The DT node parsing and pinctrl selection is done only if 
>>>> 'dev.of_node'
>>>> exists and the NON-DT logic is still maintained under the 'else' part.
>>>>
>>>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>>>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/exynos/exynos_drm_fimd.c |   37
>>>> ++++++++++++++++++++++++++----
>>>>    1 file changed, 33 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>> index 9537761..8b2c0ff 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>>>> @@ -19,7 +19,9 @@
>>>>    #include <linux/clk.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/pm_runtime.h>
>>>> +#include <linux/pinctrl/consumer.h>
>>>>    +#include <video/of_display_timing.h>
>>>>    #include <video/samsung_fimd.h>
>>>>    #include <drm/exynos_drm.h>
>>>>    @@ -877,16 +879,43 @@ static int fimd_probe(struct platform_device
>>>> *pdev)
>>>>          struct exynos_drm_subdrv *subdrv;
>>>>          struct exynos_drm_fimd_pdata *pdata;
>>>>          struct exynos_drm_panel_info *panel;
>>>> +       struct fb_videomode *fbmode;
>>>> +       struct pinctrl *pctrl;
>>>>          struct resource *res;
>>>>          int win;
>>>>          int ret = -EINVAL;
>>>>          DRM_DEBUG_KMS("%s\n", __FILE__);
>>>>    -     pdata = pdev->dev.platform_data;
>>>> -       if (!pdata) {
>>>> -               dev_err(dev, "no platform data specified\n");
>>>> -               return -EINVAL;
>>>> +       if (pdev->dev.of_node) {
>>>> +               pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>>> +               if (!pdata) {
>>>> +                       DRM_ERROR("memory allocation for pdata 
>>>> failed\n");
>>>> +                       return -ENOMEM;
>>>> +               }
>>>> +
>>>> +               fbmode = &pdata->panel.timing;
>>>> +               ret = of_get_fb_videomode(dev->of_node, fbmode,
>>>> +                                       OF_USE_NATIVE_MODE);
>>>
>>> fbmode variable can be substituted to &pdata->panel.timing directly 
>>> then can
>>> remove fbmode variable.
>>>
>> this is can be done.
>>>> +               if (ret) {
>>>> +                       DRM_ERROR("failed: of_get_fb_videomode()\n"
>>>> +                               "with return value: %d\n", ret);
>>>> +                       return ret;
>>>> +               }
>>>> +
>>>> +               pctrl = devm_pinctrl_get_select_default(dev);
>>>> +               if (IS_ERR_OR_NULL(pctrl)) {
>>>
>>> It's enough to "if (IS_ERR(pctrl)) {".
>>>
>> what if it returns NULL?
>
> devm_pinctrl_get_select_default() never return NULL.
>

My mistake. If CONFIG_PINCTRL is disabled, 
devm_pinctrl_get_select_default can return NULL.

Linus,

devm_pinctrl_get_select() and pinctrl_get_select() also need 
IS_ERR_OR_NULL instead of IS_ERR?
And many drivers using above functions don't check NULL, right?

Thanks.

>
>>>> + DRM_ERROR("failed:
>>>> devm_pinctrl_get_select_default()\n"
>>>> +                               "with return value: %d\n",
>>>> PTR_RET(pctrl));
>>>> +                       return PTR_RET(pctrl);
>>>
>>> It's enough to "return PTR_ERR(pctrl);"
>>>
>> ok.
>>
>>>> +               }
>>>
>>> If this needs, parts for pinctrl should go to another patch.
>>>
>> I have it as a separate patch already. [PATCH v7 2/2] video: drm:
>> exynos: Add pinctrl support to fimd
>>>> +
>>>> +       } else {
>>>> +               pdata = pdev->dev.platform_data;
>>>> +               if (!pdata) {
>>>> +                       DRM_ERROR("no platform data specified\n");
>>>> +                       return -EINVAL;
>>>> +               }
>>>>          }
>>>>          panel = &pdata->panel;
>>>
>>> Thanks.
>>
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH v6 1/1] video: drm: exynos: Add display-timing node parsing using video helper function

[Linus Walleij]

On Thu, Feb 28, 2013 at 2:51 AM, Joonyoung Shim <jy0922.shim@samsung.com> wrote:

> My mistake. If CONFIG_PINCTRL is disabled, devm_pinctrl_get_select_default
> can return NULL.

Yes, and that is perfectly legal and *NOT* an error.

> devm_pinctrl_get_select() and pinctrl_get_select() also need IS_ERR_OR_NULL
> instead of IS_ERR?

No.

In fact, IS_ERR_OR_NULL() shall not be used at all.

Check the LKML mailinglist for Russells recent struggle to
purge it from the kernel.

> And many drivers using above functions don't check NULL, right?

No, and they should not. Stub pinctrl handles, just like stub
clocks and stub regulators, are perfectly legal, just that they have
no effect.

Yours,
Linus Walleij