From mboxrd@z Thu Jan  1 00:00:00 1970
From: Lukasz Majewski <l.majewski@samsung.com>
Date: Thu, 02 Aug 2012 15:55:24 +0200
Subject: [U-Boot] [PATCH v3 3/7] dfu: DFU backend implementation
In-Reply-To: <201208011857.31725.vapier@gentoo.org>
References: <1341308291-14663-1-git-send-email-l.majewski@samsung.com>
	<1343716623-8943-1-git-send-email-l.majewski@samsung.com>
	<1343716623-8943-4-git-send-email-l.majewski@samsung.com>
	<201208011857.31725.vapier@gentoo.org>
Message-ID: <20120802155524.004dfabb@amdc308.digital.local>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Mike Frysinger,

> On Tuesday 31 July 2012 02:36:59 Lukasz Majewski wrote:
> > --- /dev/null
> > +++ b/drivers/dfu/dfu.c
> >
> > +static int dfu_find_alt_num(char *s)
> 
> const char *s
Good point.
> 
> > +{
> > +	int i = 0;
> > +
> > +	for (; *s; s++)
> > +		if (*s == ';')
> > +			i++;
> > +
> > +	return ++i;
> > +}

In this function I count how many times the ';' separator appears.
I didn't found proper function at ./lib/string.c.

> 
> looks kind of like:
> 	return (strrchr(s, ';') - s) + 1;

The above code returns position of the last occurrence of the ';'
separator.

> 
> > +int dfu_write(struct dfu_entity *dfu, void *buf, int size, int
> > blk_seq_num) +{
> > +	static unsigned char *i_buf;
> > +	static int i_blk_seq_num;
> > +	long w_size = 0;
> > +	int ret = 0;
> > +
> > +	if (blk_seq_num == 0) {
> > +		memset(dfu_buf, '\0', sizeof(dfu_buf));
> > ...
> > +	memcpy(i_buf, buf, size);
> > +	i_buf += size;
> 
> why bother clearing it ?  since right below we memcpy() in the data
> we care about from buf, i'd skip the memset() completely.
You are right, removed.
> 
> > +int dfu_read(struct dfu_entity *dfu, void *buf, int size, int
> > blk_seq_num) +{
> > +	static unsigned char *i_buf;
> > +	static int i_blk_seq_num;
> > +	static long r_size;
> > +	static u32 crc;
> > +	int ret = 0;
> > +
> > +	if (blk_seq_num == 0) {
> > +		i_buf = dfu_buf;
> > +		memset(dfu_buf, '\0', sizeof(dfu_buf));
> > +		ret = dfu->read_medium(dfu, i_buf, &r_size);
> > +		debug("%s: %s %ld [B]\n", __func__, dfu->name,
> > r_size);
> > +		i_blk_seq_num = 0;
> > +		/* Integrity check (if needed) */
> > +		crc = crc32(0, dfu_buf, r_size);
> > +	}
> 
> same here -- punt the memset()
OK,
> 
> > +static int dfu_fill_entity(struct dfu_entity *dfu, char* s, int
> > alt,
> 
> "char *s", not "char* s"
OK,
> 
> > +int dfu_config_entities(char *env, char *interface, int num)
> > +{
> > +	struct dfu_entity *dfu;
> > +	int i, ret;
> > +	char *s;
> > +
> > +	dfu_alt_num = dfu_find_alt_num(env);
> > +	debug("%s: dfu_alt_num=%d\n", __func__, dfu_alt_num);
> > +
> > +	for (i = 0; i < dfu_alt_num; i++) {
> > +		dfu = calloc(sizeof(struct dfu_entity), 1);
> 
> seems like you can do this in a single call outside of the for loop:
> 	dfu = calloc(sizeof(*dfu), dfu_alt_num);
> 	if (!dfu)
> 		return -1;
> 	for (i = 0; i < dfu_alt_num; i++) {
> 		s = strsep(&env, ";");
> 		ret = dfu_fill_entity(&dfu[i], s, i, interface, num);
> 		if (ret)
> 			return -1;
> 		list_add_tail(&dfu[i].list, &dfu_list);
> 	}
> 

I'd prefer to leave it as it is (since IMHO is more readable) with the
addition of following code:

for (i = 0; i < dfu_alt_num; i++) {
	dfu = calloc(sizeof(struct dfu_entity), 1);
	if (!dfu) {
		dfu_free_entities();
		return -1;
	}
}

> > --- /dev/null
> > +++ b/include/dfu.h
> >
> > +char *dfu_extract_token(char** e, int *n);
> > +extern int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s);
> > +static inline int dfu_fill_entity_mmc(struct dfu_entity *dfu,
> > char* s)
> 
> "char *s", not "char* s"
OK
> -mike



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group