diff mbox

[3/3] video: fbdev: imxfb: add some error handling

Message ID 1457380425-20244-4-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Uwe Kleine-König March 7, 2016, 7:53 p.m. UTC 
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

Comments

Philipp Zabel March 8, 2016, 8 a.m. UTC | #1 
Am Montag, den 07.03.2016, 20:53 +0100 schrieb Uwe Kleine-König:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 3dd2824e6773..671b3719db56 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -473,8 +473,9 @@ static int imxfb_set_par(struct fb_info *info)
>  	return 0;
>  }
>  
> -static void imxfb_enable_controller(struct imxfb_info *fbi)
> +static int imxfb_enable_controller(struct imxfb_info *fbi)
>  {
> +	int ret;
>  
>  	if (fbi->enabled)
>  		return;
> @@ -496,10 +497,27 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
>  	 */
>  	writel(RMCR_LCDC_EN_MX1, fbi->regs + LCDC_RMCR);
>  
> -	clk_prepare_enable(fbi->clk_ipg);
> -	clk_prepare_enable(fbi->clk_ahb);
> -	clk_prepare_enable(fbi->clk_per);
> +	ret = clk_prepare_enable(fbi->clk_ipg);
> +	if (ret)
> +		goto err_enable_ipg;
> +
> +	ret = clk_prepare_enable(fbi->clk_ahb);
> +	if (ret)
> +		goto err_enable_ahb;
> +
> +	ret = clk_prepare_enable(fbi->clk_per);
> +	if (ret) {
> +		clk_disable_unprepare(fbi->clk_ahb);
> +err_enable_ahb:
> +		clk_disable_unprepare(fbi->clk_ipg);
> +err_enable_ipg:
> +		writel(0, fbi->regs + LCDC_RMCR);
> +
> +		return ret;
> +	}
> +
>  	fbi->enabled = true;
> +	return 0;
>  }
>  
>  static void imxfb_disable_controller(struct imxfb_info *fbi)
> @@ -510,8 +528,8 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  	pr_debug("Disabling LCD controller\n");
>  
>  	clk_disable_unprepare(fbi->clk_per);
> -	clk_disable_unprepare(fbi->clk_ipg);
>  	clk_disable_unprepare(fbi->clk_ahb);
> +	clk_disable_unprepare(fbi->clk_ipg);
>  	fbi->enabled = false;
>  
>  	writel(0, fbi->regs + LCDC_RMCR);
> @@ -520,6 +538,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  static int imxfb_blank(int blank, struct fb_info *info)
>  {
>  	struct imxfb_info *fbi = info->par;
> +	int ret = 0;;

s/;;/;/

>  
>  	pr_debug("imxfb_blank: blank=%d\n", blank);
>  
> @@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)
>  		break;
>  
>  	case FB_BLANK_UNBLANK:
> -		imxfb_enable_controller(fbi);
> +		ret = imxfb_enable_controller(fbi);
>  		break;

or this could be simplified to:
		return imxfb_enable_controller(fbi);

>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  static struct fb_ops imxfb_ops = {

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp
Tomi Valkeinen March 11, 2016, 11:26 a.m. UTC | #2 
On 07/03/16 21:53, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Always have a description in a patch. In trivial cases it can be more or
less the same as the subject.

> ---
>  drivers/video/fbdev/imxfb.c | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
> index 3dd2824e6773..671b3719db56 100644
> --- a/drivers/video/fbdev/imxfb.c
> +++ b/drivers/video/fbdev/imxfb.c
> @@ -473,8 +473,9 @@ static int imxfb_set_par(struct fb_info *info)
>  	return 0;
>  }
>  
> -static void imxfb_enable_controller(struct imxfb_info *fbi)
> +static int imxfb_enable_controller(struct imxfb_info *fbi)
>  {
> +	int ret;
>  
>  	if (fbi->enabled)
>  		return;
> @@ -496,10 +497,27 @@ static void imxfb_enable_controller(struct imxfb_info *fbi)
>  	 */
>  	writel(RMCR_LCDC_EN_MX1, fbi->regs + LCDC_RMCR);
>  
> -	clk_prepare_enable(fbi->clk_ipg);
> -	clk_prepare_enable(fbi->clk_ahb);
> -	clk_prepare_enable(fbi->clk_per);
> +	ret = clk_prepare_enable(fbi->clk_ipg);
> +	if (ret)
> +		goto err_enable_ipg;
> +
> +	ret = clk_prepare_enable(fbi->clk_ahb);
> +	if (ret)
> +		goto err_enable_ahb;
> +
> +	ret = clk_prepare_enable(fbi->clk_per);
> +	if (ret) {
> +		clk_disable_unprepare(fbi->clk_ahb);
> +err_enable_ahb:
> +		clk_disable_unprepare(fbi->clk_ipg);
> +err_enable_ipg:
> +		writel(0, fbi->regs + LCDC_RMCR);

Please don't do that =). If you use goto, have the labels at the end of
the function, not in the middle inside a if() block...

> +
> +		return ret;
> +	}
> +
>  	fbi->enabled = true;
> +	return 0;
>  }
>  
>  static void imxfb_disable_controller(struct imxfb_info *fbi)
> @@ -510,8 +528,8 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  	pr_debug("Disabling LCD controller\n");
>  
>  	clk_disable_unprepare(fbi->clk_per);
> -	clk_disable_unprepare(fbi->clk_ipg);
>  	clk_disable_unprepare(fbi->clk_ahb);
> +	clk_disable_unprepare(fbi->clk_ipg);

