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

Lukasz Majewski
2012-07-04T15:13:57Z

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 :-).

--
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group