From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Date: Sun, 9 Aug 2015 23:37:09 +0200 Subject: [U-Boot] [PATCH v2 5/9] dfu: tftp: update: Add dfu_write_from_mem_addr() function In-Reply-To: References: <1437811877-13764-1-git-send-email-l.majewski@majess.pl> <1437811877-13764-6-git-send-email-l.majewski@majess.pl> Message-ID: <20150809233709.18e3c60b@jawa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, > Hi Lukasz, > > On 25 July 2015 at 02:11, Lukasz Majewski > 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 > > --- > > 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 > > 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 181 bytes Desc: OpenPGP digital signature URL: