[PATCH RESEND 0/6] Remove incorrect usage of IS_ERR_OR_NULL on clk_get

[PATCH RESEND 0/6] Remove incorrect usage of IS_ERR_OR_NULL on clk_get

[Tony Prisk]

Resend to include mailing lists.

As per the kernel docs, clk_get() cannot return NULL.

 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.

This patch set replaces instances of IS_ERR_OR_NULL tests againt clk_get()
results with IS_ERR tests.

Tony Prisk (6):
  clk: omap: Fix incorrect usage of IS_ERR_OR_NULL
  clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL
  clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL
  clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL
  clk: s5p-fimc: Fix incorrect usage of IS_ERR_OR_NULL
  clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

 arch/arm/plat-omap/dmtimer.c                   |    6 +++---
 drivers/gpu/drm/exynos/exynos_hdmi.c           |   10 +++++-----
 drivers/gpu/drm/exynos/exynos_mixer.c          |   14 +++++++-------
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |    2 +-
 drivers/media/platform/s5p-g2d/g2d.c           |    4 ++--
 drivers/media/platform/s5p-tv/hdmi_drv.c       |   10 +++++-----
 drivers/media/platform/s5p-tv/mixer_drv.c      |   10 +++++-----
 drivers/media/platform/s5p-tv/sdo_drv.c        |   10 +++++-----
 8 files changed, 33 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[PATCH RESEND 1/6] clk: omap: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/plat-omap/dmtimer.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/plat-omap/dmtimer.c b/arch/arm/plat-omap/dmtimer.c
