[flashrom] [PATCH 04/12] use the max_data_read field of the new spi_programmer struct to simplify run_opcode.

[Carl-Daniel Hailfinger]

Am 11.06.2011 21:04 schrieb Carl-Daniel Hailfinger:
> Am 09.06.2011 06:44 schrieb Stefan Tauner:
>   
>> On Thu, 09 Jun 2011 00:56:08 +0200
>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>>
>>   
>>     
>>> Am 08.06.2011 04:55 schrieb Stefan Tauner:
>>>     
>>>       
>>>> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>>>> ---
>>>>  ichspi.c |   41 +++++++++++++++++------------------------
>>>>  1 files changed, 17 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/ichspi.c b/ichspi.c
>>>> index 12727d1..dff6f32 100644
>>>> --- a/ichspi.c
>>>> +++ b/ichspi.c
>>>> @@ -913,37 +913,30 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
>>>>  static int run_opcode(OPCODE op, uint32_t offset,
>>>>  		      uint8_t datalength, uint8_t * data)
>>>>  {
>>>> +	uint8_t maxlength = spi_programmer->max_data_read;
>>>>   
>>>>       
>>>>         
>>> Mh. I see what you're doing here, and it makes sense, but a comment
>>> would be appreciated in the code:
>>> /* The maximum data length is identical on ICH/VIA for the maximum read
>>> length and for the maximum write length without opcode and address.
>>> Opcode and address are stored in separate registers, not in the data
>>> registers. The only exception applies if the opcode definition
>>> (un)intentionally classifies said opcode incorrectly as non-address
>>> opcode or vice versa. */
>>>
>>> Any suggestions on how to improve that comment?
>>>     
>>>       
>> i am not sure if this is the right place to put the second half of that
>> comment, because i dont really see the relevance inside that function:
>> it just delegates its inputs.
>> OTOH it makes sense to have it at a place where both versions share
>> code.
>> what about putting "The maximum payload
>>     
> I don't really like the word "payload". It has a totally different
> meaning in the coreboot world.
>
>   
>>  lengths for read and write
>> operations without opcode and address are identical on ICH/VIA - using
>> max_data_read arbitrarily here."
>>     
> That description is incomplete and (for me) slightly confusing.
>   

Admittedly that's my fault because my text wasn't good either and you
tried to stay close to it.

New suggested text for ich_spi_send_command (your location):

The maximum data length is identical for the maximum read length and
for the maximum write length excluding opcode and address.
Opcode and address are stored in separate registers, not in the data
registers and are thus not counted towards data length. The only
exception applies if the opcode definition (un)intentionally classifies
said opcode incorrectly as non-address opcode or vice versa.



>>  there and the rest inside
>> ich_spi_send_command in the "/* translate read/write array/count */"
>> comment-block?
>>   
>>     
> Excellent idea, that location is where the _whole_ comment should live.
> It doesn't belong in run_opcode() anyway.
>   

Followup, pasted from IRC:
Maybe add some short blurb like "max_data_read == max_data_write for all
Intel/VIA SPI masters" to run_opcode(). That would be readable and short
enough.

Regards,
Carl-Daniel

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