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

login
register
mail settings
Submitter Łukasz Majewski
Date July 3, 2012, 9:38 a.m.
Message ID <1341308291-14663-5-git-send-email-l.majewski@samsung.com>
Download mbox | patch
Permalink /patch/168751/
State Changes Requested
Headers show

Comments

Łukasz Majewski - July 3, 2012, 9:38 a.m.
Support for MMC storage devices to work with DFU framework.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/dfu/Makefile  |    1 +
 drivers/dfu/dfu_mmc.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dfu/dfu_mmc.c
Marek Vasut - July 3, 2012, 9:29 p.m.
Dear Lukasz Majewski,

> Support for MMC storage devices to work with DFU framework.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> ---

[...]

> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <dfu.h>
> +
> +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:
> +		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__);
> +	}
> +
> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> +	run_command(cmd_buf, 0);

Holy Moly ... can we not make this into simple calls to those subsystems ? 
Instead invoking command is crazy ;-)

[...]

Best regards,
Marek Vasut
Tom Rini - July 3, 2012, 9:55 p.m.
On Tue, Jul 03, 2012 at 11:29:31PM +0200, Marek Vasut wrote:
> Dear Lukasz Majewski,
> 
> > Support for MMC storage devices to work with DFU framework.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> > ---
> 
> [...]
> 
> > +
> > +#include <common.h>
> > +#include <malloc.h>
> > +#include <dfu.h>
> > +
> > +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:
> > +		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__);
> > +	}
> > +
> > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > +	run_command(cmd_buf, 0);
> 
> Holy Moly ... can we not make this into simple calls to those subsystems ? 
> Instead invoking command is crazy ;-)

Are they really simple?  There's a few other places we do this, and so
long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing
to fat and so forth.
Marek Vasut - July 3, 2012, 10:01 p.m.
Dear Tom Rini,

> On Tue, Jul 03, 2012 at 11:29:31PM +0200, Marek Vasut wrote:
> > Dear Lukasz Majewski,
> > 
> > > Support for MMC storage devices to work with DFU framework.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > > ---
> > 
> > [...]
> > 
> > > +
> > > +#include <common.h>
> > > +#include <malloc.h>
> > > +#include <dfu.h>
> > > +
> > > +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:
> > > +		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__);
> > > +	}
> > > +
> > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > +	run_command(cmd_buf, 0);
> > 
> > Holy Moly ... can we not make this into simple calls to those subsystems
> > ? Instead invoking command is crazy ;-)
> 
> Are they really simple?  There's a few other places we do this, and so
> long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing
> to fat and so forth.

Well ain't it easier to call fat_write() or similar?

Best regards,
Marek Vasut
Tom Rini - July 3, 2012, 10:06 p.m.
On Wed, Jul 04, 2012 at 12:01:27AM +0200, Marek Vasut wrote:
> Dear Tom Rini,
> 
> > On Tue, Jul 03, 2012 at 11:29:31PM +0200, Marek Vasut wrote:
> > > Dear Lukasz Majewski,
> > > 
> > > > Support for MMC storage devices to work with DFU framework.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > ---
> > > 
> > > [...]
> > > 
> > > > +
> > > > +#include <common.h>
> > > > +#include <malloc.h>
> > > > +#include <dfu.h>
> > > > +
> > > > +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:
> > > > +		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__);
> > > > +	}
> > > > +
> > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > +	run_command(cmd_buf, 0);
> > > 
> > > Holy Moly ... can we not make this into simple calls to those subsystems
> > > ? Instead invoking command is crazy ;-)
> > 
> > Are they really simple?  There's a few other places we do this, and so
> > long as it's documented that DFU depends on CONFIG_FAT_WRITE for writing
> > to fat and so forth.
> 
> Well ain't it easier to call fat_write() or similar?

Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
think a good portion of it is, at first glance at least.
Marek Vasut - July 3, 2012, 10:31 p.m.
Dear Tom Rini,

[...]

> > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > +	run_command(cmd_buf, 0);
> > > > 
> > > > Holy Moly ... can we not make this into simple calls to those
> > > > subsystems ? Instead invoking command is crazy ;-)
> > > 
> > > Are they really simple?  There's a few other places we do this, and so
> > > long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> > > writing to fat and so forth.
> > 
> > Well ain't it easier to call fat_write() or similar?
> 
> Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
> think a good portion of it is, at first glance at least.

Abstracting it out into a function won't cut it?

Best regards,
Marek Vasut
Tom Rini - July 3, 2012, 10:33 p.m.
On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
> Dear Tom Rini,
> 
> [...]
> 
> > > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > > +	run_command(cmd_buf, 0);
> > > > > 
> > > > > Holy Moly ... can we not make this into simple calls to those
> > > > > subsystems ? Instead invoking command is crazy ;-)
> > > > 
> > > > Are they really simple?  There's a few other places we do this, and so
> > > > long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> > > > writing to fat and so forth.
> > > 
> > > Well ain't it easier to call fat_write() or similar?
> > 
> > Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
> > think a good portion of it is, at first glance at least.
> 
> Abstracting it out into a function won't cut it?

My inclination would be to say that seems a bit sillier than just using
run_command(...) to call the existing command.
Stephen Warren - July 3, 2012, 11:07 p.m.
On 07/03/2012 04:33 PM, Tom Rini wrote:
> On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
>> Dear Tom Rini,
>>
>> [...]
>>
>>>>>>> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
>>>>>>> +	run_command(cmd_buf, 0);
>>>>>>
>>>>>> Holy Moly ... can we not make this into simple calls to those
>>>>>> subsystems ? Instead invoking command is crazy ;-)
>>>>>
>>>>> Are they really simple?  There's a few other places we do this, and so
>>>>> long as it's documented that DFU depends on CONFIG_FAT_WRITE for
>>>>> writing to fat and so forth.
>>>>
>>>> Well ain't it easier to call fat_write() or similar?
>>>
>>> Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
>>> think a good portion of it is, at first glance at least.
>>
>> Abstracting it out into a function won't cut it?
> 
> My inclination would be to say that seems a bit sillier than just using
> run_command(...) to call the existing command.

Abstracting into a function definitely means the compiler will be able
to type-check all the arguments to the function. Is the same true using
run_command; I assume everything gets serialized to an array of strings
and hence any validation is deferred until run-time then?
Tom Rini - July 3, 2012, 11:38 p.m.
On Tue, Jul 03, 2012 at 05:07:04PM -0600, Stephen Warren wrote:
> On 07/03/2012 04:33 PM, Tom Rini wrote:
> > On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
> >> Dear Tom Rini,
> >>
> >> [...]
> >>
> >>>>>>> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> >>>>>>> +	run_command(cmd_buf, 0);
> >>>>>>
> >>>>>> Holy Moly ... can we not make this into simple calls to those
> >>>>>> subsystems ? Instead invoking command is crazy ;-)
> >>>>>
> >>>>> Are they really simple?  There's a few other places we do this, and so
> >>>>> long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> >>>>> writing to fat and so forth.
> >>>>
> >>>> Well ain't it easier to call fat_write() or similar?
> >>>
> >>> Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
> >>> think a good portion of it is, at first glance at least.
> >>
> >> Abstracting it out into a function won't cut it?
> > 
> > My inclination would be to say that seems a bit sillier than just using
> > run_command(...) to call the existing command.
> 
> Abstracting into a function definitely means the compiler will be able
> to type-check all the arguments to the function. Is the same true using
> run_command; I assume everything gets serialized to an array of strings
> and hence any validation is deferred until run-time then?

