Re: [U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function

Hi Simon,

Hi Lukasz,

On 25 July 2015 at 02:11, Lukasz Majewski l.majewski@majess.pl wrote:

...

This function allows writing via DFU data stored from fixed buffer address (like e.g. loadaddr env variable).

Such predefined buffers are used in the update_tftp() code. In fact this function is a wrapper on the dfu_write() and dfu_flush().

Signed-off-by: Lukasz Majewski l.majewski@majess.pl

Changes for v2:

• Use min() macro instead of comparison
• Change definition of left variable to be unsigned long - this

allowed safe usage of min() macro

drivers/dfu/dfu.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ include/dfu.h | 1 + 2 files changed, 49 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

But see nits below.

...

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c index 2267dbf..c83ee41 100644 --- a/drivers/dfu/dfu.c +++ b/drivers/dfu/dfu.c @@ -568,3 +568,51 @@ int dfu_get_alt(char *name)

    return -ENODEV;

}

+/**

• • dfu_write_from_mem_addr - this function adds support for

writing data

• •                       starting from fixed memory address
    

(like $loadaddr)

• •                       to dfu managed medium (e.g. NAND, MMC)
    

I think it's better to start with a direct one-line description, like:

• • dfu_write_from_mem_addr - wite data to dfu-managed medium
• • More detail goes here ...

Ok

...

• • @param dfu - dfu entity to which we want to store data
• • @param buf - fixed memory addres from where data starts
• • @param size - number of bytes to write
• • @return - 0 on success, other value on failure
• */

+int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size)

const void *buf? My understanding is that this buffer is not changed.

In principle I agree with you.

For practical reasons, I would opt for leaving the buf definition as void *, since other functions (like globally visible dfu_write) uses this buffer as void *.

Adding const there would require removing this qualifier in the line [1] since then dp pointer is passed to dfu_write(), which requires void * pointer.

...

+{

•   unsigned long dfu_buf_size, write, left = size;
  

•   int i, ret = 0;
  

•   void *dp = buf;
  

^^^^^^^^^^^^^^^^ [1]

...

•   /*
  

•    * Here we must call dfu_get_buf(dfu) first to be sure that
  

dfu_buf_size

•    * has been properly initialized - e.g. if "dfu_bufsiz" has
  

been taken

•    * into account.
  

•    */
  

•   dfu_get_buf(dfu);
  

•   dfu_buf_size = dfu_get_buf_size();
  

•   debug("%s: dfu buf size: %lu\n", __func__, dfu_buf_size);
  

•   for (i = 0; left > 0; i++) {
  

•           write = min(dfu_buf_size, left);
  

•           debug("%s: dp: 0x%p left: %lu write: %lu\n",
  

__func__,

•                 dp, left, write);
  

•           ret = dfu_write(dfu, dp, write, i);
  

•           if (ret) {
  

•                   error("DFU write failed\n");
  

•                   return ret;
  

•           }
  

•           dp += write;
  

•           left -= write;
  

•   }
  

•   ret = dfu_flush(dfu, NULL, 0, i);
  

•   if (ret)
  

•           error("DFU flush failed!");
  

•   return ret;
  

+} diff --git a/include/dfu.h b/include/dfu.h index 7f78cc2..e624c41 100644 --- a/include/dfu.h +++ b/include/dfu.h @@ -162,6 +162,7 @@ bool dfu_usb_get_reset(void); int dfu_read(struct dfu_entity *de, void *buf, int size, int blk_seq_num); int dfu_write(struct dfu_entity *de, void *buf, int size, int blk_seq_num); int dfu_flush(struct dfu_entity *de, void *buf, int size, int blk_seq_num);

Please can you put your function comment here in the header?

ok

...

+int dfu_write_from_mem_addr(struct dfu_entity *dfu, void *buf, int size); /* Device specific */ #ifdef CONFIG_DFU_MMC extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *devstr, char *s); -- 2.1.4

Regards, Simon

Best regards,

Lukasz Majewski