diff mbox

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

Message ID 1355852048-23188-4-git-send-email-linux@prisktech.co.nz (mailing list archive)
State New, archived
Headers show

Commit Message

Tony Prisk Dec. 18, 2012, 5:34 p.m. UTC
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@lists.freedesktop.org
---
 drivers/gpu/drm/exynos/exynos_hdmi.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Dan Carpenter Dec. 18, 2012, 6:39 p.m. UTC | #1
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
Tony Prisk Dec. 18, 2012, 6:52 p.m. UTC | #2
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
Dan Carpenter Dec. 18, 2012, 7:03 p.m. UTC | #3
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
Tony Prisk Dec. 18, 2012, 7:11 p.m. UTC | #4
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
Tony Prisk Dec. 18, 2012, 7:39 p.m. UTC | #5
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
diff mbox

Patch

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;
 	}