[U-Boot] [PATCH 1/3] usb, dfu: extract flush code into seperate function

[Lukasz Majewski]

Hi Heiko,

I'm also CCing Pantelis to the loop.

> Hello Marek,
> 
> Am 12.03.2014 12:43, schrieb Marek Vasut:
> > On Wednesday, March 12, 2014 at 11:01:19 AM, Heiko Schocher wrote:
> >> move the flushing code into an extra function dfu_flush(),
> >> so it can be used from other code.
> >>
> >> Signed-off-by: Heiko Schocher<hs at denx.de>
> >> Cc: Lukasz Majewski<l.majewski at samsung.com>
> >> Cc: Kyungmin Park<kyungmin.park at samsung.com>
> >> Cc: Marek Vasut<marex at denx.de>
> >> ---
> >>   drivers/dfu/dfu.c | 46
> >> ++++++++++++++++++++++++++-------------------- include/dfu.h
> >> |  1 + 2 files changed, 27 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> >> index 56e69fd..193e047 100644
> >> --- a/drivers/dfu/dfu.c
> >> +++ b/drivers/dfu/dfu.c
> >> @@ -127,6 +127,31 @@ static int dfu_write_buffer_drain(struct
> >> dfu_entity *dfu) return ret;
> >>   }
> >>
> >> +int dfu_flush(struct dfu_entity *dfu, void *buf, int size, int
> >> blk_seq_num) +{
> >> +	int ret = 0;
> >> +
> >> +	/* end? */
> >
> > What does this comment mean ? I don't understand it ...
> 
> Comes from original code... Thinking about it, it seems better to
> let this comment and the below "if" in the dfu_write function...

This comment doesn't bring any meaningful information, so can be
removed.

> 
> >> +	if (size == 0) {
> >
> > You can check this like so:
> >
> > if (size)
> >    return;
> 
> ... as when moving this "if" back to dfu_write(), this "if" kindly
> disappears in the patch 2/3 of this series, as calling dfu_flush()
> is only in the new dfuMANIFEST state necessary, which is called at
> the end of the dfu transfer, so no need for checking, if end of
> size is reached!

So, in my opinion, this if can be safely removed.

> 
> >
> > code
> > code
> > code
> >
> > This would cut down the indent, no ?
> 
> Yes. Did the above change and this "if" is no longer necessary ;-)
> 
> I will wait for comments from Lukasz before posting v2.
> 
> > Please let's fix this when this popped up, either in separate patch
> > or wrap it into this one.
> >
> >> +		/* Now try and flush to the medium if needed. */
> >> +		if (dfu->flush_medium)
> >> +			ret = dfu->flush_medium(dfu);
> >> +		printf("\nDFU complete CRC32: 0x%08x\n",
> >> dfu->crc); +
> >> +		/* clear everything */
> >> +		dfu_free_buf();
> >> +		dfu->crc = 0;
> >> +		dfu->offset = 0;
> >> +		dfu->i_blk_seq_num = 0;
> >> +		dfu->i_buf_start = dfu_buf;
> >> +		dfu->i_buf_end = dfu_buf;
> >> +		dfu->i_buf = dfu->i_buf_start;
> >> +
> >> +		dfu->inited = 0;
> >> +	}
> >> +	return ret;
> >> +}
> > [...]
> 
> Thanks!
> 
> bye,
> Heiko

BTW: Thanks for proper extension of the DFU state machine in u-boot :-).

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group