This is not error handling. I don't mind it being in the same patch, but
this change is something to mention in the description.

>  	fbi->enabled = false;
>  
>  	writel(0, fbi->regs + LCDC_RMCR);
> @@ -520,6 +538,7 @@ static void imxfb_disable_controller(struct imxfb_info *fbi)
>  static int imxfb_blank(int blank, struct fb_info *info)
>  {
>  	struct imxfb_info *fbi = info->par;
> +	int ret = 0;;

Extra ;.

>  
>  	pr_debug("imxfb_blank: blank=%d\n", blank);
>  
> @@ -532,10 +551,10 @@ static int imxfb_blank(int blank, struct fb_info *info)
>  		break;
>  
>  	case FB_BLANK_UNBLANK:
> -		imxfb_enable_controller(fbi);
> +		ret = imxfb_enable_controller(fbi);
>  		break;
>  	}
> -	return 0;
> +	return ret;
>  }
>  
>  static struct fb_ops imxfb_ops = {
>
diff mbox

Patch

diff --git a/drivers/video/fbdev/imxfb.c b/drivers/video/fbdev/imxfb.c
index 3dd2824e6773..671b3719db56 100644
--- a/drivers/video/fbdev/imxfb.c
+++ b/drivers/video/fbdev/imxfb.c
@@ -473,8 +473,9 @@  static int imxfb_set_par(struct fb_info *info)
 	return 0;
 }
 
-static void imxfb_enable_controller(struct imxfb_info *fbi)
+static int imxfb_enable_controller(struct imxfb_info *fbi)
 {
+	int ret;
 
 	if (fbi->enabled)
 		return;
@@ -496,10 +497,27 @@  static void imxfb_enable_controller(struct imxfb_info *fbi)
 	 */
 	writel(RMCR_LCDC_EN_MX1, fbi->regs + LCDC_RMCR);
 
-	clk_prepare_enable(fbi->clk_ipg);
-	clk_prepare_enable(fbi->clk_ahb);
-	clk_prepare_enable(fbi->clk_per);
+	ret = clk_prepare_enable(fbi->clk_ipg);
+	if (ret)
+		goto err_enable_ipg;
+
+	ret = clk_prepare_enable(fbi->clk_ahb);
+	if (ret)
+		goto err_enable_ahb;
+
+	ret = clk_prepare_enable(fbi->clk_per);
+	if (ret) {
+		clk_disable_unprepare(fbi->clk_ahb);
+err_enable_ahb:
+		clk_disable_unprepare(fbi->clk_ipg);
+err_enable_ipg:
+		writel(0, fbi->regs + LCDC_RMCR);
+
+		return ret;
+	}
+
 	fbi->enabled = true;
+	return 0;
 }
 
 static void imxfb_disable_controller(struct imxfb_info *fbi)
@@ -510,8 +528,8 @@  static void imxfb_disable_controller(struct imxfb_info *fbi)
 	pr_debug("Disabling LCD controller\n");
 
 	clk_disable_unprepare(fbi->clk_per);
-	clk_disable_unprepare(fbi->clk_ipg);
 	clk_disable_unprepare(fbi->clk_ahb);
+	clk_disable_unprepare(fbi->clk_ipg);
 	fbi->enabled = false;
 
 	writel(0, fbi->regs + LCDC_RMCR);
@@ -520,6 +538,7 @@  static void imxfb_disable_controller(struct imxfb_info *fbi)
 static int imxfb_blank(int blank, struct fb_info *info)
 {
 	struct imxfb_info *fbi = info->par;
+	int ret = 0;;
 
 	pr_debug("imxfb_blank: blank=%d\n", blank);
 
@@ -532,10 +551,10 @@  static int imxfb_blank(int blank, struct fb_info *info)
 		break;
 
 	case FB_BLANK_UNBLANK:
-		imxfb_enable_controller(fbi);
+		ret = imxfb_enable_controller(fbi);
 		break;
 	}
-	return 0;
+	return ret;
 }
 
 static struct fb_ops imxfb_ops = {