[U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation

[Wolfgang Denk]

Dear Lukasz Majewski,

In message <1341416922-13792-5-git-send-email-l.majewski at samsung.com> you wrote:
> Support for MMC storage devices to work with DFU framework.

Sorry for jumping in late.


> +	switch (dfu->layout) {
> +	case RAW_ADDR:
> +		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned int) buf,
> +			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
> +		break;
> +	case FAT:
> +		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
> +			dfu->data.mmc.dev, dfu->data.mmc.part,
> +			(unsigned int) buf, dfu->name, *len);
> +		break;
> +	default:
> +		printf("%s: Wrong layout!\n", __func__);
> +	}

In case of error, you should always print what the unexpected data
was.  The end user who receives an "Wrong layout!" error is probably
pretty much surpised and doesn't know what he did wrong.   If you
print instead: "EXT2 layout not supported (yet)" he would know exactly
what to change to make it work.


There has been some dicussion already if using this sprintf() /
run_command() approach is good or not, or how it should be fixed.

It appears, all this discussion forgot to take into account that
patch 3/7 dfu: DFU backend implementation promised to add "platform
and storage independent operation of DFU."  Here we are breaking this
promise.

And by adding special functions to the FAT file system code thing sget
just worse, as now not only the DFU code needs to be extended when we
want to use this with any other file system type, but suddenly this
also bleeds into all supported file system code.


I am aware that the current implementation suffers from the fact that
we don't have a unified access to file systems - each comes with it's
own set of commands and more or less (in)compatible command line and
function call interfaces.  This will hopefully improve in the context
of the device model rework, but we don't want to block you until then.
But considering that this is going to change in a foreseeable future,
it also makes littel sense to invest big efforts into generic code
that covers all potential future uses.

Here is my poposal:

Let's introduce a thin shim layer to abstract the file system
interface from the accesses you need here.  As far as I can see here,
you need exactly 4 functions:

- block_read()
- block_write()
- file_read()
- file_write()

These names could be function pointers to implement the device I/O in
a both device and file system agnostic way; you can implement
specific versions of the functions like mmc_block_read(),
usb_block_read(), ... etc. and use the settings of dfu_device_type
and dfu_layout to set the pointers to these functions.

For this shim layer I would actually prefer the original approach
(shown above) to use sprintf() to build commands based on the existing
command interface - this is the minimal intrusive way to implement
this for now.  With this approach, no modifications to file system
and/or device driver code are needed, and we still maintain full
flexibility here in the DFU code.  Yes, error checking may not be
perfect, and we my not win a price for elegance of design either, but
we should get mostly clean, working code quickly.


The advantage I see for this code is that we have separated all
this device interface stuff from both the generic DFU code and from
the device and file system code as well.  As soon as we have a better
implementation below, all we need to adjust (or potentially even
remove) is this shim layer.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
To the systems programmer,  users  and  applications  serve  only  to
provide a test load.