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.
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
