Re: [flashrom] flashrom on PowerPC (002-lh28f008bjt.patch)

[Stefan Tauner]

On Wed, 27 Jul 2011 18:20:19 +0200
Mattias Mattsson  wrote:
> Index: 82802ab.c
>
> --- 82802ab.c	(revision 1396)
> +++ 82802ab.c	(working copy)
> @@ -208,3 +208,56 @@
>
> return 0;
>  }
>
> +int unlock_lh28f008bjt(struct flashchip *flash)
> +{
> chipaddr bios = flash->virtual_memory;
> uint8_t mcfg, bcfg, need_unlock = 0, can_unlock = 0;

mixing initialized and declared-only variables is :(
> int i;
> /* Wait if chip is busy */
> wait_82802ab(flash);
> /* Read identifier codes */
> chip_writeb(0x90, bios);
> /* Read master lock-bit */
> mcfg = chip_readb(bios + 0x3);
> msg_cdbg("master lock is ");
> if (mcfg) {
> msg_cdbg("locked!\n");
> } else {
> msg_cdbg("unlocked!\n");
> can_unlock = 1;
> }
> /* Read block lock-bits, 8 * 8 KiB + 15 * 64 KiB */

although correct, we don't use the IEC prefixes nowhere else...
> for (i = 0; i < flash->total_size * 1024; i+= (i >= (64 * 1024) ? 64 * 1024 : 8 * 1024)) {

hm... line limit. looks very odd at first sight. maybe a while loop
would be better?
> bcfg = chip_readb(bios + i + 2); // read block lock config

/* */?
not that i find this convention necessary...
> msg_cdbg("block lock at %06x is %slocked!\n", i, bcfg ? "" : "un");
> if (bcfg) {
> 	need_unlock = 1;
> }
> }
> /* Reset chip */
> chip_writeb(0xFF, bios);
> /* Unlock: clear block lock-bits, if needed */
> if (can_unlock && need_unlock) {
> msg_cdbg("Unlock: ");
> chip_writeb(0x60, bios);
> chip_writeb(0xD0, bios);
> chip_writeb(0xFF, bios);
> wait_82802ab(flash);
> msg_cdbg("Done!\n");
> }
> /* Error: master locked or a block is locked */
> if (!can_unlock && need_unlock) {
> msg_cerr("At least one block is locked and lockdown is active!\n");
> return -1;
> }
> return 0;
>
> +}

you start messages lower-case and end them with an exclamation mark very
often. we usually do this for errors (or warnings) only.
> Index: flashchips.c
>
> --- flashchips.c	(revision 1396)
> +++ flashchips.c	(working copy)
> @@ -5379,6 +5379,36 @@
>
> {
>    	.vendor		= "Sharp",
> .name		= "LH28F008BJT-BTLZ1",

why the -BTLZ1 suffix?
i have just looked briefly at the LH28F008BJT-BTLZ1 and
LH28F008BJT-BTLZC datasheets, but they seem to be almost identical?
> .bustype	= BUS_PARALLEL,
> .manufacture_id	= SHARP_ID,
> .model_id	= SHARP_LH28F008BJxxPB,

i am not sure about those id names either, but they are not as
important because they are not visible to the user.
> .total_size	= 1024,
> .page_size	= 64 * 1024,
> .tested		= TEST_OK_PREW,
> .probe		= probe_82802ab,
> .probe_timing	= TIMING_ZERO,
> .block_erasers	=
> {
> 	{
> 		.eraseblocks = {
> 			{8 * 1024, 8},
> 			{64 * 1024, 15}
> 		 },
> 		.block_erase = erase_block_82802ab,
> 	}, {
> 		.eraseblocks = { {1024 * 1024, 1} },
> 		.block_erase = erase_sector_49lfxxxc,
> 	}
> },
> .unlock		= unlock_lh28f008bjt,
> .write		= write_82802ab,
> .read		= read_memmapped,
> .voltage	= {2700, 3600},
> },
> {
> .vendor		= "Sharp",
> .name		= "LHF00L04",
> .bustype	= BUS_FWH, /* A/A Mux */
> .manufacture_id	= SHARP_ID,
> Index: chipdrivers.h
>
> --- chipdrivers.h	(revision 1396)
> +++ chipdrivers.h	(working copy)
> @@ -83,6 +83,7 @@
>  void print_status_82802ab(uint8_t status);
>  int unlock_82802ab(struct flashchip *flash);
>  int unlock_28f004s5(struct flashchip *flash);
> +int unlock_lh28f008bjt(struct flashchip *flash);
>
> /* jedec.c */
>  uint8_t oddparity(uint8_t val);