Patchwork [6/9] mtd: gpmi: simplify the setting DLL code

login
register
mail settings
Submitter Huang Shijie
Date Sept. 11, 2012, 6:17 a.m.
Message ID <1347344231-10295-7-git-send-email-b32955@freescale.com>
Download mbox | patch
Permalink /patch/183016/
State New
Headers show

Comments

Huang Shijie - Sept. 11, 2012, 6:17 a.m.
The setting DLL code is a little mess.
Just simplify the code and the comments.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)
Vikram Narayanan - Sept. 11, 2012, 6:08 p.m.
Hello Huang Shijie,

On 9/11/2012 11:47 AM, Huang Shijie wrote:
> The setting DLL code is a little mess.
> Just simplify the code and the comments.
>
> Signed-off-by: Huang Shijie<b32955@freescale.com>
> ---
>   drivers/mtd/nand/gpmi-nand/gpmi-lib.c |   22 +++++++++-------------
>   1 files changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 037438a..83c5573 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -779,30 +779,26 @@ void gpmi_begin(struct gpmi_nand_data *this)
>   	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_CLR);
>
>   	/* Clear out the DLL control fields. */
> -	writel(BM_GPMI_CTRL1_RDN_DELAY,   gpmi_regs + HW_GPMI_CTRL1_CLR);
> -	writel(BM_GPMI_CTRL1_HALF_PERIOD, gpmi_regs + HW_GPMI_CTRL1_CLR);
> +	reg = BM_GPMI_CTRL1_RDN_DELAY | BM_GPMI_CTRL1_HALF_PERIOD;
> +	writel(reg, gpmi_regs + HW_GPMI_CTRL1_CLR);
>
>   	/* If no sample delay is called for, return immediately. */
>   	if (!hw.sample_delay_factor)
>   		return;
>
> -	/* Configure the HALF_PERIOD flag. */
> -	if (hw.use_half_periods)
> -		writel(BM_GPMI_CTRL1_HALF_PERIOD,
> -						gpmi_regs + HW_GPMI_CTRL1_SET);
> +	/* Set RDN_DELAY or HALF_PERIOD. */
> +	reg = ((hw.use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
> +		| BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor);
>
> -	/* Set the delay factor. */
> -	writel(BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor),
> -						gpmi_regs + HW_GPMI_CTRL1_SET);
> +	writel(reg, gpmi_regs + HW_GPMI_CTRL1_SET);
>
> -	/* Enable the DLL. */
> +	/* At last, we enable the DLL. */
>   	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_SET);
>
>   	/*
>   	 * After we enable the GPMI DLL, we have to wait 64 clock cycles before
> -	 * we can use the GPMI.
> -	 *
> -	 * Calculate the amount of time we need to wait, in microseconds.
> +	 * we can use the GPMI. Calculate the amount of time we need to wait,
> +	 * in microseconds.
>   	 */
>   	clock_period_in_ns = 1000000000 / clk_get_rate(r->clock[0]);

NSEC_PER_SEC macro in the above statement?

Don't curse me for commenting about the code that you've not written. As 
this patch does some cleanups, I'm suggesting this.


>   	dll_wait_time_in_us = (clock_period_in_ns * 64) / 1000;

Regards,
Vikram
Huang Shijie - Sept. 12, 2012, 2:28 a.m.
于 2012年09月12日 02:08, Vikram Narayanan 写道:
> Hello Huang Shijie,
>
> On 9/11/2012 11:47 AM, Huang Shijie wrote:
>> The setting DLL code is a little mess.
>> Just simplify the code and the comments.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> ---
>> drivers/mtd/nand/gpmi-nand/gpmi-lib.c | 22 +++++++++-------------
>> 1 files changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c 
>> b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> index 037438a..83c5573 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> @@ -779,30 +779,26 @@ void gpmi_begin(struct gpmi_nand_data *this)
>> writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_CLR);
>>
>> /* Clear out the DLL control fields. */
>> - writel(BM_GPMI_CTRL1_RDN_DELAY, gpmi_regs + HW_GPMI_CTRL1_CLR);
>> - writel(BM_GPMI_CTRL1_HALF_PERIOD, gpmi_regs + HW_GPMI_CTRL1_CLR);
>> + reg = BM_GPMI_CTRL1_RDN_DELAY | BM_GPMI_CTRL1_HALF_PERIOD;
>> + writel(reg, gpmi_regs + HW_GPMI_CTRL1_CLR);
>>
>> /* If no sample delay is called for, return immediately. */
>> if (!hw.sample_delay_factor)
>> return;
>>
>> - /* Configure the HALF_PERIOD flag. */
>> - if (hw.use_half_periods)
>> - writel(BM_GPMI_CTRL1_HALF_PERIOD,
>> - gpmi_regs + HW_GPMI_CTRL1_SET);
>> + /* Set RDN_DELAY or HALF_PERIOD. */
>> + reg = ((hw.use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
>> + | BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor);
>>
>> - /* Set the delay factor. */
>> - writel(BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor),
>> - gpmi_regs + HW_GPMI_CTRL1_SET);
>> + writel(reg, gpmi_regs + HW_GPMI_CTRL1_SET);
>>
>> - /* Enable the DLL. */
>> + /* At last, we enable the DLL. */
>> writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_SET);
>>
>> /*
>> * After we enable the GPMI DLL, we have to wait 64 clock cycles before
>> - * we can use the GPMI.
>> - *
>> - * Calculate the amount of time we need to wait, in microseconds.
>> + * we can use the GPMI. Calculate the amount of time we need to wait,
>> + * in microseconds.
>> */
>> clock_period_in_ns = 1000000000 / clk_get_rate(r->clock[0]);
>
> NSEC_PER_SEC macro in the above statement?
thanks. I like this macro.

