Re: [PATCH v2 2/2] Creating helper func for block alignment verfication

[Vimal Singh <vimal.newwork@gmail.com>]

On Thu, Jan 28, 2010 at 8:16 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> Hi,
>
> On Wed, 2010-01-13 at 18:59 +0530, Vimal Singh wrote:
>> From 310f7faa8f319bd9384512f7d5a7f13dcfbeebc8 Mon Sep 17 00:00:00 2001
>> From: Vimal Singh <vimalsingh@ti.com>
>> Date: Wed, 13 Jan 2010 18:11:47 +0530
>> Subject: [PATCH] Creating helper func for block alignment verfication
>>
>> These checks are fairly common in 'nand_erase_nand', 'nand_lock'
>> and 'nand_unlock' functions.
>>
>> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> ---
>>  drivers/mtd/nand/nand_base.c |   97 +++++++++++++++---------------------------
>>  1 files changed, 34 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index 4e27426..c80cec5 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct
>>   */
>>  DEFINE_LED_TRIGGER(nand_led_trigger);
>>
>> +static int block_alignment_verification(struct mtd_info *mtd,
>> +                                     loff_t ofs, uint64_t len)
>> +{
>
> This function checks not only alignment, so the name is bad. I suggest
> check_offs_len() - it at least does not lie about what it does :-)

OK, no problem.

>
>> +     struct nand_chip *chip = mtd->priv;
>> +
>> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
>> +                     __func__, (unsigned long long)ofs, len);
>
> No, you should keep the DEBUG part in the caller. Because of __func__.

Agree.

>
> Also please, introduce the helper in the _first_ patch, and then use it
> in your functions in the second patch. This is more logical.

Before 1st patch this helper will be called by just one function
"nand_erase_nand". And then in that creating helper function does not
makes sense to me.
To me doing this in 2nd patch looks more logical.

Either way we will achieve same goal only number of lines in patches will defer.
So, if you still insist I can make it 1st patch.

-- 
Regards,
Vimal Singh