[PATCH v9 0/2] Add display-timing node parsing to exynos drm fimd

[PATCH v9 0/2] 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 v8:
	- replaced IS_ERR() with IS_ERR_OR_NULL(),
	because devm_pinctrl_get_select_default can return NULL,
	If CONFIG_PINCTRL is disabled.
	- modified the error log, such that it shall NOT cross 80 column.
	- added Acked-by.
changes since v7:
	- addressed comments from Joonyoung Shim <jy0922.shim@samsung.com> 
	to remove a unnecessary variable.

changes since v6:
	addressed comments from Inki Dae <inki.dae@samsung.com> to
	separated out the pinctrl functionality and made a separate patch.

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 (2):
  video: drm: exynos: Add display-timing node parsing using video
    helper function
  video: drm: exynos: Add pinctrl support to fimd

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

-- 
1.7.9.5

[PATCH v9 1/2] 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 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>
Acked-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |   24 ++++++++++++++++++++----
 1 file changed, 20 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..e323cf9 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -20,6 +20,7 @@
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 
+#include <video/of_display_timing.h>
 #include <video/samsung_fimd.h>
 #include <drm/exynos_drm.h>
 
@@ -883,10 +884,25 @@ static int fimd_probe(struct platform_device *pdev)
 
 	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;
+		}
+
+		ret = of_get_fb_videomode(dev->of_node, &pdata->panel.timing,
+					OF_USE_NATIVE_MODE);
+		if (ret) {
+			DRM_ERROR("failed: of_get_fb_videomode() : %d\n", ret);
+			return ret;
+		}
+	} else {
+		pdata = pdev->dev.platform_data;
+		if (!pdata) {
+			DRM_ERROR("no platform data specified\n");
+			return -EINVAL;
+		}
 	}
 
 	panel = &pdata->panel;
-- 
1.7.9.5

[PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Vikas Sajjan]

Adds support for pinctrl to drm fimd

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 |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index e323cf9..21ada8d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -19,6 +19,7 @@
 #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>
@@ -879,6 +880,7 @@ static int fimd_probe(struct platform_device *pdev)
 	struct exynos_drm_fimd_pdata *pdata;
 	struct exynos_drm_panel_info *panel;
 	struct resource *res;
+	struct pinctrl *pctrl;
 	int win;
 	int ret = -EINVAL;
 
@@ -897,6 +899,13 @@ static int fimd_probe(struct platform_device *pdev)
 			DRM_ERROR("failed: of_get_fb_videomode() : %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():"
+				"%d\n", PTR_RET(pctrl));
+			return PTR_ERR(pctrl);
+		}
+
 	} else {
 		pdata = pdev->dev.platform_data;
 		if (!pdata) {
-- 
1.7.9.5

[PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Sylwester Nawrocki]

On 02/28/2013 05:12 AM, Vikas Sajjan wrote:
> Adds support for pinctrl to drm fimd
>
> 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 |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index e323cf9..21ada8d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,6 +19,7 @@
>   #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>
> @@ -879,6 +880,7 @@ static int fimd_probe(struct platform_device *pdev)
>   	struct exynos_drm_fimd_pdata *pdata;
>   	struct exynos_drm_panel_info *panel;
>   	struct resource *res;
> +	struct pinctrl *pctrl;
>   	int win;
>   	int ret = -EINVAL;
>
> @@ -897,6 +899,13 @@ static int fimd_probe(struct platform_device *pdev)
>   			DRM_ERROR("failed: of_get_fb_videomode() : %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():"
> +				"%d\n", PTR_RET(pctrl));
> +			return PTR_ERR(pctrl);

In situations like this I really side attempts to remove IS_ERR_OR_NULL()
macro from the kernel completely ([1], [2]). What is the value returned 
from
fimd_probe() when devm_pinctrl_get_select_default() returns NULL ?

What header file have you added to use struct pinctrl in this driver ?
Is this data structure fully declared there ? Are drivers supposed to
dereference struct pinctrl at all ?

I believe original intention was to have the pinctrl handle as an opaque
cookie, and as long as it is used with the pinctrl API only and tested
for errors with *IS_ERR()*, everything should be fine. The pinctrl API
should handle any NULL pointer as it returned it to a driver in the first
place.

Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity.

> +		}
> +
>   	} else {
>   		pdata = pdev->dev.platform_data;
>   		if (!pdata) {

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/140543.html
[2] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg78030.html

Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Sylwester Nawrocki]

On 02/28/2013 05:12 AM, Vikas Sajjan wrote:
> Adds support for pinctrl to drm fimd
>
> 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 |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index e323cf9..21ada8d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,6 +19,7 @@
>   #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>
> @@ -879,6 +880,7 @@ static int fimd_probe(struct platform_device *pdev)
>   	struct exynos_drm_fimd_pdata *pdata;
>   	struct exynos_drm_panel_info *panel;
>   	struct resource *res;
> +	struct pinctrl *pctrl;
>   	int win;
>   	int ret = -EINVAL;
>
> @@ -897,6 +899,13 @@ static int fimd_probe(struct platform_device *pdev)
>   			DRM_ERROR("failed: of_get_fb_videomode() : %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():"
> +				"%d\n", PTR_RET(pctrl));
> +			return PTR_ERR(pctrl);

In situations like this I really side attempts to remove IS_ERR_OR_NULL()
macro from the kernel completely ([1], [2]). What is the value returned 
from
fimd_probe() when devm_pinctrl_get_select_default() returns NULL ?

What header file have you added to use struct pinctrl in this driver ?
Is this data structure fully declared there ? Are drivers supposed to
dereference struct pinctrl at all ?

I believe original intention was to have the pinctrl handle as an opaque
cookie, and as long as it is used with the pinctrl API only and tested
for errors with *IS_ERR()*, everything should be fine. The pinctrl API
should handle any NULL pointer as it returned it to a driver in the first
place.

Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity.

> +		}
> +
>   	} else {
>   		pdata = pdev->dev.platform_data;
>   		if (!pdata) {

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-January/140543.html
[2] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg78030.html

[PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Russell King - ARM Linux]

On Thu, Feb 28, 2013 at 11:03:57PM +0100, Sylwester Nawrocki wrote:
> Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity.

Yes, indeed.

On that topic (and off-topic for this thread, sorry) I've committed
a set of patches to remove most users of IS_ERR_OR_NULL() from arch/arm.
These are the last remaining ones there - and I don't want to see any
more appearing:

arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk))
arch/arm/plat-samsung/clock.c:	if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->round_rate)
arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk))
arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk) || IS_ERR_OR_NULL(parent))
arch/arm/mach-imx/devices/platform-ipu-core.c:	if (IS_ERR_OR_NULL(imx_ipu_coredev))
arch/arm/mach-imx/devices/platform-ipu-core.c:	if (IS_ERR_OR_NULL(imx_ipu_coredev))
arch/arm/kernel/smp_twd.c:		 * We use IS_ERR_OR_NULL() here, because if the clock stubs
arch/arm/kernel/smp_twd.c:		if (!IS_ERR_OR_NULL(twd_clk))