index 938b50a..e5e150d 100644
--- a/arch/arm/plat-omap/dmtimer.c
+++ b/arch/arm/plat-omap/dmtimer.c
@@ -143,7 +143,7 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 	 */
 	if (!(timer->capability & OMAP_TIMER_NEEDS_RESET)) {
 		timer->fclk = clk_get(&timer->pdev->dev, "fck");
-		if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
+		if (WARN_ON_ONCE(IS_ERR(timer->fclk))) {
 			timer->fclk = NULL;
 			dev_err(&timer->pdev->dev, ": No fclk handle.\n");
 			return -EINVAL;
@@ -418,7 +418,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 		return pdata->set_timer_src(timer->pdev, source);
 
 	fclk = clk_get(&timer->pdev->dev, "fck");
-	if (IS_ERR_OR_NULL(fclk)) {
+	if (IS_ERR(fclk)) {
 		pr_err("%s: fck not found\n", __func__);
 		return -EINVAL;
 	}
@@ -438,7 +438,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	}
 
 	parent = clk_get(&timer->pdev->dev, parent_name);
-	if (IS_ERR_OR_NULL(parent)) {
+	if (IS_ERR(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);
 		ret = -EINVAL;
 		goto out;
-- 
1.7.9.5

[PATCH RESEND 2/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

In the fail: path of mixer_resources_init() and vp_resources_init()
the first clk tested cannot be NULL either, so IS_ERR_OR_NULL is
removed from these as well. Other clocks may still be NULL as they
haven't been clk_get'd yet.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Inki Dae <inki.dae@samsung.com>
CC: Joonyoung Shim <jy0922.shim@samsung.com>
CC: Seung-Woo Kim <sw0312.kim@samsung.com>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: dri-devel at lists.freedesktop.org
---
 drivers/gpu/drm/exynos/exynos_mixer.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index e7fbb82..dbd97c0 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -972,14 +972,14 @@ static int __devinit mixer_resources_init(struct exynos_drm_hdmi_context *ctx,
 	spin_lock_init(&mixer_res->reg_slock);
 
 	mixer_res->mixer = clk_get(dev, "mixer");
-	if (IS_ERR_OR_NULL(mixer_res->mixer)) {
+	if (IS_ERR(mixer_res->mixer)) {
 		dev_err(dev, "failed to get clock 'mixer'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 
 	mixer_res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_hdmi)) {
+	if (IS_ERR(mixer_res->sclk_hdmi)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmi'\n");
 		ret = -ENODEV;
 		goto fail;
@@ -1019,7 +1019,7 @@ static int __devinit mixer_resources_init(struct exynos_drm_hdmi_context *ctx,
 fail:
 	if (!IS_ERR_OR_NULL(mixer_res->sclk_hdmi))
 		clk_put(mixer_res->sclk_hdmi);
-	if (!IS_ERR_OR_NULL(mixer_res->mixer))
+	if (!IS_ERR(mixer_res->mixer))
 		clk_put(mixer_res->mixer);
 	return ret;
 }
@@ -1034,19 +1034,19 @@ static int __devinit vp_resources_init(struct exynos_drm_hdmi_context *ctx,
 	int ret;
 
 	mixer_res->vp = clk_get(dev, "vp");
-	if (IS_ERR_OR_NULL(mixer_res->vp)) {
+	if (IS_ERR(mixer_res->vp)) {
 		dev_err(dev, "failed to get clock 'vp'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 	mixer_res->sclk_mixer = clk_get(dev, "sclk_mixer");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_mixer)) {
+	if (IS_ERR(mixer_res->sclk_mixer)) {
 		dev_err(dev, "failed to get clock 'sclk_mixer'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 	mixer_res->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_dac)) {
+	if (IS_ERR(mixer_res->sclk_dac)) {
 		dev_err(dev, "failed to get clock 'sclk_dac'\n");
 		ret = -ENODEV;
 		goto fail;
@@ -1077,7 +1077,7 @@ fail:
 		clk_put(mixer_res->sclk_dac);
 	if (!IS_ERR_OR_NULL(mixer_res->sclk_mixer))
 		clk_put(mixer_res->sclk_mixer);
-	if (!IS_ERR_OR_NULL(mixer_res->vp))
+	if (!IS_ERR(mixer_res->vp))
 		clk_put(mixer_res->vp);
 	return ret;
 }
-- 
1.7.9.5

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Inki Dae <inki.dae@samsung.com>
CC: Joonyoung Shim <jy0922.shim@samsung.com>
CC: Seung-Woo Kim <sw0312.kim@samsung.com>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: dri-devel at lists.freedesktop.org
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 2c115f8..557ef2f 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2167,27 +2167,27 @@ static int __devinit hdmi_resources_init(struct hdmi_context *hdata)
 
 	/* get clocks, power */
 	res->hdmi = clk_get(dev, "hdmi");
-	if (IS_ERR_OR_NULL(res->hdmi)) {
+	if (IS_ERR(res->hdmi)) {
 		DRM_ERROR("failed to get clock 'hdmi'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		DRM_ERROR("failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_pixel = clk_get(dev, "sclk_pixel");
-	if (IS_ERR_OR_NULL(res->sclk_pixel)) {
+	if (IS_ERR(res->sclk_pixel)) {
 		DRM_ERROR("failed to get clock 'sclk_pixel'\n");
 		goto fail;
 	}
 	res->sclk_hdmiphy = clk_get(dev, "sclk_hdmiphy");
-	if (IS_ERR_OR_NULL(res->sclk_hdmiphy)) {
+	if (IS_ERR(res->sclk_hdmiphy)) {
 		DRM_ERROR("failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
 	res->hdmiphy = clk_get(dev, "hdmiphy");
-	if (IS_ERR_OR_NULL(res->hdmiphy)) {
+	if (IS_ERR(res->hdmiphy)) {
 		DRM_ERROR("failed to get clock 'hdmiphy'\n");
 		goto fail;
 	}
-- 
1.7.9.5

[PATCH RESEND 4/6] clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media at vger.kernel.org
---
 drivers/media/platform/s5p-tv/hdmi_drv.c  |   10 +++++-----
 drivers/media/platform/s5p-tv/mixer_drv.c |   10 +++++-----
 drivers/media/platform/s5p-tv/sdo_drv.c   |   10 +++++-----
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/s5p-tv/hdmi_drv.c b/drivers/media/platform/s5p-tv/hdmi_drv.c
index 8a9cf43..1c48ca5 100644
--- a/drivers/media/platform/s5p-tv/hdmi_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmi_drv.c
@@ -781,27 +781,27 @@ static int hdmi_resources_init(struct hdmi_device *hdev)
 	/* get clocks, power */
 
 	res->hdmi = clk_get(dev, "hdmi");
-	if (IS_ERR_OR_NULL(res->hdmi)) {
+	if (IS_ERR(res->hdmi)) {
 		dev_err(dev, "failed to get clock 'hdmi'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_pixel = clk_get(dev, "sclk_pixel");
-	if (IS_ERR_OR_NULL(res->sclk_pixel)) {
+	if (IS_ERR(res->sclk_pixel)) {
 		dev_err(dev, "failed to get clock 'sclk_pixel'\n");
 		goto fail;
 	}
 	res->sclk_hdmiphy = clk_get(dev, "sclk_hdmiphy");
-	if (IS_ERR_OR_NULL(res->sclk_hdmiphy)) {
+	if (IS_ERR(res->sclk_hdmiphy)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
 	res->hdmiphy = clk_get(dev, "hdmiphy");
-	if (IS_ERR_OR_NULL(res->hdmiphy)) {
+	if (IS_ERR(res->hdmiphy)) {
 		dev_err(dev, "failed to get clock 'hdmiphy'\n");
 		goto fail;
 	}
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c b/drivers/media/platform/s5p-tv/mixer_drv.c
index ca0f297..c1b2e0e 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -240,27 +240,27 @@ static int mxr_acquire_clocks(struct mxr_device *mdev)
 	struct device *dev = mdev->dev;
 
 	res->mixer = clk_get(dev, "mixer");
-	if (IS_ERR_OR_NULL(res->mixer)) {
+	if (IS_ERR(res->mixer)) {
 		mxr_err(mdev, "failed to get clock 'mixer'\n");
 		goto fail;
 	}
 	res->vp = clk_get(dev, "vp");
-	if (IS_ERR_OR_NULL(res->vp)) {
+	if (IS_ERR(res->vp)) {
 		mxr_err(mdev, "failed to get clock 'vp'\n");
 		goto fail;
 	}
 	res->sclk_mixer = clk_get(dev, "sclk_mixer");
-	if (IS_ERR_OR_NULL(res->sclk_mixer)) {
+	if (IS_ERR(res->sclk_mixer)) {
 		mxr_err(mdev, "failed to get clock 'sclk_mixer'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		mxr_err(mdev, "failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(res->sclk_dac)) {
+	if (IS_ERR(res->sclk_dac)) {
 		mxr_err(mdev, "failed to get clock 'sclk_dac'\n");
 		goto fail;
 	}
diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
index ad68bbe..269d246 100644
--- a/drivers/media/platform/s5p-tv/sdo_drv.c
+++ b/drivers/media/platform/s5p-tv/sdo_drv.c
@@ -341,25 +341,25 @@ static int __devinit sdo_probe(struct platform_device *pdev)
 
 	/* acquire clocks */
 	sdev->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(sdev->sclk_dac)) {
+	if (IS_ERR(sdev->sclk_dac)) {
 		dev_err(dev, "failed to get clock 'sclk_dac'\n");
 		ret = -ENXIO;
 		goto fail;
 	}
 	sdev->dac = clk_get(dev, "dac");
-	if (IS_ERR_OR_NULL(sdev->dac)) {
+	if (IS_ERR(sdev->dac)) {
 		dev_err(dev, "failed to get clock 'dac'\n");
 		ret = -ENXIO;
 		goto fail_sclk_dac;
 	}
 	sdev->dacphy = clk_get(dev, "dacphy");
-	if (IS_ERR_OR_NULL(sdev->dacphy)) {
+	if (IS_ERR(sdev->dacphy)) {
 		dev_err(dev, "failed to get clock 'dacphy'\n");
 		ret = -ENXIO;
 		goto fail_dac;
 	}
 	sclk_vpll = clk_get(dev, "sclk_vpll");
-	if (IS_ERR_OR_NULL(sclk_vpll)) {
+	if (IS_ERR(sclk_vpll)) {
 		dev_err(dev, "failed to get clock 'sclk_vpll'\n");
 		ret = -ENXIO;
 		goto fail_dacphy;
@@ -367,7 +367,7 @@ static int __devinit sdo_probe(struct platform_device *pdev)
 	clk_set_parent(sdev->sclk_dac, sclk_vpll);
 	clk_put(sclk_vpll);
 	sdev->fout_vpll = clk_get(dev, "fout_vpll");
-	if (IS_ERR_OR_NULL(sdev->fout_vpll)) {
+	if (IS_ERR(sdev->fout_vpll)) {
 		dev_err(dev, "failed to get clock 'fout_vpll'\n");
 		goto fail_dacphy;
 	}
-- 
1.7.9.5

[PATCH RESEND 5/6] clk: s5p-fimc: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media at vger.kernel.org
---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 0531ab7..3ac4da2 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -730,7 +730,7 @@ static int fimc_md_get_clocks(struct fimc_md *fmd)
 	for (i = 0; i < FIMC_MAX_CAMCLKS; i++) {
 		snprintf(clk_name, sizeof(clk_name), "sclk_cam%u", i);
 		clock = clk_get(NULL, clk_name);
-		if (IS_ERR_OR_NULL(clock)) {
+		if (IS_ERR(clock)) {
 			v4l2_err(&fmd->v4l2_dev, "Failed to get clock: %s",
 				  clk_name);
 			return -ENXIO;
-- 
1.7.9.5

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media at vger.kernel.org
---
 drivers/media/platform/s5p-g2d/g2d.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 1bfbc32..dcd5335 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -715,7 +715,7 @@ static int g2d_probe(struct platform_device *pdev)
 	}
 
 	dev->clk = clk_get(&pdev->dev, "sclk_fimg2d");
-	if (IS_ERR_OR_NULL(dev->clk)) {
+	if (IS_ERR(dev->clk)) {
 		dev_err(&pdev->dev, "failed to get g2d clock\n");
 		return -ENXIO;
 	}
@@ -727,7 +727,7 @@ static int g2d_probe(struct platform_device *pdev)
 	}
 
 	dev->gate = clk_get(&pdev->dev, "fimg2d");
-	if (IS_ERR_OR_NULL(dev->gate)) {
+	if (IS_ERR(dev->gate)) {
 		dev_err(&pdev->dev, "failed to get g2d clock gate\n");
 		ret = -ENXIO;
 		goto unprep_clk;
-- 
1.7.9.5

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Wed, Dec 19, 2012 at 06:34:05AM +1300, Tony Prisk wrote:
> Resend to include mailing lists.
> 
> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
> 

The original code is correct.  clk_get() can return NULL depending
on the .config.

regards,
dan carpenter

[PATCH RESEND 0/6] Remove incorrect usage of IS_ERR_OR_NULL on clk_get

[Dan Carpenter]

On Wed, Dec 19, 2012 at 06:34:02AM +1300, Tony Prisk wrote:
> Resend to include mailing lists.
> 
> As per the kernel docs, clk_get() cannot return NULL.
> 

It returns NULL if CONFIG_HAVE_CLK is disabled.

regards,
dan carpenter

[PATCH RESEND 2/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Wed, Dec 19, 2012 at 06:34:04AM +1300, Tony Prisk wrote:
> Resend to include mailing lists.

These kind of comments should go under the Signed of by line under
a "---" line.  They will be removed by git-am instead of being
preserved in the git log.

Signed-off-by bla bla blah
---
Commments...

<patch>

regards,
dan carpenter

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

On Tue, 2012-12-18 at 21:39 +0300, Dan Carpenter wrote:
> On Wed, Dec 19, 2012 at 06:34:05AM +1300, Tony Prisk wrote:
> > Resend to include mailing lists.
> > 
> > Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
> > 
> 
> The original code is correct.  clk_get() can return NULL depending
> on the .config.
> 
> regards,
> dan carpenter

Thanks for than Dan,

Arguably that seems like an incorrect behaviour on the part of the clock
subsystem given that the 'proper' function has kernel doc:

* Returns a struct clk corresponding to the clock producer, or
* valid IS_ERR() condition containing errno.

Therefore the 'empty' version should adhere to the same rules, and not
return NULL.

I've cc'd Mike Turquette as well for his thoughts.

Regards
Tony P

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

I don't care either way, but being different from the documentation
is less bad than crashing which is what your patch does.  Please
be more careful in the future.

regards,
dan carpenter

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

On Tue, 2012-12-18 at 22:03 +0300, Dan Carpenter wrote:
> I don't care either way, but being different from the documentation
> is less bad than crashing which is what your patch does.  Please
> be more careful in the future.
> 
> regards,
> dan carpenter

Critism accepted.

Given that the driver depends on (PLAT_SAMSUNG || ARCH_MULTIPLATFORM),
and multiplatform select COMMON_CLK and PLAT_SAMSUNG is selected only by
ARCH_S3C64XX which has HAVE_CLK I didn't see a problem here.

Your point is still valid and I will sort it out with Mike first.

Regards
Tony P

[PATCH RESEND 3/6] clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

On Wed, 2012-12-19 at 08:11 +1300, Tony Prisk wrote:
> On Tue, 2012-12-18 at 22:03 +0300, Dan Carpenter wrote:
> > I don't care either way, but being different from the documentation
> > is less bad than crashing which is what your patch does.  Please
> > be more careful in the future.
> > 
> > regards,
> > dan carpenter
> 
> Critism accepted.
> 
> Given that the driver depends on (PLAT_SAMSUNG || ARCH_MULTIPLATFORM),
> and multiplatform select COMMON_CLK and PLAT_SAMSUNG is selected only by
> ARCH_S3C64XX which has HAVE_CLK I didn't see a problem here.
> 
> Your point is still valid and I will sort it out with Mike first.
> 
> Regards
> Tony P

Actually, I was wrong here - PLAT_SAMSUNG is selectable via

PLAT_S3C24XX || ARCH_S3C64XX || PLAT_S5P

but all these options (or the options that select them) seem to select a
clock system as well, hence still no reason for a crash.

Regards
Tony P

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sergei Shtylyov]

Hello.

On 18-12-2012 21:34, Tony Prisk wrote:

> Resend to include mailing lists.

    Such remarks should be placed under --- tear line, not in the changelog.

> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> CC: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
> CC: linux-media at vger.kernel.org

WBR, Sergei

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 12/22/2012 10:53 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 18-12-2012 21:34, Tony Prisk wrote:
>
>> Resend to include mailing lists.
>
> Such remarks should be placed under --- tear line, not in the changelog.
>
>> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
>
>> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
>> CC: Kyungmin Park <kyungmin.park@samsung.com>
>> CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> CC: linux-media at vger.kernel.org

I've applied first version of this patch to my tree for 3.9, thanks.

[PATCH RESEND 4/6] clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 12/18/2012 06:34 PM, Tony Prisk wrote:
> Resend to include mailing lists.

In the future please put such comments after the --- tear line below.

> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
>
> Signed-off-by: Tony Prisk<linux@prisktech.co.nz>
> CC: Kyungmin Park<kyungmin.park@samsung.com>
> CC: Tomasz Stanislawski<t.stanislaws@samsung.com>
> CC: linux-media at vger.kernel.org
> ---
>   drivers/media/platform/s5p-tv/hdmi_drv.c  |   10 +++++-----
>   drivers/media/platform/s5p-tv/mixer_drv.c |   10 +++++-----
>   drivers/media/platform/s5p-tv/sdo_drv.c   |   10 +++++-----
>   3 files changed, 15 insertions(+), 15 deletions(-)

Applied, after resolving conflict with other patch that addressed
those issues in sdo_drv.c file. Thanks.

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.

I told Tony about this but everyone has been gone with end of year
holidays so it hasn't been addressed.

Tony, please fix it so people don't apply these patches until
clk_get() is updated to not return NULL.  It sucks to have to revert
patches.

regards,
dan carpenter

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> 
> I told Tony about this but everyone has been gone with end of year
> holidays so it hasn't been addressed.
> 
> Tony, please fix it so people don't apply these patches until
> clk_get() is updated to not return NULL.  It sucks to have to revert
> patches.
> 
> regards,
> dan carpenter

I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel
mailing lists, regarding the return of NULL when HAVE_CLK is undefined.

Short Answer: A return value of NULL is valid and not an error therefore
we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.

I see the obvious problem this creates, and asked this question:

If the driver can't operate with a NULL clk, it should use a
IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.


And Russell's answer:

Why should a _consumer_ of a clock care?  It is _very_ important that
people get this idea - to a consumer, the struct clk is just an opaque
cookie.  The fact that it appears to be a pointer does _not_ mean that
the driver can do any kind of dereferencing on that pointer - it should
never do so.

Thread can be viewed here:
https://lkml.org/lkml/2012/12/20/105


Regards
Tony Prisk

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Wed, 2 Jan 2013, Tony Prisk wrote:

> On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
>> clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
>>
>> I told Tony about this but everyone has been gone with end of year
>> holidays so it hasn't been addressed.
>>
>> Tony, please fix it so people don't apply these patches until
>> clk_get() is updated to not return NULL.  It sucks to have to revert
>> patches.
>>
>> regards,
>> dan carpenter
>
> I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel
> mailing lists, regarding the return of NULL when HAVE_CLK is undefined.
>
> Short Answer: A return value of NULL is valid and not an error therefore
> we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.
>
> I see the obvious problem this creates, and asked this question:
>
> If the driver can't operate with a NULL clk, it should use a
> IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
>
>
> And Russell's answer:
>
> Why should a _consumer_ of a clock care?  It is _very_ important that
> people get this idea - to a consumer, the struct clk is just an opaque
> cookie.  The fact that it appears to be a pointer does _not_ mean that
> the driver can do any kind of dereferencing on that pointer - it should
> never do so.
>
> Thread can be viewed here:
> https://lkml.org/lkml/2012/12/20/105

There are dereferences to the result of clk_get a few times.  I tried the 
following semantic patch:

@@ expression E; identifier I; @@
* E = clk_get(...) ... E->I

It gives the results shown below (- marks matched lines, not lines to 
remove).  I also tried with devm_clk_get instead of clk_get, but got 
nothing.

julia

diff -u -p /var/linuxes/linux-next/arch/sh/kernel/cpufreq.c /tmp/nothing/arch/sh/kernel/cpufreq.c
--- /var/linuxes/linux-next/arch/sh/kernel/cpufreq.c
+++ /tmp/nothing/arch/sh/kernel/cpufreq.c
@@ -117,15 +117,11 @@ static int sh_cpufreq_cpu_init(struct cp

  	dev = get_cpu_device(cpu);

-	cpuclk = clk_get(dev, "cpu_clk");
-	if (IS_ERR(cpuclk)) {
  		dev_err(dev, "couldn't get CPU clk\n");
  		return PTR_ERR(cpuclk);
  	}

-	policy->cur = policy->min = policy->max = sh_cpufreq_get(cpu);

-	freq_table = cpuclk->nr_freqs ? cpuclk->freq_table : NULL;
  	if (freq_table) {
  		int result;

diff -u -p /var/linuxes/linux-next/arch/mips/kernel/cpufreq/loongson2_cpufreq.c /tmp/nothing/arch/mips/kernel/cpufreq/loongson2_cpufreq.c
--- /var/linuxes/linux-next/arch/mips/kernel/cpufreq/loongson2_cpufreq.c
+++ /tmp/nothing/arch/mips/kernel/cpufreq/loongson2_cpufreq.c
@@ -111,13 +111,10 @@ static int loongson2_cpufreq_cpu_init(st
  	if (!cpu_online(policy->cpu))
  		return -ENODEV;

-	cpuclk = clk_get(NULL, "cpu_clk");
-	if (IS_ERR(cpuclk)) {
  		printk(KERN_ERR "cpufreq: couldn't get CPU clk\n");
  		return PTR_ERR(cpuclk);
  	}

-	cpuclk->rate = cpu_clock_freq / 1000;
  	if (!cpuclk->rate)
  		return -EINVAL;

diff -u -p /var/linuxes/linux-next/drivers/net/ethernet/ti/cpts.c /tmp/nothing/drivers/net/ethernet/ti/cpts.c
--- /var/linuxes/linux-next/drivers/net/ethernet/ti/cpts.c
+++ /tmp/nothing/drivers/net/ethernet/ti/cpts.c
@@ -241,14 +241,10 @@ static void cpts_overflow_check(struct w

  static void cpts_clk_init(struct cpts *cpts)
  {
-	cpts->refclk = clk_get(NULL, CPTS_REF_CLOCK_NAME);
-	if (IS_ERR(cpts->refclk)) {
  		pr_err("Failed to clk_get %s\n", CPTS_REF_CLOCK_NAME);
  		cpts->refclk = NULL;
  		return;
  	}
-	clk_enable(cpts->refclk);
-	cpts->freq = cpts->refclk->recalc(cpts->refclk);
  }

  static void cpts_clk_release(struct cpts *cpts)
diff -u -p /var/linuxes/linux-next/drivers/i2c/busses/i2c-sh7760.c /tmp/nothing/drivers/i2c/busses/i2c-sh7760.c
--- /var/linuxes/linux-next/drivers/i2c/busses/i2c-sh7760.c
+++ /tmp/nothing/drivers/i2c/busses/i2c-sh7760.c
@@ -397,11 +397,7 @@ static int calc_CCR(unsigned long scl_hz
  	signed char cdf, cdfm;
  	int scgd, scgdm, scgds;

-	mclk = clk_get(NULL, "peripheral_clk");
-	if (IS_ERR(mclk)) {
  		return PTR_ERR(mclk);
-	} else {
-		mck = mclk->rate;
  		clk_put(mclk);
  	}

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> 
> I told Tony about this but everyone has been gone with end of year
> holidays so it hasn't been addressed.
> 
> Tony, please fix it so people don't apply these patches until
> clk_get() is updated to not return NULL.  It sucks to have to revert
> patches.

How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't
be used for?

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 08:29:57AM +0100, Julia Lawall wrote:
> There are dereferences to the result of clk_get a few times.  I tried the 
> following semantic patch:

And those are buggy; struct clk is _SUPPOSED_ to be an OPAQUE COOKIE
and no one other than the clk code should be dereferencing it.

I guess this is the danger of the common clk API - we now have a struct
clk that is exposed in a linux/*.h include which anyone can include, and
now anyone can get a definition for a struct clk and dereference it,
destroying opaqueness of this cookie.

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Wed, 2 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> >
> > I told Tony about this but everyone has been gone with end of year
> > holidays so it hasn't been addressed.
> >
> > Tony, please fix it so people don't apply these patches until
> > clk_get() is updated to not return NULL.  It sucks to have to revert
> > patches.
>
> How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't
> be used for?

Perhaps the cases where clk_get returns NULL could have a comment
indicating that NULL does not represent a failure?

In 3.7.1, it looks like it might have been possible for NULL to be
returned by clk_get in arch/mips/loongson1/common/clock.c, but that
definition seems to be gone in a recent linux-next.  The remaining
definitions look OK.

julia

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 10:44:32AM +0100, Julia Lawall wrote:
> On Wed, 2 Jan 2013, Russell King - ARM Linux wrote:
> 
> > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> > >
> > > I told Tony about this but everyone has been gone with end of year
> > > holidays so it hasn't been addressed.
> > >
> > > Tony, please fix it so people don't apply these patches until
> > > clk_get() is updated to not return NULL.  It sucks to have to revert
> > > patches.
> >
> > How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't
> > be used for?
> 
> Perhaps the cases where clk_get returns NULL could have a comment
> indicating that NULL does not represent a failure?

No.  More documentation is never the answer to people not reading
existing documentation.

> In 3.7.1, it looks like it might have been possible for NULL to be
> returned by clk_get in arch/mips/loongson1/common/clock.c, but that
> definition seems to be gone in a recent linux-next.  The remaining
> definitions look OK.

How about people just read the API and comply with it rather than
doing their own thing all the time?  We've already had at least one
instance where someone has tried using IS_ERR() with the ioremap()
return value.

Really, if you're going to program kernel space, it is very important
that you *know* the interfaces that you're using and you comply with
them.  Otherwise, you have no business saying that you're a kernel
programmer.

Yes, the odd mistake happens, but that's no excuse for the constant
blatent mistakes with stuff like IS_ERR_OR_NULL() with clk_get()
which just comes from total laziness on the part of the coder to
understand the interfaces being used.  Hell, it's even documented
in linux/clk.h - that just shows how many people read the
documentation which has been around since the clk API came about.

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 01/02/2013 06:10 AM, Dan Carpenter wrote:
> clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.

It's not a problem for this driver, as it never dereferences what's
returned from clk_get(). It would have to include <plat/clock.h>, which
it doesn't and which would have clearly indicated abuse of the clock API.

Moreover, this driver now depends on architectures that select HAVE_CLK,
so it couldn't be build when CONFIG_HAVE_CLK is disabled.

> I told Tony about this but everyone has been gone with end of year
> holidays so it hasn't been addressed.
>
> Tony, please fix it so people don't apply these patches until
> clk_get() is updated to not return NULL.  It sucks to have to revert
> patches.

As explained by Russell many times, the clock API users should not care
whether the value returned from clk_get() is NULL or not. It should only
be tested with IS_ERR(). The patches look fine to me, no need to do
anything.

---

Regards,
Sylwester

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> > 
> > I told Tony about this but everyone has been gone with end of year
> > holidays so it hasn't been addressed.
> > 
> > Tony, please fix it so people don't apply these patches until
> > clk_get() is updated to not return NULL.  It sucks to have to revert
> > patches.
> > 
> > regards,
> > dan carpenter
> 
> I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel
> mailing lists, regarding the return of NULL when HAVE_CLK is undefined.
> 
> Short Answer: A return value of NULL is valid and not an error therefore
> we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.
> 
> I see the obvious problem this creates, and asked this question:
> 
> If the driver can't operate with a NULL clk, it should use a
> IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
> 
> 
> And Russell's answer:
> 
> Why should a _consumer_ of a clock care?  It is _very_ important that
> people get this idea - to a consumer, the struct clk is just an opaque
> cookie.  The fact that it appears to be a pointer does _not_ mean that
> the driver can do any kind of dereferencing on that pointer - it should
> never do so.
> 
> Thread can be viewed here:
> https://lkml.org/lkml/2012/12/20/105
> 

Ah.  Grand.  Thanks...

Btw. The documentation for clk_get() really should include some of
this information.  I know Russell thinks that the driver authors are
stupid and lazy, and it's probably true.  But if everyone makes the
same mistake over and over, then it probably means we could put a
special note:

"Do not check this with IS_ERR_OR_NULL().  Null values are not an
error.  Drivers should treat the return value as an opaque cookie
and they should not dereference it."

This is probably there in the file somewhere else, but I searched
for "opaque", "cookie", and "dereference" and I didn't find
anything.  I'm not saying the documentation isn't perfect, just that
driver authors are lazy and stupid but we can't kill them so we have
to live with them.

regards,
dan carpenter

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Thu, 3 Jan 2013, Dan Carpenter wrote:

> On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> > >
> > > I told Tony about this but everyone has been gone with end of year
> > > holidays so it hasn't been addressed.
> > >
> > > Tony, please fix it so people don't apply these patches until
> > > clk_get() is updated to not return NULL.  It sucks to have to revert
> > > patches.
> > >
> > > regards,
> > > dan carpenter
> >
> > I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel
> > mailing lists, regarding the return of NULL when HAVE_CLK is undefined.
> >
> > Short Answer: A return value of NULL is valid and not an error therefore
> > we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.
> >
> > I see the obvious problem this creates, and asked this question:
> >
> > If the driver can't operate with a NULL clk, it should use a
> > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
> >
> >
> > And Russell's answer:
> >
> > Why should a _consumer_ of a clock care?  It is _very_ important that
> > people get this idea - to a consumer, the struct clk is just an opaque
> > cookie.  The fact that it appears to be a pointer does _not_ mean that
> > the driver can do any kind of dereferencing on that pointer - it should
> > never do so.
> >
> > Thread can be viewed here:
> > https://lkml.org/lkml/2012/12/20/105
> >
>
> Ah.  Grand.  Thanks...
>
> Btw. The documentation for clk_get() really should include some of
> this information.  I know Russell thinks that the driver authors are
> stupid and lazy, and it's probably true.  But if everyone makes the
> same mistake over and over, then it probably means we could put a
> special note:
>
> "Do not check this with IS_ERR_OR_NULL().  Null values are not an
> error.  Drivers should treat the return value as an opaque cookie
> and they should not dereference it."
>
> This is probably there in the file somewhere else, but I searched
> for "opaque", "cookie", and "dereference" and I didn't find
> anything.  I'm not saying the documentation isn't perfect, just that
> driver authors are lazy and stupid but we can't kill them so we have
> to live with them.

I still think it would also be helpful for the definition that returns
NULL to have some documentation associated with it.  Having a feature
disabled and then trying to use the feature could reasonably considered to
lead to a failure, so it is not obvious what the NULL represents.

julia

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 12:05:20PM +0300, Dan Carpenter wrote:
> On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > Why should a _consumer_ of a clock care?  It is _very_ important that
> > people get this idea - to a consumer, the struct clk is just an opaque
> > cookie.  The fact that it appears to be a pointer does _not_ mean that
> > the driver can do any kind of dereferencing on that pointer - it should
> > never do so.
> > 
> > Thread can be viewed here:
> > https://lkml.org/lkml/2012/12/20/105
> > 
> 
> Ah.  Grand.  Thanks...
> 
> Btw. The documentation for clk_get() really should include some of
> this information.

It *does* contain this information.  The problem is that driver authors
_ARE_ stupid, lazy morons who don't bother to read documentation.

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.  The implementation
 * uses @dev and @id to determine the clock consumer, and thereby
 * the clock producer.  (IOW, @id may be identical strings, but
 * clk_get may return different clock producers depending on @dev.)
 *
 * Drivers must assume that the clock source is not enabled.
 *
 * clk_get should not be called from within interrupt context.
 */

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 10:14:13AM +0100, Julia Lawall wrote:
> On Thu, 3 Jan 2013, Dan Carpenter wrote:
> 
> > On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > > > clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.
> > > >
> > > > I told Tony about this but everyone has been gone with end of year
> > > > holidays so it hasn't been addressed.
> > > >
> > > > Tony, please fix it so people don't apply these patches until
> > > > clk_get() is updated to not return NULL.  It sucks to have to revert
> > > > patches.
> > > >
> > > > regards,
> > > > dan carpenter
> > >
> > > I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel
> > > mailing lists, regarding the return of NULL when HAVE_CLK is undefined.
> > >
> > > Short Answer: A return value of NULL is valid and not an error therefore
> > > we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.
> > >
> > > I see the obvious problem this creates, and asked this question:
> > >
> > > If the driver can't operate with a NULL clk, it should use a
> > > IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.
> > >
> > >
> > > And Russell's answer:
> > >
> > > Why should a _consumer_ of a clock care?  It is _very_ important that
> > > people get this idea - to a consumer, the struct clk is just an opaque
> > > cookie.  The fact that it appears to be a pointer does _not_ mean that
> > > the driver can do any kind of dereferencing on that pointer - it should
> > > never do so.
> > >
> > > Thread can be viewed here:
> > > https://lkml.org/lkml/2012/12/20/105
> > >
> >
> > Ah.  Grand.  Thanks...
> >
> > Btw. The documentation for clk_get() really should include some of
> > this information.  I know Russell thinks that the driver authors are
> > stupid and lazy, and it's probably true.  But if everyone makes the
> > same mistake over and over, then it probably means we could put a
> > special note:
> >
> > "Do not check this with IS_ERR_OR_NULL().  Null values are not an
> > error.  Drivers should treat the return value as an opaque cookie
> > and they should not dereference it."
> >
> > This is probably there in the file somewhere else, but I searched
> > for "opaque", "cookie", and "dereference" and I didn't find
> > anything.  I'm not saying the documentation isn't perfect, just that
> > driver authors are lazy and stupid but we can't kill them so we have
> > to live with them.
> 
> I still think it would also be helpful for the definition that returns
> NULL to have some documentation associated with it.  Having a feature
> disabled and then trying to use the feature could reasonably considered to
> lead to a failure, so it is not obvious what the NULL represents.

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.  The implementation
 * uses @dev and @id to determine the clock consumer, and thereby
 * the clock producer.  (IOW, @id may be identical strings, but
 * clk_get may return different clock producers depending on @dev.)
 *
 * Drivers must assume that the clock source is not enabled.
 *
 * clk_get should not be called from within interrupt context.
 */

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

Come on...  Don't say we haven't read comment.  Obviously, the first
thing we did was read that comment.  I've read it many times at this
point and I still think we should add in a bit which says:

"NOTE:  Drivers should treat the return value as an opaque cookie
and not dereference it.  NULL returns don't imply an error so don't
use IS_ERR_OR_NULL() to check for errors."

regards,
dan carpenter

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 02:10:40PM +0300, Dan Carpenter wrote:
> Come on...  Don't say we haven't read comment.  Obviously, the first
> thing we did was read that comment.  I've read it many times at this
> point and I still think we should add in a bit which says:

So where does it give you in that comment permission to treat NULL any
differently to any other non-IS_ERR() return value?

It is very clear: values where IS_ERR() is true are considered errors.
Everything else is considered valid.

> "NOTE:  Drivers should treat the return value as an opaque cookie
> and not dereference it.  NULL returns don't imply an error so don't
> use IS_ERR_OR_NULL() to check for errors."

No.  The one thing I've learnt through maintaining www.arm.linux.org.uk
is that the more of these kinds of "lets add to documentation" suggestions
you get, the more _unclear_ the documentation becomes, and the more it is
open to bad interpretation, and the more suggestions to add more words you
receive.

Concise documentation is the only way to go.  And what we have there today
is concise and to the point.  It specifies it very clearly:

 * Returns a struct clk corresponding to the clock producer, or
 * valid IS_ERR() condition containing errno.

That one sentence gives you all the information you need about it's return
value.  It gives you two choices.  (1) a return value where IS_ERR() is
true, which is an error, and (2) a return value where IS_ERR() is false,
which is a valid cookie.

Maybe you don't realise, but IS_ERR(NULL) is false.  Therefore, this falls
into category (2).

You can't get clearer than that, unless you don't understand the IS_ERR()
and associated macro.

Moreover, it tells you the function to use to check the return value for
errors.  IS_ERR().  It doesn't say IS_ERR_OR_NULL(), it says IS_ERR().

All it takes is for people to engage their grey cells and read the
documentation as it stands, rather than trying to weasel their way around
it and invent crap that it doesn't say.

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote:
> Maybe you don't realise, but IS_ERR(NULL) is false.  Therefore, this falls
> into category (2).

No, obviously, I know the difference between IS_ERR() and
IS_ERR_OR_NULL().  That's how we started this thread.

*shrug*.

regards,
dan carpenter

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 04:45:54PM +0300, Dan Carpenter wrote:
> On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote:
> > Maybe you don't realise, but IS_ERR(NULL) is false.  Therefore, this falls
> > into category (2).
> 
> No, obviously, I know the difference between IS_ERR() and
> IS_ERR_OR_NULL().  That's how we started this thread.

Well, if you know that, then how is the documentation anything _but_
clear?