Re: [Qemu-devel] [PATCH 4/4] i8254: convert to qdev

[Blue Swirl]

On Sun, Mar 6, 2011 at 6:31 PM, Anthony Liguori <address@hidden> wrote:
> On 02/13/2011 03:10 PM, Blue Swirl wrote:
>>
>> Convert to qdev. Don't expose PITState.
>>
>> Signed-off-by: Blue Swirl<address@hidden>
>> ---
>>  hw/i8254.c         |   61
>> +++++++++++++++++++++++++++++++++++++--------------
>>  hw/mips_fulong2e.c |    4 +-
>>  hw/mips_jazz.c     |    4 +-
>>  hw/mips_malta.c    |    4 +-
>>  hw/mips_r4k.c      |    4 +-
>>  hw/pc.c            |    5 +--
>>  hw/pc.h            |   25 ++++++++++++++------
>>  hw/pcspk.c         |    4 +-
>>  hw/ppc_prep.c      |    4 +-
>>  9 files changed, 75 insertions(+), 40 deletions(-)
>>
>> diff --git a/hw/i8254.c b/hw/i8254.c
>> index 06b225c..680caab 100644
>> --- a/hw/i8254.c
>> +++ b/hw/i8254.c
>> @@ -53,9 +53,12 @@ typedef struct PITChannelState {
>>      qemu_irq irq;
>>  } PITChannelState;
>>
>> -struct PITState {
>> +typedef struct PITState {
>> +    ISADevice dev;
>>
>
> The PIT is not an ISA device.  Modelling it as such is worse than leaving it
> unmodelled.

No. These days, PIT is part of Super I/O chip, which is accessed via
LPC bus. LPC is from software point of view equal to ISA. Therefore,
in absence of LPC, ISA is correct.

>> -PITState *pit_init(int base, qemu_irq irq);
>> -void pit_set_gate(PITState *pit, int channel, int val);
>> -int pit_get_gate(PITState *pit, int channel);
>> -int pit_get_initial_count(PITState *pit, int channel);
>> -int pit_get_mode(PITState *pit, int channel);
>> -int pit_get_out(PITState *pit, int channel, int64_t current_time);
>> +void pit_set_gate(ISADevice *dev, int channel, int val);
>> +int pit_get_gate(ISADevice *dev, int channel);
>> +int pit_get_initial_count(ISADevice *dev, int channel);
>> +int pit_get_mode(ISADevice *dev, int channel);
>> +int pit_get_out(ISADevice *dev, int channel, int64_t current_time);
>>
>
> Making these functions take an ISADevice hurts type safety.  They should
> take a PITState.

These functions should be removed anyway. For example, pcspk probably
should be merged with i825, or a signal like interface could be used.