string is constructed with sprintf and that gives us some type checking.
Only cast today is buf to an unsigned int.  And the information either
comes from tools that should/must have done sanity checking or from the
'dfu' command which again should (didn't verify that patch yet) have
done sanity checking to get us this far.
Stephen Warren - July 3, 2012, 11:58 p.m.
On 07/03/2012 05:38 PM, Tom Rini wrote:
> On Tue, Jul 03, 2012 at 05:07:04PM -0600, Stephen Warren wrote:
>> On 07/03/2012 04:33 PM, Tom Rini wrote:
>>> On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
>>>> Dear Tom Rini,
>>>>
>>>> [...]
>>>>
>>>>>>>>> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
>>>>>>>>> +	run_command(cmd_buf, 0);
>>>>>>>>
>>>>>>>> Holy Moly ... can we not make this into simple calls to those
>>>>>>>> subsystems ? Instead invoking command is crazy ;-)
>>>>>>>
>>>>>>> Are they really simple?  There's a few other places we do this, and so
>>>>>>> long as it's documented that DFU depends on CONFIG_FAT_WRITE for
>>>>>>> writing to fat and so forth.
>>>>>>
>>>>>> Well ain't it easier to call fat_write() or similar?
>>>>>
>>>>> Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
>>>>> think a good portion of it is, at first glance at least.
>>>>
>>>> Abstracting it out into a function won't cut it?
>>>
>>> My inclination would be to say that seems a bit sillier than just using
>>> run_command(...) to call the existing command.
>>
>> Abstracting into a function definitely means the compiler will be able
>> to type-check all the arguments to the function. Is the same true using
>> run_command; I assume everything gets serialized to an array of strings
>> and hence any validation is deferred until run-time then?
> 
> string is constructed with sprintf and that gives us some type checking.
> Only cast today is buf to an unsigned int.  And the information either
> comes from tools that should/must have done sanity checking or from the
> 'dfu' command which again should (didn't verify that patch yet) have
> done sanity checking to get us this far.

So that checks the parameters against the sprintf format string, but
what checks that you chose the right format string for the command?
Marek Vasut - July 4, 2012, 12:13 a.m.
Dear Tom Rini,

> On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
> > Dear Tom Rini,
> > 
> > [...]
> > 
> > > > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > > > +	run_command(cmd_buf, 0);
> > > > > > 
> > > > > > Holy Moly ... can we not make this into simple calls to those
> > > > > > subsystems ? Instead invoking command is crazy ;-)
> > > > > 
> > > > > Are they really simple?  There's a few other places we do this, and
> > > > > so long as it's documented that DFU depends on CONFIG_FAT_WRITE
> > > > > for writing to fat and so forth.
> > > > 
> > > > Well ain't it easier to call fat_write() or similar?
> > > 
> > > Assuming that most of the logic in do_fat_fswrite is needed, no.  And I
> > > think a good portion of it is, at first glance at least.
> > 
> > Abstracting it out into a function won't cut it?
> 
> My inclination would be to say that seems a bit sillier than just using
> run_command(...) to call the existing command.

Whose's syntax will likely soon change, breaking all this :-(

Best regards,
Marek Vasut
Łukasz Majewski - July 4, 2012, 9:10 a.m.
Hi Marek,

> Dear Tom Rini,
> 
> > On Tue, Jul 03, 2012 at 11:29:31PM +0200, Marek Vasut wrote:
> > > Dear Lukasz Majewski,
> > > 
> > > > Support for MMC storage devices to work with DFU framework.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > ---
> > > 
> > > [...]
> > > 
> > > > +
> > > > +#include <common.h>
> > > > +#include <malloc.h>
> > > > +#include <dfu.h>
> > > > +
> > > > +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:
> > > > +		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__);
> > > > +	}
> > > > +
> > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > +	run_command(cmd_buf, 0);
> > > 
> > > Holy Moly ... can we not make this into simple calls to those
> > > subsystems ? Instead invoking command is crazy ;-)
> > 
> > Are they really simple?  There's a few other places we do this, and
> > so long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> > writing to fat and so forth.
> 
> Well ain't it easier to call fat_write() or similar?
> 
I've decided to use run_command on a purpose.

This call provides clean and reliable API. It is very unlikely that the
mmc write <dev> <addr> <start> <size> command will change (or any
other). 
On the other hand the fields of struct mmc are changed from time to
time.

Moreover, mmc drivers are also a subject to change (like adding dw_mmc
recently).
Using run_command also takes the burden of mmc_init() related calls. 

Of course the run_command's downside is the speed of execution. But is
it so important when one considers, the firmware update? 

Side note: DFU uses only EP0 (for transfer and configuration), so this
is rather slow communication link. 