>
> Don't curse me for commenting about the code that you've not written. 
> As this patch does some cleanups, I'm suggesting this.
>
sorry, I do not understand it very well.
could you tell me in detail what's the "commenting about the code that 
you've not written" meaning?

Which comments do you think is not proper?

Best Regards
Huang Shijie



>
>> dll_wait_time_in_us = (clock_period_in_ns * 64) / 1000;
>
> Regards,
> Vikram
>
Huang Shijie - Sept. 12, 2012, 7 a.m.
于 2012年09月12日 02:08, Vikram Narayanan 写道:
>>
>> /*
>> * After we enable the GPMI DLL, we have to wait 64 clock cycles before
>> - * we can use the GPMI.
>> - *
>> - * Calculate the amount of time we need to wait, in microseconds.
>> + * we can use the GPMI. Calculate the amount of time we need to wait,
>> + * in microseconds.
>> */
>> clock_period_in_ns = 1000000000 / clk_get_rate(r->clock[0]);
>
> NSEC_PER_SEC macro in the above statement?
>
> Don't curse me for commenting about the code that you've not written. 
> As this patch does some cleanups, I'm suggesting this. 
I finally get what's your meaning. thanks for you review :)

I really appreciate it.


Best Regards
Huang Shijie

Patch

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 037438a..83c5573 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -779,30 +779,26 @@  void gpmi_begin(struct gpmi_nand_data *this)
 	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_CLR);
 
 	/* Clear out the DLL control fields. */
-	writel(BM_GPMI_CTRL1_RDN_DELAY,   gpmi_regs + HW_GPMI_CTRL1_CLR);
-	writel(BM_GPMI_CTRL1_HALF_PERIOD, gpmi_regs + HW_GPMI_CTRL1_CLR);
+	reg = BM_GPMI_CTRL1_RDN_DELAY | BM_GPMI_CTRL1_HALF_PERIOD;
+	writel(reg, gpmi_regs + HW_GPMI_CTRL1_CLR);
 
 	/* If no sample delay is called for, return immediately. */
 	if (!hw.sample_delay_factor)
 		return;
 
-	/* Configure the HALF_PERIOD flag. */
-	if (hw.use_half_periods)
-		writel(BM_GPMI_CTRL1_HALF_PERIOD,
-						gpmi_regs + HW_GPMI_CTRL1_SET);
+	/* Set RDN_DELAY or HALF_PERIOD. */
+	reg = ((hw.use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
+		| BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor);
 
-	/* Set the delay factor. */
-	writel(BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor),
-						gpmi_regs + HW_GPMI_CTRL1_SET);
+	writel(reg, gpmi_regs + HW_GPMI_CTRL1_SET);
 
-	/* Enable the DLL. */
+	/* At last, we enable the DLL. */
 	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_SET);
 
 	/*
 	 * After we enable the GPMI DLL, we have to wait 64 clock cycles before
-	 * we can use the GPMI.
-	 *
-	 * Calculate the amount of time we need to wait, in microseconds.
+	 * we can use the GPMI. Calculate the amount of time we need to wait,
+	 * in microseconds.
 	 */
 	clock_period_in_ns = 1000000000 / clk_get_rate(r->clock[0]);
 	dll_wait_time_in_us = (clock_period_in_ns * 64) / 1000;