Patchwork NUC900/rtc: change the waiting for device ready implement

login
register
mail settings
Submitter Wan ZongShun
Date May 27, 2010, 6:59 a.m.
Message ID <4BFE1853.5070105@gmail.com>
Download mbox | patch
Permalink /patch/53697/
State New
Headers show

Comments

Wan ZongShun - May 27, 2010, 6:59 a.m.
Dear Andrew,

This patch is only to change the waiting for device ready implement
for winbond nuc900 platform.

Signed-off-by:Wan ZongShun<mcuos.com@gmail.com>

---
 drivers/rtc/rtc-nuc900.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
Wan ZongShun - June 1, 2010, 5:03 a.m.
Hi Andrew,

How about this patch?
Do you think this change is necessary?

在 2010年5月27日 下午2:59,Wan ZongShun <mcuos.com@gmail.com> 写道:
> Dear Andrew,
>
> This patch is only to change the waiting for device ready implement
> for winbond nuc900 platform.
>
> Signed-off-by:Wan ZongShun<mcuos.com@gmail.com>
>
> ---
>  drivers/rtc/rtc-nuc900.c |   15 +++++++--------
>  1 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/rtc/rtc-nuc900.c b/drivers/rtc/rtc-nuc900.c
> index a351bd5..21d1330 100644
> --- a/drivers/rtc/rtc-nuc900.c
> +++ b/drivers/rtc/rtc-nuc900.c
> @@ -85,22 +85,21 @@ static irqreturn_t nuc900_rtc_interrupt(int irq, void *_rtc)
>
>  static int *check_rtc_access_enable(struct nuc900_rtc *nuc900_rtc)
>  {
> -       unsigned int i;
> +       unsigned int i, timeout = 0x1000;
>        __raw_writel(INIRRESET, nuc900_rtc->rtc_reg + REG_RTC_INIR);
>
>        mdelay(10);
>
>        __raw_writel(AERPOWERON, nuc900_rtc->rtc_reg + REG_RTC_AER);
>
> -       for (i = 0; i < 1000; i++) {
> -               if (__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
> -                       return 0;
> -       }
> +       while (!(__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
> +                                                               && timeout--)
> +               mdelay(1);
>
> -       if ((__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB) == 0x0)
> -               return ERR_PTR(-ENODEV);
> +       if (!timeout)
> +               return ERR_PTR(-EPERM);
>
> -       return ERR_PTR(-EPERM);
> +       return 0;
>  }
>
>  static void nuc900_rtc_bcd2bin(unsigned int timereg,
> --
> 1.6.3.3
>
Andrew Morton - June 1, 2010, 10:43 p.m.
On Thu, 27 May 2010 14:59:31 +0800
Wan ZongShun <mcuos.com@gmail.com> wrote:

> Dear Andrew,
> 
> This patch is only to change the waiting for device ready implement
> for winbond nuc900 platform.
> 

It's not very helpful to only say "I changed it".  _why_ did you change
it?  What change did you make?  What was wrong with the old code and
what's better about the new code?

> --- a/drivers/rtc/rtc-nuc900.c
> +++ b/drivers/rtc/rtc-nuc900.c
> @@ -85,22 +85,21 @@ static irqreturn_t nuc900_rtc_interrupt(int irq, void *_rtc)
> 
>  static int *check_rtc_access_enable(struct nuc900_rtc *nuc900_rtc)
>  {
> -	unsigned int i;
> +	unsigned int i, timeout = 0x1000;
>  	__raw_writel(INIRRESET, nuc900_rtc->rtc_reg + REG_RTC_INIR);
> 
>  	mdelay(10);
> 
>  	__raw_writel(AERPOWERON, nuc900_rtc->rtc_reg + REG_RTC_AER);
> 
> -	for (i = 0; i < 1000; i++) {
> -		if (__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
> -			return 0;
> -	}
> +	while (!(__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
> +								&& timeout--)
> +		mdelay(1);
> 
> -	if ((__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB) == 0x0)
> -		return ERR_PTR(-ENODEV);
> +	if (!timeout)
> +		return ERR_PTR(-EPERM);
> 
> -	return ERR_PTR(-EPERM);
> +	return 0;
> }

I can see that the patch makes two changes: it adds an mdelay(1) to the
polling loop and it changes the return value from ENODEV to EPERM if
the loop timed out.

But I don't have the faintest idea _why_ the changes were made!


So.  I merged the patch without a changelog.  Please send a changelog
for this patch which permits others to understand the change.
Wan ZongShun - June 2, 2010, 7:06 a.m.
Hi Andrew ,

The patch makes two changes:
(1) it adds an mdelay(1) to the polling loop

Here I change the 'for' loop to 'while' loop and and add an mdelay(1),
which can make less access to the hardware register.

(2) it changes the return value from ENODEV to EPERM if the loop timed out.

I think the 'Operation not permitted' description is more suitable for
the meaning
of 'check_rtc_access_enable()' function, it just be used to judge rtc
access operation is permitted or not.



2010/6/2 Andrew Morton <akpm@linux-foundation.org>:
> On Thu, 27 May 2010 14:59:31 +0800
> Wan ZongShun <mcuos.com@gmail.com> wrote:
>
>> Dear Andrew,
>>
>> This patch is only to change the waiting for device ready implement
>> for winbond nuc900 platform.
>>
>
> It's not very helpful to only say "I changed it".  _why_ did you change
> it?  What change did you make?  What was wrong with the old code and
> what's better about the new code?
>
>> --- a/drivers/rtc/rtc-nuc900.c
>> +++ b/drivers/rtc/rtc-nuc900.c
>> @@ -85,22 +85,21 @@ static irqreturn_t nuc900_rtc_interrupt(int irq, void *_rtc)
>>
>>  static int *check_rtc_access_enable(struct nuc900_rtc *nuc900_rtc)
>>  {
>> -     unsigned int i;
>> +     unsigned int i, timeout = 0x1000;
>>       __raw_writel(INIRRESET, nuc900_rtc->rtc_reg + REG_RTC_INIR);
>>
>>       mdelay(10);
>>
>>       __raw_writel(AERPOWERON, nuc900_rtc->rtc_reg + REG_RTC_AER);
>>
>> -     for (i = 0; i < 1000; i++) {
>> -             if (__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
>> -                     return 0;
>> -     }
>> +     while (!(__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
>> +                                                             && timeout--)
>> +             mdelay(1);
>>
>> -     if ((__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB) == 0x0)
>> -             return ERR_PTR(-ENODEV);
>> +     if (!timeout)
>> +             return ERR_PTR(-EPERM);
>>
>> -     return ERR_PTR(-EPERM);
>> +     return 0;
>> }
>
> I can see that the patch makes two changes: it adds an mdelay(1) to the
> polling loop and it changes the return value from ENODEV to EPERM if
> the loop timed out.
>
> But I don't have the faintest idea _why_ the changes were made!
>
>
> So.  I merged the patch without a changelog.  Please send a changelog
> for this patch which permits others to understand the change.
>
>

Patch

diff --git a/drivers/rtc/rtc-nuc900.c b/drivers/rtc/rtc-nuc900.c
index a351bd5..21d1330 100644
--- a/drivers/rtc/rtc-nuc900.c
+++ b/drivers/rtc/rtc-nuc900.c
@@ -85,22 +85,21 @@  static irqreturn_t nuc900_rtc_interrupt(int irq, void *_rtc)

 static int *check_rtc_access_enable(struct nuc900_rtc *nuc900_rtc)
 {
-	unsigned int i;
+	unsigned int i, timeout = 0x1000;
 	__raw_writel(INIRRESET, nuc900_rtc->rtc_reg + REG_RTC_INIR);

 	mdelay(10);

 	__raw_writel(AERPOWERON, nuc900_rtc->rtc_reg + REG_RTC_AER);

-	for (i = 0; i < 1000; i++) {
-		if (__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
-			return 0;
-	}
+	while (!(__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB)
+								&& timeout--)
+		mdelay(1);

-	if ((__raw_readl(nuc900_rtc->rtc_reg + REG_RTC_AER) & AERRWENB) == 0x0)
-		return ERR_PTR(-ENODEV);
+	if (!timeout)
+		return ERR_PTR(-EPERM);

-	return ERR_PTR(-EPERM);
+	return 0;
 }

 static void nuc900_rtc_bcd2bin(unsigned int timereg,