Message ID | 1455502619-16093-11-git-send-email-sjg@chromium.org |
---|---|
State | Superseded |
Delegated to: | Simon Glass |
Headers | show |
Hi Simon, On Mon, Feb 15, 2016 at 10:16 AM, Simon Glass <sjg@chromium.org> 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 <sjg@chromium.org> > --- > > 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 <bmeng.cn@gmail.com> 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], &desc); > + ret = blk_get_device_str(argv[0], argv[1], &desc); > 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], &desc); > + ret = blk_get_device_str(argv[0], argv[1], &desc); > 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], &desc); > + ret = blk_get_device_str(argv[0], argv[1], &desc); > 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], &bdev); > + ret = blk_get_device_str(argv[1], argv[2], &bdev); > 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, &block_dev); > + ret = blk_get_device_str(devtype, devnum, &block_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", "scsi") > + * @dev_str: Device and optional hw partition. This can either be a string > + * containing the device number (e.g. "2") or the device number > + * and hardware partition number (e.g. "2.4") for devices that > + * support it (currently only MMC). > + * @dev_desc: Returns a pointer to the block device on success > + * @return block device number (local to the interface), or -1 on error > + */ > +int blk_get_device_str(const char *ifname, const char *dev_str, > + struct blk_desc **dev_desc); > int get_device_and_partition(const char *ifname, const char *dev_part_str, > struct blk_desc **dev_desc, > disk_partition_t *info, int allow_whole_dev); > @@ -124,8 +150,8 @@ static inline int get_partition_info(struct blk_desc *dev_desc, int part, > static inline void print_part(struct blk_desc *dev_desc) {} > static inline void init_part(struct blk_desc *dev_desc) {} > static inline void dev_print(struct blk_desc *dev_desc) {} > -static inline int get_device(const char *ifname, const char *dev_str, > - struct blk_desc **dev_desc) > +static inline int blk_get_device_str(const char *ifname, const char *dev_str, > + struct blk_desc **dev_desc) > { return -1; } > static inline int get_device_and_partition(const char *ifname, > const char *dev_part_str, > diff --git a/test/dm/usb.c b/test/dm/usb.c > index 25cde68..1df25b1 100644 > --- a/test/dm/usb.c > +++ b/test/dm/usb.c > @@ -45,7 +45,7 @@ static int dm_test_usb_flash(struct unit_test_state *uts) > state_set_skip_delays(true); > ut_assertok(usb_init()); > ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev)); > - ut_assertok(get_device("usb", "0", &dev_desc)); > + ut_assertok(blk_get_device_str("usb", "0", &dev_desc)); > > /* Read a few blocks and look for the string we expect */ > ut_asserteq(512, dev_desc->blksz); > -- Regards, Bin
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.
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], &desc); + ret = blk_get_device_str(argv[0], argv[1], &desc); 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], &desc); + ret = blk_get_device_str(argv[0], argv[1], &desc); 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], &desc); + ret = blk_get_device_str(argv[0], argv[1], &desc); 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], &bdev); + ret = blk_get_device_str(argv[1], argv[2], &bdev); 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, &block_dev); + ret = blk_get_device_str(devtype, devnum, &block_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 + * + * 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", "scsi") + * @dev_str: Device and optional hw partition. This can either be a string + * containing the device number (e.g. "2") or the device number + * and hardware partition number (e.g. "2.4") for devices that + * support it (currently only MMC). + * @dev_desc: Returns a pointer to the block device on success + * @return block device number (local to the interface), or -1 on error + */ +int blk_get_device_str(const char *ifname, const char *dev_str, + struct blk_desc **dev_desc); int get_device_and_partition(const char *ifname, const char *dev_part_str, struct blk_desc **dev_desc, disk_partition_t *info, int allow_whole_dev); @@ -124,8 +150,8 @@ static inline int get_partition_info(struct blk_desc *dev_desc, int part, static inline void print_part(struct blk_desc *dev_desc) {} static inline void init_part(struct blk_desc *dev_desc) {} static inline void dev_print(struct blk_desc *dev_desc) {} -static inline int get_device(const char *ifname, const char *dev_str, - struct blk_desc **dev_desc) +static inline int blk_get_device_str(const char *ifname, const char *dev_str, + struct blk_desc **dev_desc) { return -1; } static inline int get_device_and_partition(const char *ifname, const char *dev_part_str, diff --git a/test/dm/usb.c b/test/dm/usb.c index 25cde68..1df25b1 100644 --- a/test/dm/usb.c +++ b/test/dm/usb.c @@ -45,7 +45,7 @@ static int dm_test_usb_flash(struct unit_test_state *uts) state_set_skip_delays(true); ut_assertok(usb_init()); ut_assertok(uclass_get_device(UCLASS_MASS_STORAGE, 0, &dev)); - ut_assertok(get_device("usb", "0", &dev_desc)); + ut_assertok(blk_get_device_str("usb", "0", &dev_desc)); /* Read a few blocks and look for the string we expect */ ut_asserteq(512, dev_desc->blksz);
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 <sjg@chromium.org> --- 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(-)