Re: [PATCH v3 4/4] commands: Add MMC ext. CSD register tool

[Sascha Hauer <s.hauer@pengutronix.de>]

On Wed, Sep 02, 2015 at 08:28:53AM +0200, Daniel Schultz wrote:
> +#define EXT_CSD_BLOCKSIZE	512
> +
> +/* Access types */
> +#define R		"R"
> +#define RW		"R/W"
> +#define RWaR		"R/W & R"
> +#define RWaRWE		"R/W & R/W/E"
> +#define RWaRWC_P	"R/W & R/W/C_P"
> +#define RWaRWC_PaRWE_P	"R/W, R/W/C_P & R/W/E_P"
> +#define WE		"W/E"
> +#define RWE		"R/W/E"
> +#define RWEaR		"R/W/E & R"
> +#define RWEaRWE_P	"R/W/E & R/W/E_P"
> +#define RWC_P		"R/W/C_P"
> +#define RWE_P		"R/W/E_P"
> +#define WE_P		"W/E_P"
> +
> +#define print_field_caption(reg_name, access_mode)			       \
> +	do {								       \
> +		printf(#reg_name"[%u]:\n", EXT_CSD_##reg_name);		       \
> +		printf("\tValue: %#02x\n", reg[index]);			       \
> +		printf("\tAccess: "access_mode"\n");			       \
> +	} while (false);
> +
> +#define print_field_caption_with_offset(reg_name, offset, access_mode)	       \
> +	do {								       \
> +		printf(#reg_name"[%u]:\n", EXT_CSD_##reg_name + offset);       \
> +		printf("\tValue: %#02x\n", reg[index]);			       \
> +		printf("\tAccess: "access_mode"\n");			       \
> +	} while (false);
> +
> +#define get_field_val(reg_name, offset, mask)				       \
> +	((reg[EXT_CSD_##reg_name] >> offset) & mask)
> +
> +#define get_field_val_with_index(index, offset, mask)			       \
> +	((reg[index] >> offset) & mask)
> +
> +static void print_access_type_key(void)
> +{
> +	printf("\nR:       Read only.\n");
> +	printf("W:       One time programmable and not readable.\n");
> +	printf("R/W:     One time programmable and readable.\n");
> +	printf("W/E:     Multiple writable with value kept after power failure, H/W reset assertion and any CMD0 reset and not readable.\n");
> +	printf("R/W/E:   Multiple writable with value kept after power failure, H/W reset assertion and any CMD0 reset and readable.\n");
> +	printf("R/W/C_P: Writable after value cleared by power failure and HW/rest assertion (the value not cleared by CMD0 reset) and readable.\n");
> +	printf("R/W/E_P: Multiple writable with value reset after power failure, H/W reset assertion and any CMD0 reset and readable.\n");
> +	printf("W/E_P:   Multiple writable with value reset after power failure, H/W reset assertion and any CMD0 reset and not readable.\n\n");

Wouldn't it make sense to explain the meaning of the individual flags
rather than all possible combinations? i.e.:

R: readable
W: One time writable
E: Multiple writable
C_P: Value cleared by power failure
E_P: Value cleared by power failure and CMD0

Also please add some linebreaks in the printfs. You can do multiline
printf like this:

	printf("permanently disable the use of permanent write protection\n"
		"for write protection groups within all the partitions in the\n"
		"user area from the point this bit is set forward. Setting this\n"
		"bit does not impact areas that are already protected\n"

Even better, shorten the descriptions. "permanently disable the use of
permanent write protection\n" should be enough in this case.

s/optimzed/optimized/
s/disbale/disable/

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox