Re: [Qemu-devel] [PATCH 09/14] hw/lan9118.c: Basic byte/word/long access support.

[Peter Maydell]

On 7 December 2011 10:58, Evgeny Voevodin <address@hidden> wrote:
> On 12/07/2011 02:09 PM, Peter Maydell wrote:
>>
>> On 7 December 2011 09:47, Evgeny Voevodin<address@hidden>  wrote:
>>>
>>> We included this chip into s5pc210 platform because SMDK board holds
>>> lan9215 chip. Difference is that 9215 access is 16-bit wide and some
>>> registers differ. By addition basic 16-bit access to 9118 emulation we
>>> achieved ethernet controller support by Linux lernel on SMDK boards.
>>
>>
>> If it differs then shouldn't we add a new qdev device for 9215 ?
>> (sharing most of the implementation code, obviously)
>>
>
> This patch could be interpreted as lan9118 emulation expansion since this
> chip supports 16-bit access too. These changes don't cover all the
> difference between 9118 and 9215, but it's enough to provide network support
> to Samsung boards. When 9215 support will be added we can easily switch to
> this chip.

If you're adding a legitimate missing feature (16 bit access support) to
lan9118 then your commit message should say so, not talk about 9215.
The right place to say "this should be a 9215 but the 9118 is close enough"
is in the patch adding the network chip to the board model, not in the
commit adding a feature to the network chip model.

We should probably make 16 vs 32 bit be a qdev property of lan9118
(corresponding to the way the hardware is configured by an input pin
on startup) and reflect this properly in the HW_CFG register.

Also, this todo comment is too obscure:
+    /* TODO: Implement fair word operation, because this implementation
+     * assumes that any register is accessed as two 16-bit operations. */

I have no idea what it means.

-- PMM