[U-Boot] [PATCH 2/3] mtd, nand: move common functions from cmd_nand.c to common place

[Heiko Schocher]

Hello Scott,

Am 30.07.2014 02:29, schrieb Scott Wood:
> On Mon, 2014-07-14 at 09:39 +0200, Heiko Schocher wrote:
>> move common functions from cmd_nand.c (for calculating offset
>> and size from cmdline paramter) to common place, so they could
>> used from other commands which use mtd partitions.
>>
>> For onenand the arg_off_size() is left in common/cmd_onenand.c.
>> It should use now the common arg_off() function, but as I could
>> not test onenand I let it there ...
>>
>> Signed-off-by: Heiko Schocher<hs at denx.de>
>> Cc: Scott Wood<scottwood at freescale.com>
>> Cc: Tom Rini<trini at ti.com>
>> ---
>>   common/cmd_nand.c       | 140 +++++++++---------------------------------------
>>   common/cmd_onenand.c    |  19 +++----
>>   drivers/mtd/Makefile    |   4 +-
>>   drivers/mtd/mtd_uboot.c | 114 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/mtd/mtd.h |   7 +++
>>   5 files changed, 154 insertions(+), 130 deletions(-)
>>   create mode 100644 drivers/mtd/mtd_uboot.c
>
> ACK the concept, some nits below.
>
> Have you given any more thought to formally taking over the
> custodianship?  If not, Tom, are you expecting another pull request from
> me or were you going to apply Heiko's patches directly?

I thinking about it, yes, but I am really busy ... but it seems
nobody else want to volunteer ... I discussed with Wolfgang, if
it would make sense to create a mtd branch ... so, if you could not
longer maintain the nand branch, we can set it to orphaned (in the
hope, someone will volunteer ...) and I can also handle the nand
patches ... but Wolfgang is currently on vacation, so this step
maybe take a while ...

>> @@ -322,7 +213,12 @@ int do_nand_env_oob(cmd_tbl_t *cmdtp, int argc, char *const argv[])
>>   			goto usage;
>>
>>   		/* We don't care about size, or maxsize. */
>> -		if (arg_off(argv[2],&idx,&addr,&maxsize,&maxsize)) {
>> +		if (arg_off(argv[2],&idx,&addr,&maxsize,&maxsize,
>> +		    MTD_DEV_TYPE_NAND, nand_info[idx].size)) {
>> +			puts("Offset or partition name expected\n");
>> +			return 1;
>> +		}
>
> MTD_DEV_TYPE_NAND should be aligned with argv[2] (if you're going to do
> alignment rather than just a couple tabs), not arg_off.  Likewise
> elsewhere.

fixed.

>> diff --git a/drivers/mtd/mtd_uboot.c b/drivers/mtd/mtd_uboot.c
>> new file mode 100644
>> index 0000000..66e49c3
>> --- /dev/null
>> +++ b/drivers/mtd/mtd_uboot.c
>> @@ -0,0 +1,114 @@
>> +/*
>> + * (C) Copyright 2014
>> + * Heiko Schocher, DENX Software Engineering, hs at denx.de.
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +#include<common.h>
>> +#include<linux/mtd/mtd.h>
>> +#include<jffs2/jffs2.h>
>
> What do you use from the jffs2 header?

Yes, good question ... because missing common mtd defines, like:

struct mtd_device {

found in include/jffs2/load_kernel.h
used in common/cmd_nand.c for example ... its really time to
cleanup the mtd subsystem!

Where to move this common defines? include/linux/mtd/mtd.h ?

But before the mtd/ubi/ubifs sync patches are not in mainline, I do
not want change this ... maybe it is okay to do this in a second step?

>> +inline int str2off(const char *p, loff_t *num)
>> +{
>> +	char *endptr;
>> +
>> +	*num = simple_strtoull(p,&endptr, 16);
>> +	return *p != '\0'&&  *endptr == '\0';
>> +}
>> +
>> +inline int str2long(const char *p, ulong *num)
>> +{
>> +	char *endptr;
>> +
>> +	*num = simple_strtoul(p,&endptr, 16);
>> +	return *p != '\0'&&  *endptr == '\0';
>> +}
>
> Should drop the inline -- it didn't make much sense in the old location
> as it wasn't in a header file (GCC can auto-inline where appropriate),
> and it makes even less sense if it's not static.

removed, thanks for the review!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany