[coreboot] [PATCH] flashrom: Group probe function together with associated IDs

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue Dec 9 10:51:37 CET 2008


On 09.12.2008 09:14, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
>   
>> The flash probe function and the IDs for that specific probe function
>> belong together. Group them together on a single line.
>> We already have chips with multiple probe functions and multiple IDs.
>> This patch is the first step in consolidating the present way of
>> creating a new chip definition for each ID of a given chip into multiple
>> IDs per chip definition.
>>
>> Attached for Gmail users.
>>   
>>     
> Thunderbird users also love this.
>
>   
>> -	{"AMD", "Am29F002(N)BB", AMD_ID, AM_29F002BB,
>> +	{"AMD", "Am29F002(N)BB",
>> +	 AMD_ID, AM_29F002BB, probe_jedec,
>>   
>>     
> Please leave the chip name and ID on the same line. Once we fixed
> flashrom to use the non-hidden IDs here, that'll be a very convenient
> way for associating IDs and chips.
>   
>>  	 256, 256,
>>  	 TEST_UNTESTED,
>> -	 probe_jedec, erase_chip_jedec, write_en29f002a
>> +	 erase_chip_jedec, write_en29f002a 
>>  	},
>>   
>>     
> I think function pointers should not be intercepted by the sized above.
> This patch obfuscates the code.
>   

How do you propose to handle chips which have multiple different IDs and
multiple different probe functions? I hope you're not seriously
suggesting something like this:

{"Vendor", "Chip", 0x01, 0x23, 0x45, 0x56, 0x78, 0x9a,
256, 256,
TEST_UNTESTED,
probe_spi_rdid, probe_spi_rems, probe_spi_res, erase_spi_foo, write_spi,
read_spi
}

If we picked that layout, it would be really difficult to find out which
ID belongs to which probe function.

And having 3 entries for almost all SPI flash chips is definitely not
the way to go. Multiply that with the number of erase functions (up to 5
for some SPI chips) and you have 15 entries per real chip. Sure, I can
generate such entries, but people are going to hate that.

If the problem is not obvious, I'll gladly send a patch to illustrate my
point.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/





More information about the coreboot mailing list