[U-Boot] [PATCH 15/44] dm: blk: Add a legacy block interface

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/09/2016 08:45 PM, Simon Glass wrote:
> There is quite a bit of duplicated common code related to block devices
> in the IDE and SCSI implementations.
>
> Create some helper functions that can be used to reduce the duplication.
> These rely on a linker list of interface-type drivers

It'd be useful to know if this gets thrown away later in the series if 
everything is converted to DM. That would affect how thoroughly one 
reviews it.

At this point I'm slightly lost where the series is going, since it all 
seems to be adding a slew of stuff for legacy paths more than converting 
to DM block devices. Perhaps it'll become more clear as I go along.

> diff --git a/drivers/block/blk_legacy.c b/drivers/block/blk_legacy.c

> +#ifdef HAVE_BLOCK_DEVICE
> +int blk_list_part(enum if_type if_type)
> +{
> +	struct blk_driver *drv = blk_driver_lookup_type(if_type);
> +	struct blk_desc *desc;
> +	int devnum, ok;
> +
> +	if (!drv)
> +		return -ENOSYS;

Nit: If drv can be NULL, I'd prefer to see the assignment right before 
the line that tests it rather than in the declaration. That makes it 
clearer to someone that they can't insert a line into the variable 
declarations that uses drv. That's something I've seen happen, and it's 
hard to spot the issue in a patch if the context isn't long enough to 
see this not-yet-happened test later in the code.

> +	for (ok = 0, devnum = 0; devnum < drv->max_devs; ++devnum) {
> +		if (get_desc(drv, devnum, &desc))
> +			continue;
> +		if (desc->part_type != PART_TYPE_UNKNOWN) {
> +			++ok;
> +			if (devnum)
> +				putc('\n');
> +			part_print(desc);

Does this function support dumping the partition list for multiple 
devices in one invocation? If so, that seems odd; is there any command 
to do that? If not, I would expect a break here, and I'm not sure what 
if (devnum) putc() is about. Also, shouldn't the putc happen if a 
previous iteration of the for loop did print something, not based on the 
index in the list, since presumably it's possible that entries 0..2 are 
PART_TYPE_UNKNOWN and entry 3 isn't?

> +int blk_dselect_hwpart(struct blk_desc *desc, int hwpart)

What does "dselect" mean as opposed to "select"?

> +struct blk_desc *blk_get_devnum_by_typename(const char *if_typename, int devnum)

Doesn't this get a desc not a devnum? It seems to be /by/ typename+devnum.

> diff --git a/include/blk.h b/include/blk.h

> +struct blk_driver {

> +	/**
> +	 * select_hwpart() - Select a hardware partition
> +	 *
> +	 * Some devices (e.g. MMC) can support partitioning at the hardware
> +	 * level. This is quite separate from the normal idea of
> +	 * software-based partitions. MMC hardware partitions must be
> +	 * explicitly selected. Once selected only the region of the device
> +	 * covered by that partition is accessible.
> +	 *
> +	 * The MMC standard provides for two boot partitions (numbered 1 and 2),
> +	 * rpmb (3), and up to 4 addition general-purpose partitions (4-7).

There's also partition 0, the main user data partition, which is what is 
primarily used.

> +	 *
> +	 * @desc:	Block device descriptor
> +	 * @hwpart:	Hardware partition number to select. 0 means the raw
> +	 *		device, 1 is the first partition, 2 is the second, etc.

0 isn't really "raw". At least to me, "raw" implies access to all data 
on the storage medium including partition tables, inter-partition gaps, 
and data across all partitions. However, for eMMC "0" means just another 
partition like any other partition, but this one just happens to be the 
one that's used most/typically.