This is second patch in series.
In driver in_atomic we should not use to check if code is unning in IRQ.
clcdfb_sleep() function is used to give some delay between operation.
I have used schedule_timeout() function for same amount of delay.
Please review the same.
Patch 1/2 I will fix comments and re-submitt.
>From 06b275e44a3951ec0a57af59aea2a4c82595e456 Mon Sep 17 00:00:00 2001
From: Parmeshwr Prasad <parmeshwr_prasad@xxxxxxxx>
Date: Fri, 13 Feb 2015 06:10:34 -0500
Subject: [PATCH 2/2] Patch to replace in_atomic with proper function from
amba-clcd.c driver. In driver code in_atomic should not be used to check the
IRQ or kernel context. to remove that I have used schedule_timeout() with
respective delay. In this patch I have also removed printk() with respective
pr_* functions
Signed-off-by: Parmeshwr Prasad <parmeshwr_prasad@xxxxxxxx>
---
drivers/video/fbdev/amba-clcd.c | 48 +++++++++++++++++++----------------------
1 file changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 0ddc1f0..7e630f5c 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -33,8 +33,9 @@
#include <video/display_timing.h>
#include <video/of_display_timing.h>
#include <video/videomode.h>
-
-#include <asm/sizes.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
+#include <linux/sizes.h>
#define to_clcd(info) container_of(info, struct clcd_fb, fb)
@@ -42,16 +43,15 @@
static const char *clcd_name = "CLCD FB";
/*
- * Unfortunately, the enable/disable functions may be called either from
- * process or IRQ context, and we _need_ to delay. This is _not_ good.
+ * Hardware need certain time for power setting
*/
-static inline void clcdfb_sleep(unsigned int ms)
+static inline void clcdfb_sleep(signed long ms)
{
- if (in_atomic()) {
- mdelay(ms);
- } else {
- msleep(ms);
- }
+ unsigned long delay;
+
+ delay = jiffies + (HZ / 1000) * ms;
+ set_current_state(TASK_INTERRUPTIBLE);
+ schedule_timeout(delay);
}
static inline void clcdfb_set_start(struct clcd_fb *fb)
@@ -313,14 +313,13 @@ static int clcdfb_set_par(struct fb_info *info)
clcdfb_enable(fb, regs.cntl);
#ifdef DEBUG
- printk(KERN_INFO
- "CLCD: Registers set to\n"
- " %08x %08x %08x %08x\n"
- " %08x %08x %08x %08x\n",
- readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
- readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
- readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
- readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
+ pr_info("CLCD: Registers set to\n"
+ " %08x %08x %08x %08x\n"
+ " %08x %08x %08x %08x\n",
+ readl(fb->regs + CLCD_TIM0), readl(fb->regs + CLCD_TIM1),
+ readl(fb->regs + CLCD_TIM2), readl(fb->regs + CLCD_TIM3),
+ readl(fb->regs + CLCD_UBAS), readl(fb->regs + CLCD_LBAS),
+ readl(fb->regs + fb->off_ienb), readl(fb->regs + fb->off_cntl));
#endif
return 0;
@@ -392,11 +391,10 @@ static int clcdfb_blank(int blank_mode, struct fb_info
*info)
{
struct clcd_fb *fb = to_clcd(info);
- if (blank_mode != 0) {
+ if (blank_mode != 0)
clcdfb_disable(fb);
- } else {
+ else
clcdfb_enable(fb, fb->clcd_cntl);
- }
return 0;
}
@@ -465,7 +463,7 @@ static int clcdfb_register(struct clcd_fb *fb)
fb->regs = ioremap(fb->fb.fix.mmio_start, fb->fb.fix.mmio_len);
if (!fb->regs) {
- printk(KERN_ERR "CLCD: unable to remap registers\n");
+ pr_err("CLCD: unable to remap registers\n");
ret = -ENOMEM;
goto clk_unprep;
}
@@ -536,7 +534,7 @@ static int clcdfb_register(struct clcd_fb *fb)
if (ret == 0)
goto out;
- printk(KERN_ERR "CLCD: cannot register framebuffer (%d)\n", ret);
+ pr_info("CLCD: cannot register framebuffer (%d)\n", ret);
fb_dealloc_cmap(&fb->fb.cmap);
unmap:
@@ -832,14 +830,12 @@ static int clcdfb_probe(struct amba_device *dev, const
struct amba_id *id)
ret = amba_request_regions(dev, NULL);
if (ret) {
- printk(KERN_ERR "CLCD: unable to reserve regs region\n");
+ pr_err("CLCD: unable to reserve regs region\n");
goto out;
}
fb = kzalloc(sizeof(struct clcd_fb), GFP_KERNEL);
if (!fb) {
- printk(KERN_INFO
- "CLCD: could not allocate new clcd_fb struct\n");
ret = -ENOMEM;
goto free_region;
}
--
1.9.3
-Parmeshwr
On Fri, Feb 13, 2015 at 05:59:56AM -0600, Joe Perches wrote:
> On Fri, 2015-02-13 at 11:35 +0000, Russell King - ARM Linux wrote:
> > On Fri, Feb 13, 2015 at 06:21:33AM -0500, Parmeshwr Prasad wrote:
> > > @@ -288,7 +288,7 @@ static int clcdfb_set_par(struct fb_info *info)
> > > struct clcd_regs regs;
> > >
> > > fb->fb.fix.line_length = fb->fb.var.xres_virtual *
> > > - fb->fb.var.bits_per_pixel / 8;
> > > + fb->fb.var.bits_per_pixel / 8;
> >
> > NAK on this one. The code as it stood before is much clearer since
> > we align the expression with the start of it on the preceding line.
>
> I agree with all of what Russell wrote, but maybe this;
>
> > > @@ -717,7 +716,9 @@ static int clcdfb_of_vram_setup(struct clcd_fb *fb)
> > > return -ENOMEM;
> > >
> > > fb->fb.fix.smem_start = of_translate_address(memory,
> > > - of_get_address(memory, 0, &size, NULL));
> > > + of_get_address(memory, 0,
> > > + &size,
> > > + NULL));
> >
> > Thi sis the exception to the rule - where scrunching an expression so that
> > it takes multiple lines because of lack of right-hand space is not on.
> > The former version was a lot better.
>
> Perhaps this could be better as:
>
> fb->fb.fix.smem_start =
> of_translate_address(memory,
> of_get_address(memory, 0, &size, NULL));
>
> But sometimes using multiple statements instead of
> embedding function calls as arguments can be simpler
> and more intelligible for the reader.
>
> __be32 addr;
>
> ...
>
> addr = of_get_address(memory, 0, &size, NULL);
> fb->fb.fix.smem_start = of_translate_address(memory, addr);
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