They currently all legal uses of it - though I'm sure that the samsung
clock uses can be reduced to just IS_ERR().  The IMX use looks "valid"
in that imx_ipu_coredev really can be an error pointer (on failure) or
NULL if the platform device hasn't yet been created.  The TWD one is
explained in the comments in the code (if people had to write explanations
for using IS_ERR_OR_NULL(), we'd probably have it used correctly!)

Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Russell King - ARM Linux]

On Thu, Feb 28, 2013 at 11:03:57PM +0100, Sylwester Nawrocki wrote:
> Please just use IS_ERR(), let's stop this IS_ERR_OR_NULL() insanity.

Yes, indeed.

On that topic (and off-topic for this thread, sorry) I've committed
a set of patches to remove most users of IS_ERR_OR_NULL() from arch/arm.
These are the last remaining ones there - and I don't want to see any
more appearing:

arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk))
arch/arm/plat-samsung/clock.c:	if (!IS_ERR_OR_NULL(clk) && clk->ops && clk->ops->round_rate)
arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk))
arch/arm/plat-samsung/clock.c:	if (IS_ERR_OR_NULL(clk) || IS_ERR_OR_NULL(parent))
arch/arm/mach-imx/devices/platform-ipu-core.c:	if (IS_ERR_OR_NULL(imx_ipu_coredev))
arch/arm/mach-imx/devices/platform-ipu-core.c:	if (IS_ERR_OR_NULL(imx_ipu_coredev))
arch/arm/kernel/smp_twd.c:		 * We use IS_ERR_OR_NULL() here, because if the clock stubs
arch/arm/kernel/smp_twd.c:		if (!IS_ERR_OR_NULL(twd_clk))

They currently all legal uses of it - though I'm sure that the samsung
clock uses can be reduced to just IS_ERR().  The IMX use looks "valid"
in that imx_ipu_coredev really can be an error pointer (on failure) or
NULL if the platform device hasn't yet been created.  The TWD one is
explained in the comments in the code (if people had to write explanations
for using IS_ERR_OR_NULL(), we'd probably have it used correctly!)

Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Linus Walleij]

On Thu, Feb 28, 2013 at 5:12 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:

> Adds support for pinctrl to drm fimd
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
(...)
> +               pctrl = devm_pinctrl_get_select_default(dev);

NAK.

The device core will do this for you as of commit
ab78029ecc347debbd737f06688d788bd9d60c1d
"drivers/pinctrl: grab default handles from device core"

Yours,
Linus Walleij

Re: [PATCH v9 2/2] video: drm: exynos: Add pinctrl support to fimd

[Vikas Sajjan]

Hi Linus,

On Fri, Mar 1, 2013 at 2:49 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 28, 2013 at 5:12 AM, Vikas Sajjan <vikas.sajjan@linaro.org> wrote:
>
>> Adds support for pinctrl to drm fimd
>>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> Signed-off-by: Vikas Sajjan <vikas.sajjan@linaro.org>
> (...)
>> +               pctrl = devm_pinctrl_get_select_default(dev);
>
> NAK.
>
> The device core will do this for you as of commit
> ab78029ecc347debbd737f06688d788bd9d60c1d
> "drivers/pinctrl: grab default handles from device core"
>
OK. Will abandon the patch.

> Yours,
> Linus Walleij
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel