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

[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?

> @@ -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.

> 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?

> +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.

-Scott