Dear Wolfgang Denk,
> Dear Lukasz Majewski,
>
> In message <1341416922-13792-5-git-send-email-l.majew...@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.
Ok, this can be easily corrected.
>
>
> 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.
>
So I suppose, that you are proposing something like this (pseudo code):
int mmc_block_write() {
sprintf(cmd_buf, "mmc write 0x%x %x %x")
run_command(cmd_buf, 0);
}
int mmc_file_write(enum fs_type) {
switch (fs_type)
case FAT:
sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx")
break;
case EXT4:
sprintf(cmd_buf, "ext4write mmc %d:%d 0x%x %s %lx")
break;
run_command(cmd_buf);
}
int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
{
ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
memset(cmd_buf, '\0', sizeof(cmd_buf));
switch (dfu->layout) {
case RAW_ADDR:
mmc_block_write()
break;
case FAT:
case EXT3:
case EXT4:
mmc_file_write(dfu->layout);
break;
default:
printf("%s: Wrong layout %s!\n", __func__,
layout_table[dfu->layout]); }
return 0;
}
>
> 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,
Lukasz Majewski
Samsung Poland R&D Center | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
