Re: [U-Boot] [PATCH 10/30] dm: blk: Rename get_device() to blk_get_device_str()

[Stephen Warren]

On 02/14/2016 07:16 PM, Simon Glass wrote:

The current name is too generic. The function returns a block device based
on a provided string. Rename it to aid searching and make its purpose
clearer. Also add a few comments.



+int blk_get_device_str(const char *ifname, const char *dev_str,
+  struct blk_desc **dev_desc);


Bikeshed: s/_str/_by_str/? Otherwise it somewhat sounds like it returns 
a "device_str" rather than returns a "device" and searches for it using 
a "str".


Of course, this is a really annoying comment that would cause a large 
amount of work, so feel free to ignore it.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 10/30] dm: blk: Rename get_device() to blk_get_device_str()

[Bin Meng]

Hi Simon,

On Mon, Feb 15, 2016 at 10:16 AM, Simon Glass  wrote:
> The current name is too generic. The function returns a block device based
> on a provided string. Rename it to aid searching and make its purpose
> clearer. Also add a few comments.
>
> Signed-off-by: Simon Glass 
> ---
>
>  cmd/part.c |  6 +++---
>  cmd/unzip.c|  2 +-
>  cmd/usb_mass_storage.c |  2 +-
>  disk/part.c|  6 +++---
>  include/part.h | 34 ++
>  test/dm/usb.c  |  2 +-
>  6 files changed, 39 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng 

One comment below

> diff --git a/cmd/part.c b/cmd/part.c
> index a572aab..f05699d 100644
> --- a/cmd/part.c
> +++ b/cmd/part.c
> @@ -81,7 +81,7 @@ static int do_part_list(int argc, char * const argv[])
> return CMD_RET_USAGE;
> }
>
> -   ret = get_device(argv[0], argv[1], );
> +   ret = blk_get_device_str(argv[0], argv[1], );
> if (ret < 0)
> return 1;
>
> @@ -128,7 +128,7 @@ static int do_part_start(int argc, char * const argv[])
>
> part = simple_strtoul(argv[2], NULL, 0);
>
> -   ret = get_device(argv[0], argv[1], );
> +   ret = blk_get_device_str(argv[0], argv[1], );
> if (ret < 0)
> return 1;
>
> @@ -162,7 +162,7 @@ static int do_part_size(int argc, char * const argv[])
>
> part = simple_strtoul(argv[2], NULL, 0);
>
> -   ret = get_device(argv[0], argv[1], );
> +   ret = blk_get_device_str(argv[0], argv[1], );
> if (ret < 0)
> return 1;
>
> diff --git a/cmd/unzip.c b/cmd/unzip.c
> index 5be1566..588fa75 100644
> --- a/cmd/unzip.c
> +++ b/cmd/unzip.c
> @@ -53,7 +53,7 @@ static int do_gzwrite(cmd_tbl_t *cmdtp, int flag,
>
> if (argc < 5)
> return CMD_RET_USAGE;
> -   ret = get_device(argv[1], argv[2], );
> +   ret = blk_get_device_str(argv[1], argv[2], );
> if (ret < 0)
> return CMD_RET_FAILURE;
>
> diff --git a/cmd/usb_mass_storage.c b/cmd/usb_mass_storage.c
> index 03b7e21..fcac389 100644
> --- a/cmd/usb_mass_storage.c
> +++ b/cmd/usb_mass_storage.c
> @@ -69,7 +69,7 @@ static int ums_init(const char *devtype, const char 
> *devnums)
> if (!devnum)
> break;
>
> -   ret = get_device(devtype, devnum, _dev);
> +   ret = blk_get_device_str(devtype, devnum, _dev);
> if (ret < 0)
> goto cleanup;
>
> diff --git a/disk/part.c b/disk/part.c
> index 2466c3e..700e505 100644
> --- a/disk/part.c
> +++ b/disk/part.c
> @@ -449,8 +449,8 @@ int get_partition_info(struct blk_desc *dev_desc, int 
> part,
> return -1;
>  }
>
> -int get_device(const char *ifname, const char *dev_hwpart_str,
> -  struct blk_desc **dev_desc)
> +int blk_get_device_str(const char *ifname, const char *dev_hwpart_str,
> +  struct blk_desc **dev_desc)
>  {
> char *ep;
> char *dup_str = NULL;
> @@ -598,7 +598,7 @@ int get_device_and_partition(const char *ifname, const 
> char *dev_part_str,
> }
>
> /* Look up the device */
> -   dev = get_device(ifname, dev_str, dev_desc);
> +   dev = blk_get_device_str(ifname, dev_str, dev_desc);
> if (dev < 0)
> goto cleanup;
>
> diff --git a/include/part.h b/include/part.h
> index ddc4422..d05b48b 100644
> --- a/include/part.h
> +++ b/include/part.h
> @@ -101,8 +101,34 @@ int get_partition_info(struct blk_desc *dev_desc, int 
> part,
>  void print_part(struct blk_desc *dev_desc);
>  void init_part(struct blk_desc *dev_desc);
>  void dev_print(struct blk_desc *dev_desc);
> -int get_device(const char *ifname, const char *dev_str,
> -  struct blk_desc **dev_desc);
> +
> +/**
> + * blk_get_device_str() - Get a block device given its interface/ hw 
> partition

nits: need one space before /, or remove space before 'hw'?

I am not sure if blk_get_device_str() is a good name, but I cannot
think of a better name..

> + *
> + * Each interface allocates its own devices and typically struct blk_desc is
> + * contained with the interface's data structure. There is no global
> + * numbering for block devices, so the interface name must be provided.
> + *
> + * The hardware parition is not related to the normal software partitioning
> + * of a device - each hardware partition is effectively a separately
> + * accessible block device. When a hardware parition is selected on MMC the
> + * other hardware partitions become inaccessible. The same block device is
> + * used to access all hardware partitions, but its capacity may change when a
> + * different hardware partition is selected.
> + *
> + * When a hardware partition number is given, the block device switches to
> + * that hardware partition.
> + *
> + * @ifname:Interface name (e.g. "ide",