Re: [Qemu-devel] [RESEND][PATCH 2/9] PCMCIA: start qdev'ication

[Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>]

On 5/16/11, andrzej zaborowski <balrogg@gmail.com> wrote:
> Hi Dmitry,
>
> On 25 April 2011 11:06, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
> wrote:
>> Convert PCMCIA bus handling code to use QBus internally.
>> MicroDrive code is still unaffected.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
>> ---
>>  Makefile.objs      |    3 ++
>>  hw/pcmcia.c        |  102
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/pcmcia.h        |   15 +++++++-
>>  hw/pxa2xx_pcmcia.c |    2 +-
>>  vl.c               |   43 ----------------------
>>  5 files changed, 120 insertions(+), 45 deletions(-)
>>  create mode 100644 hw/pcmcia.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index 44ce368..153a148 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -289,6 +289,9 @@ hw-obj-$(CONFIG_REALLY_VIRTFS) += virtio-9p-debug.o
>>  hw-obj-$(CONFIG_VIRTFS) += virtio-9p-local.o virtio-9p-xattr.o
>>  hw-obj-$(CONFIG_VIRTFS) += virtio-9p-xattr-user.o virtio-9p-posix-acl.o
>>
>> +# PCMCIA
>> +hw-obj-y += pcmcia.o
>> +
>>  ######################################################################
>>  # libdis
>>  # NOTE: the disassembler code is only needed for debugging
>> diff --git a/hw/pcmcia.c b/hw/pcmcia.c
>> new file mode 100644
>> index 0000000..17a49b6
>> --- /dev/null
>> +++ b/hw/pcmcia.c
>> @@ -0,0 +1,102 @@
>> +/*
>> + * QEMU System Emulator
>> + * PCMCIA subsystem
>> + *
>> + * Copyright (c) 2003-2008 Fabrice Bellard
>> + * Copyright (c) 2011 Dmitry Eremin-Solenikov
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining
>> a copy
>> + * of this software and associated documentation files (the "Software"),
>> to deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
>> sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
>> IN
>> + * THE SOFTWARE.
>> + */
>> +#include "hw.h"
>> +#include "pcmcia.h"
>> +#include "monitor.h"
>> +
>> +/***********************************************************/
>> +/* PCMCIA/Cardbus */
>> +
>> +static struct pcmcia_socket_entry_s {
>> +    PCMCIASocket *socket;
>> +    struct pcmcia_socket_entry_s *next;
>> +} *pcmcia_sockets = 0;
>> +
>> +static BusInfo pcmcia_bus_info = {
>> +    .name       = "PCMCIA",
>> +    .size       = sizeof(PCMCIASocket),
>> +};
>> +
>> +void pcmcia_socket_register(PCMCIASocket *socket, DeviceState *parent)
>> +{
>> +    struct pcmcia_socket_entry_s *entry;
>> +
>> +    qbus_create_inplace(&socket->qbus, &pcmcia_bus_info,
>> +                    parent, "pcmcia");
>> +
>> +    entry = qemu_malloc(sizeof(struct pcmcia_socket_entry_s));
>> +    entry->socket = socket;
>> +    entry->next = pcmcia_sockets;
>> +    pcmcia_sockets = entry;
>> +}
>> +
>> +void pcmcia_socket_unregister(PCMCIASocket *socket)
>> +{
>> +    struct pcmcia_socket_entry_s *entry, **ptr;
>> +
>> +    ptr = &pcmcia_sockets;
>> +    for (entry = *ptr; entry; ptr = &entry->next, entry = *ptr)
>> +        if (entry->socket == socket) {
>> +            *ptr = entry->next;
>> +            qemu_free(entry);
>> +        }
>
> I think you need a "break" here, or something else.  Otherwise you'll
> be accessing entry->next incorrectly.  (I notice that this error is
> present in the current code too)

OK, will fix it.

> It would also be great if someone with understanding of qbus could ack
> the rest of this patch.

/me is thinking about restructurisation of PCMCIA patchset, so anyway...

> Out of curiosity, In the patch 1/9 can you include pxa.h instead of
> moving the declaration out of pxa.h?

Yes and no. Yes, because it´s possible. No, because I´d like not to pollute
the StrongARM namespace. Anyway, I´m thinking about just copying this
pxa_pcmcia_init to strongarm.c to be able to customize socket naming.

> I also want to remind you that (iirc) the tosa code was merged in a
> rather incomplete state, like the collie, kind of on the condition
> that it would be worked on for stuff like audio/video soon after ;)

AC97 stuff is in progress. Current status (with the huge pile of other patches)
can be found in my repo on repo.or.cz. Would you like a cleaner branch
or whatever
for review? Current blocking points are the design of audio stream vs.
slots mapping
and PXA dma functions.

I haven´t yet looked on the MCP (for collie), will probably add LCD
and some other
stuff (like StrongARM-specific wait-for-interrupt) first.

-- 
With best wishes
Dmitry