From: Ming Lei <[email protected]>
drivers/mtd/nand/omap2.c: In function 'omap_wait':
drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this function)
drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported only once
drivers/mtd/nand/omap2.c:543: error: for each function it appears in.)
drivers/mtd/nand/omap2.c:557: error: implicit declaration of function 'time_before'
......
drivers/mtd/nand/omap2.c: In function 'omap_wait':
drivers/mtd/nand/omap2.c:545: warning: 'status' may be used uninitialized in this function
Signed-off-by: Ming Lei <[email protected]>
---
drivers/mtd/nand/omap2.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0cd76f8..2905c79 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -15,6 +15,7 @@
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
#include <linux/io.h>
+#include <linux/jiffies.h>
#include <asm/dma.h>
@@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
unsigned long timeo = jiffies;
- int status, state = this->state;
+ int state = this->state;
+ int status = 0;
if (state == FL_ERASING)
timeo += (HZ * 400) / 1000;
--
1.6.0.GIT
On Fri, 2009-06-26 at 21:21 +0800, [email protected] wrote:
> From: Ming Lei <[email protected]>
>
> drivers/mtd/nand/omap2.c: In function 'omap_wait':
> drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this function)
> drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported only once
> drivers/mtd/nand/omap2.c:543: error: for each function it appears in.)
> drivers/mtd/nand/omap2.c:557: error: implicit declaration of function 'time_before'
OK.
> ......
>
> drivers/mtd/nand/omap2.c: In function 'omap_wait':
> drivers/mtd/nand/omap2.c:545: warning: 'status' may be used uninitialized in this function
Hm, why do you set it to zero? In the (fairly unlikely) case where you
never go through the loop at all, because you're preempted until the
timeout has completed, what value do you want it to return?
Don't just make changes by bashing on the keyboard until the compiler
shuts up; please apply brain.
That way, I think you'll notice that the return value from this function
is completely screwed anyway.... Vimal? Shouldn't it be returning
NAND_STATUS_FAIL in the failure case, not a value read directly from the
hardware register? And shouldn't there be a cpu_relax() (or even
schedule) in there too?
> Signed-off-by: Ming Lei <[email protected]>
> ---
> drivers/mtd/nand/omap2.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 0cd76f8..2905c79 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -15,6 +15,7 @@
> #include <linux/mtd/nand.h>
> #include <linux/mtd/partitions.h>
> #include <linux/io.h>
> +#include <linux/jiffies.h>
>
> #include <asm/dma.h>
>
> @@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> mtd);
> unsigned long timeo = jiffies;
> - int status, state = this->state;
> + int state = this->state;
> + int status = 0;
>
> if (state == FL_ERASING)
> timeo += (HZ * 400) / 1000;
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
On Fri, Jun 26, 2009 at 8:39 PM, David Woodhouse<[email protected]> wrote:
> On Fri, 2009-06-26 at 21:21 +0800, [email protected] wrote:
>> From: Ming Lei <[email protected]>
>>
>> drivers/mtd/nand/omap2.c: In function 'omap_wait':
>> drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this function)
>> drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported only once
>> drivers/mtd/nand/omap2.c:543: error: for each function it appears in.)
>> drivers/mtd/nand/omap2.c:557: error: implicit declaration of function 'time_before'
>
> OK.
>
>> ......
>>
>> drivers/mtd/nand/omap2.c: In function 'omap_wait':
>> drivers/mtd/nand/omap2.c:545: warning: 'status' may be used uninitialized in this function
>
> Hm, why do you set it to zero? In the (fairly unlikely) case where you
> never go through the loop at all, because you're preempted until the
> timeout has completed, what value do you want it to return?
>
> Don't just make changes by bashing on the keyboard until the compiler
> shuts up; please apply brain.
>
> That way, I think you'll notice that the return value from this function
> is completely screwed anyway.... Vimal? Shouldn't it be returning
> NAND_STATUS_FAIL in the failure case, not a value read directly from the
> hardware register? And shouldn't there be a cpu_relax() (or even
> schedule) in there too?
That's correct David. I think 'cond_resched()' would be fine here.
>
>> Signed-off-by: Ming Lei <[email protected]>
>> ---
>> drivers/mtd/nand/omap2.c | 4 +++-
>> 1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
>> index 0cd76f8..2905c79 100644
>> --- a/drivers/mtd/nand/omap2.c
>> +++ b/drivers/mtd/nand/omap2.c
>> @@ -15,6 +15,7 @@
>> #include <linux/mtd/nand.h>
>> #include <linux/mtd/partitions.h>
>> #include <linux/io.h>
>> +#include <linux/jiffies.h>
>>
>> #include <asm/dma.h>
>>
>> @@ -541,7 +542,8 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip *chip)
>> struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
>> mtd);
>> unsigned long timeo = jiffies;
>> - int status, state = this->state;
>> + int state = this->state;
>> + int status = 0;
>>
>> if (state == FL_ERASING)
>> timeo += (HZ * 400) / 1000;
> --
> David Woodhouse Open Source Technology Centre
> [email protected] Intel Corporation
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
---
Vimal-
On Fri, 2009-06-26 at 21:15 +0530, Singh, Vimal wrote:
>
> That's correct David. I think 'cond_resched()' would be fine here.
Got a patch? I have you down as the person responsible for this driver,
given that [email protected] is bouncing...
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
Somehow I do not see below errors on compilation:
drivers/mtd/nand/omap2.c: In function 'omap_wait':
drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this
function)
drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported
only once
drivers/mtd/nand/omap2.c:543: error: for each function it appears in.)
drivers/mtd/nand/omap2.c:557: error: implicit declaration of function
'time_before'
But still including 'linux/jiffies.h'.
Below is the patch.
Signed-off-by: Vimal Singh <[email protected]>
Signed-off-by: Ming Lei <[email protected]>
---
drivers/mtd/nand/omap2.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0cd76f8..3b6e6ad 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -11,6 +11,8 @@
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
@@ -541,7 +543,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
*chip)
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
unsigned long timeo = jiffies;
- int status, state = this->state;
+ int status = NAND_STATUS_FAIL, state = this->state;
if (state == FL_ERASING)
timeo += (HZ * 400) / 1000;
@@ -558,6 +560,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
*chip)
status = __raw_readb(this->IO_ADDR_R);
if (!(status & 0x40))
break;
+ cond_resched();
}
return status;
}
On Fri, 2009-06-26 at 22:32 +0530, vimal singh wrote:
> Somehow I do not see below errors on compilation:
> drivers/mtd/nand/omap2.c: In function 'omap_wait':
> drivers/mtd/nand/omap2.c:543: error: 'jiffies' undeclared (first use in this
> function)
> drivers/mtd/nand/omap2.c:543: error: (Each undeclared identifier is reported
> only once
> drivers/mtd/nand/omap2.c:543: error: for each function it appears in.)
> drivers/mtd/nand/omap2.c:557: error: implicit declaration of function
> 'time_before'
>
> But still including 'linux/jiffies.h'.
>
> Below is the patch.
The 'status' you return in the case that you _have_ gone through the
loop is still wrong though, isn't it?
--
dwmw2
> The 'status' you return in the case that you _have_ gone through the
> loop is still wrong though, isn't it?
Yes, I have corrected that in below patch. Its my bad, I did not fix it
previously.
Signed-off-by: Vimal Singh <vimalsingh at ti.com>
Signed-off-by: Ming Lei <tom.leiming at gmail.com>
---
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0cd76f8..3b6e6ad 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -11,6 +11,8 @@
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
@@ -541,7 +543,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
unsigned long timeo = jiffies;
- int status, state = this->state;
+ int status = NAND_STATUS_FAIL, state = this->state;
if (state == FL_ERASING)
timeo += (HZ * 400) / 1000;
@@ -558,6 +560,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
status = __raw_readb(this->IO_ADDR_R);
- if (!(status & 0x40))
+ if (status & NAND_STATUS_READY)
break;
+ cond_resched();
}
return status;
}
On Sat, 2009-06-27 at 11:07 +0530, vimal singh wrote:
> > The 'status' you return in the case that you _have_ gone through the
> > loop is still wrong though, isn't it?
> Yes, I have corrected that in below patch. Its my bad, I did not fix it
> previously.
>
> Signed-off-by: Vimal Singh <vimalsingh at ti.com>
> Signed-off-by: Ming Lei <tom.leiming at gmail.com>
Er, did he? And please put proper email addresses in. If you can't find
an @ sign on your keyboard, cut and paste it from elsewhere.
> @@ -558,6 +560,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
> status = __raw_readb(this->IO_ADDR_R);
> - if (!(status & 0x40))
> + if (status & NAND_STATUS_READY)
> break;
Hm... now you're polling the PASS/FAIL bit in the status, not the
READY/BUSY bit. I suspect that's not going to work too well...
--
David Woodhouse Open Source Technology Centre
[email protected] Intel Corporation
> Er, did he? And please put proper email addresses in. If you can't find
> an @ sign on your keyboard, cut and paste it from elsewhere.
Corrected this.
>> - if (!(status & 0x40))
>> + if (status & NAND_STATUS_READY)
>> break;
>
> Hm... now you're polling the PASS/FAIL bit in the status, not the
> READY/BUSY bit. I suspect that's not going to work too well...
>
Value of macro 'NAND_STATUS_READY' is '0x40' only. And AFAIK this is the bit
for ready/busy, isn't it?
Signed-off-by: Vimal Singh <[email protected]>
---
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 0cd76f8..3b6e6ad 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -11,6 +11,8 @@
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/sched.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/nand.h>
#include <linux/mtd/partitions.h>
@@ -541,7 +543,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
mtd);
unsigned long timeo = jiffies;
- int status, state = this->state;
+ int status = NAND_STATUS_FAIL, state = this->state;
if (state == FL_ERASING)
timeo += (HZ * 400) / 1000;
@@ -558,6 +560,7 @@ static int omap_wait(struct mtd_info *mtd, struct nand_chip
status = __raw_readb(this->IO_ADDR_R);
- if (!(status & 0x40))
+ if (status & NAND_STATUS_READY)
break;
+ cond_resched();
}
return status;
}