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

Submitted by Huang Shijie on Sept. 11, 2012, 6:17 a.m.

Details

Message ID 1347344231-10295-7-git-send-email-b32955@freescale.com
State New, archived
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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;