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

[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.


>  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.

Regards,
Carl-Daniel

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