[U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command

[Steve Rae <srae@broadcom.com>]

On 14-07-30 06:37 PM, Marek Vasut wrote:
> On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote:
> [...]
>> +
>> +#include <common.h>
>> +#include <fb_mmc.h>
>> +#include <part.h>
>> +#include <sparse_format.h>
>> +
>> +/* The 64 defined bytes plus \0 */
>> +#define RESPONSE_LEN	(64 + 1)
>> +
>> +static char *response_str;
>
> I'd suggest to pass this "response_str" around instead of making it global.
>

That would involve adding it to fastboot_resp(), which is called 11 
times in this code, from 3 different functions (so would need to add 
this to two of the functions...). And as these evolve, there will likely 
be more nested functions, which would all require "passing it around"....
I think that this "static global pointer" is a cleaner implementation.

>> +static void fastboot_resp(const char *s)
>> +{
>> +	strncpy(response_str, s, RESPONSE_LEN);
>> +	response_str[RESPONSE_LEN - 1] = '\0';
>
> This could be shrunk to a single snprintf(response_str, RESPONSE_LENGTH, s); I
> think, but I'm not sure if the overhead won't grow.
>

snprintf() is used very sparingling in U-Boot, and with the cautionary 
statements in README (line 852) and the fact that CONFIG_SYS_VSNPRINTF 
is not defined for armv7 builds, I am not going to use it....

>> +}
>> +
>> +static int is_sparse_image(void *buf)
>> +{
>> +	sparse_header_t *s_header = (sparse_header_t *)buf;
>> +
>> +	if ((le32_to_cpu(s_header->magic) == SPARSE_HEADER_MAGIC) &&
>> +	    (le16_to_cpu(s_header->major_version) == 1))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> +static void write_sparse_image(block_dev_desc_t *dev_desc,
>> +		disk_partition_t *info, const char *part_name,
>> +		void *buffer, unsigned int download_bytes)
>> +{
>> +	lbaint_t blk;
>> +	lbaint_t blkcnt;
>> +	lbaint_t blks;
>> +	sparse_header_t *s_header = (sparse_header_t *)buffer;
>> +	chunk_header_t *c_header;
>> +	void *buf;
>> +	uint32_t blk_sz;
>> +	uint32_t remaining_chunks;
>> +	uint32_t bytes_written = 0;
>> +
>> +	blk_sz = le32_to_cpu(s_header->blk_sz);
>> +
>> +	/* verify s_header->blk_sz is exact multiple of info->blksz */
>> +	if (blk_sz != (blk_sz & ~(info->blksz - 1))) {
>> +		printf("%s: Sparse image block size issue [%u]\n",
>> +		       __func__, blk_sz);
>> +		fastboot_resp("FAILsparse image block size issue");
>
> Can't you just make the fastboot_resp() function a variadic one AND move the
> printf() into the fastboot_resp() function? You could then even get consistent
> output on both the device and in the response if you snprintf() into the
> response_str first and then printf() the response_str .
>

Generally, the printf() statements which are sent to the console, and 
the fastboot_resp() statements which are sent to the host running the 
"fastboot" application are not the same....

>> +		return;
>> +	}
>
> [...]
>
>> +static void write_raw_image(block_dev_desc_t *dev_desc, disk_partition_t
>> *info, +		const char *part_name, void *buffer,
>> +		unsigned int download_bytes)
>> +{
>> +	lbaint_t blkcnt;
>> +	lbaint_t blks;
>> +
>> +	/* determine number of blocks to write */
>> +	blkcnt = ((download_bytes + (info->blksz - 1)) & ~(info->blksz - 1));
>> +	blkcnt = blkcnt / info->blksz;
>> +
>> +	if (blkcnt > info->size) {
>> +		printf("%s: too large for partition: '%s'\n", __func__,
>> +		       part_name);
>> +		fastboot_resp("FAILtoo large for partition");
>> +		return;
>> +	}
>> +
>> +	printf("Flashing Raw Image\n");
>
> Use puts() here and everywhere where printf() is not taking any args please.

done in v4 - Thanks!

>
>> +	blks = dev_desc->block_write(dev_desc->dev, info->start, blkcnt,
>> +				     buffer);
>> +	if (blks != blkcnt) {
>> +		printf("%s: failed writing to device %d\n", __func__,
>> +		       dev_desc->dev);
>> +		fastboot_resp("FAILfailed writing to device");
>> +		return;
>> +	}
>> +
>> +	printf("........ wrote " LBAFU " bytes to '%s'\n", blkcnt * info->blksz,
>> +	       part_name);
>> +	fastboot_resp("OKAY");
>> +}
> [...]
>

Thanks, Steve