I'm open for discussion.
Marek Vasut - July 4, 2012, 2:38 p.m.
Dear Lukasz Majewski,

[...]

> > > > Holy Moly ... can we not make this into simple calls to those
> > > > subsystems ? Instead invoking command is crazy ;-)
> > > 
> > > Are they really simple?  There's a few other places we do this, and
> > > so long as it's documented that DFU depends on CONFIG_FAT_WRITE for
> > > writing to fat and so forth.
> > 
> > Well ain't it easier to call fat_write() or similar?
> 
> I've decided to use run_command on a purpose.
> 
> This call provides clean and reliable API. It is very unlikely that the
> mmc write <dev> <addr> <start> <size> command will change (or any
> other).
> On the other hand the fields of struct mmc are changed from time to
> time.

I'm afraid it might change with the driver model soon.

> Moreover, mmc drivers are also a subject to change (like adding dw_mmc
> recently).
> Using run_command also takes the burden of mmc_init() related calls.
> 
> Of course the run_command's downside is the speed of execution. But is
> it so important when one considers, the firmware update?

But as Stephen pointed out, the type checking is much better when used as 
function.

> Side note: DFU uses only EP0 (for transfer and configuration), so this
> is rather slow communication link.

I see

> I'm open for discussion.

Yes please, I think I started some bad flamewar in here :/

Best regards,
Marek Vasut
Łukasz Majewski - July 4, 2012, 3:13 p.m.
Hi Marek,

> Dear Lukasz Majewski,
> 
> [...]
> 
> > > > > Holy Moly ... can we not make this into simple calls to those
> > > > > subsystems ? Instead invoking command is crazy ;-)
> > > > 
> > > > Are they really simple?  There's a few other places we do this,
> > > > and so long as it's documented that DFU depends on
> > > > CONFIG_FAT_WRITE for writing to fat and so forth.
> > > 
> > > Well ain't it easier to call fat_write() or similar?
> > 
> > I've decided to use run_command on a purpose.
> > 
> > This call provides clean and reliable API. It is very unlikely that
> > the mmc write <dev> <addr> <start> <size> command will change (or
> > any other).
> > On the other hand the fields of struct mmc are changed from time to
> > time.
> 
> I'm afraid it might change with the driver model soon.
You have probably more information than I about the driver model :-)

Since I know u-boot this API was stable. If it changes, I will adjust
the sprintf :-)

> 
> > Moreover, mmc drivers are also a subject to change (like adding
> > dw_mmc recently).
> > Using run_command also takes the burden of mmc_init() related calls.
> > 
> > Of course the run_command's downside is the speed of execution. But
> > is it so important when one considers, the firmware update?
> 
> But as Stephen pointed out, the type checking is much better when
> used as function.
Yes, I agree about the type check. 

Contrary, the cmd_mmc.c code is not checking the correctness of passed
data. It performs strncmp, then simple_strtoul and with this parameter
calls 
mmc->block_dev.block_read(). For this command such behavior is
acceptable.


> 
> > Side note: DFU uses only EP0 (for transfer and configuration), so
> > this is rather slow communication link.
> 
> I see
> 
> > I'm open for discussion.
> 
> Yes please, I think I started some bad flamewar in here :/

Maybe we come up with a better solution thanks to that :-).
Mike Frysinger - July 20, 2012, 4:25 a.m.
On Tuesday 03 July 2012 20:13:37 Marek Vasut wrote:
> Dear Tom Rini,
> > On Wed, Jul 04, 2012 at 12:31:14AM +0200, Marek Vasut wrote:
> > > Dear Tom Rini,
> > > > > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > > > > +	run_command(cmd_buf, 0);
> > > > > > > 
> > > > > > > Holy Moly ... can we not make this into simple calls to those
> > > > > > > subsystems ? Instead invoking command is crazy ;-)
> > > > > > 
> > > > > > Are they really simple?  There's a few other places we do this,
> > > > > > and so long as it's documented that DFU depends on
> > > > > > CONFIG_FAT_WRITE for writing to fat and so forth.
> > > > > 
> > > > > Well ain't it easier to call fat_write() or similar?
> > > > 
> > > > Assuming that most of the logic in do_fat_fswrite is needed, no.  And
> > > > I think a good portion of it is, at first glance at least.
> > > 
> > > Abstracting it out into a function won't cut it?
> > 
> > My inclination would be to say that seems a bit sillier than just using
> > run_command(...) to call the existing command.
> 
> Whose's syntax will likely soon change, breaking all this :-(

and would be pretty much impossible to detect at compile time.  extending the 
API for code to call is the right answer ... abusing the shell at runtime from 
other code should be an absolute last resort.
-mike

Patch

diff --git a/drivers/dfu/Makefile b/drivers/dfu/Makefile
index 7736485..7b717bc 100644
--- a/drivers/dfu/Makefile
+++ b/drivers/dfu/Makefile
@@ -26,6 +26,7 @@  include $(TOPDIR)/config.mk
 LIB	= $(obj)libdfu.o
 
 COBJS-$(CONFIG_DFU_FUNCTION) += dfu.o
+COBJS-$(CONFIG_DFU_MMC) += dfu_mmc.o
 
 SRCS    := $(COBJS-y:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS-y))
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
new file mode 100644
index 0000000..3151fbc
--- /dev/null
+++ b/drivers/dfu/dfu_mmc.c
@@ -0,0 +1,126 @@ 
+/*
+ * dfu.c -- DFU back-end routines
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * authors: Andrzej Pietrasiewicz <andrzej.p@samsung.com>
+ *	    Lukasz Majewski <l.majewski@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <dfu.h>
+
+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:
+		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__);
+	}
+
+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+	run_command(cmd_buf, 0);
+
+	return 0;
+}
+
+int dfu_read_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
+{
+	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);
+	char *str_env = NULL;
+	int ret = 0;
+
+	memset(cmd_buf, '\0', sizeof(cmd_buf));
+
+	switch (dfu->layout) {
+	case RAW_ADDR:
+		sprintf(cmd_buf, "mmc read 0x%x %x %x", (unsigned int) buf,
+			dfu->data.mmc.lba_start, dfu->data.mmc.lba_size);
+
+		*len = dfu->data.mmc.lba_blk_size * dfu->data.mmc.lba_size;
+		break;
+	case FAT:
+		sprintf(cmd_buf, "fatload mmc %d:%d 0x%x %s",
+			dfu->data.mmc.dev, dfu->data.mmc.part,
+			(unsigned int) buf, dfu->name);
+		break;
+	default:
+		printf("%s: Wrong layout!\n", __func__);
+	}
+
+	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
+
+	ret = run_command(cmd_buf, 0);
+	if (ret) {
+		puts("dfu: Read error!\n");
+		return ret;
+	}
+
+	if (dfu->layout != RAW_ADDR) {
+		str_env = getenv("filesize");
+		if (str_env == NULL) {
+			puts("dfu: Wrong file size!\n");
+			return -1;
+		}
+
+		*len = simple_strtoul(str_env, NULL, 16);
+	}
+	return ret;
+}
+
+int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s)
+{
+	char *st = NULL;
+	int n = 0;
+
+	dfu->dev_type = MMC;
+	st = dfu_extract_token(&s, &n);
+
+	if (!strncmp(st, "mmc", n)) {
+		dfu->layout = RAW_ADDR;
+
+		dfu->data.mmc.lba_start = simple_strtoul(s, &s, 16);
+		dfu->data.mmc.lba_size = simple_strtoul(++s, &s, 16);
+		dfu->data.mmc.lba_blk_size = get_mmc_blk_size(dfu->dev_num);
+
+	} else if (!strncmp(st, "fat", n)) {
+		dfu->layout = FAT;
+
+		dfu->data.mmc.dev = simple_strtoul(s, &s, 10);
+		dfu->data.mmc.part = simple_strtoul(++s, &s, 10);
+
+	} else {
+		printf("%s: Wrong memory layout!\n", __func__);
+	}
+
+	dfu->read_medium = dfu_read_medium_mmc;
+	dfu->write_medium = dfu_write_medium_mmc;
+
+	return 0;
+}