Re: [Qemu-devel] [PATCH 1/3] hw/usb/hcd-ohci.c: Move sysbus and PCI code to new files

["Andreas Färber" <afaerber@suse.de>]

Am 23.08.2013 03:18, schrieb Ákos Kovács:
> Move the existing sysbus and PCI logic to hcd-ohci-sysbus.c and
> hcd-ohci-pci.c from hcd-ohci.c. Create a new hcd-ohci.h header for the
> shared declarations and macros.
> 
> Signed-off-by: Ákos Kovács <akoskovacs@gmx.com>
> ---
>  hw/usb/hcd-ohci-pci.c    |   96 ++++++++++
>  hw/usb/hcd-ohci-sysbus.c |   84 +++++++++
>  hw/usb/hcd-ohci.c        |  434 +++-------------------------------------------
>  hw/usb/hcd-ohci.h        |  311 +++++++++++++++++++++++++++++++++
>  4 files changed, 514 insertions(+), 411 deletions(-)
>  create mode 100644 hw/usb/hcd-ohci-pci.c
>  create mode 100644 hw/usb/hcd-ohci-sysbus.c
>  create mode 100644 hw/usb/hcd-ohci.h
> 
> diff --git a/hw/usb/hcd-ohci-pci.c b/hw/usb/hcd-ohci-pci.c
> new file mode 100644
> index 0000000..ac50951
> --- /dev/null
> +++ b/hw/usb/hcd-ohci-pci.c
> @@ -0,0 +1,96 @@
> +/*
> + * QEMU USB OHCI Emulation
> + * Copyright (c) 2004 Gianni Tedesco
> + * Copyright (c) 2006 CodeSourcery
> + * Copyright (c) 2006 Openedhand Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * TODO:
> + *  o Isochronous transfers
> + *  o Allocate bandwidth in frames properly
> + *  o Disable timers when nothing needs to be done, or remove timer usage
> + *    all together.
> + *  o BIOS work to boot from USB storage
> +*/
[...]
> diff --git a/hw/usb/hcd-ohci-sysbus.c b/hw/usb/hcd-ohci-sysbus.c
> new file mode 100644
> index 0000000..b9d15db
> --- /dev/null
> +++ b/hw/usb/hcd-ohci-sysbus.c
> @@ -0,0 +1,84 @@
> +/*
> + * QEMU USB OHCI Emulation
> + * Copyright (c) 2004 Gianni Tedesco
> + * Copyright (c) 2006 CodeSourcery
> + * Copyright (c) 2006 Openedhand Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * TODO:
> + *  o Isochronous transfers
> + *  o Allocate bandwidth in frames properly
> + *  o Disable timers when nothing needs to be done, or remove timer usage
> + *    all together.
> + *  o BIOS work to boot from USB storage
> +*/
[...]
> diff --git a/hw/usb/hcd-ohci.h b/hw/usb/hcd-ohci.h
> new file mode 100644
> index 0000000..8be00f2
> --- /dev/null
> +++ b/hw/usb/hcd-ohci.h
> @@ -0,0 +1,311 @@
> +/*
> + * QEMU USB OHCI Emulation
> + * Copyright (c) 2004 Gianni Tedesco
> + * Copyright (c) 2006 CodeSourcery
> + * Copyright (c) 2006 Openedhand Ltd.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> + *
> + * TODO:
> + *  o Isochronous transfers
> + *  o Allocate bandwidth in frames properly
> + *  o Disable timers when nothing needs to be done, or remove timer usage
> + *    all together.
> + *  o BIOS work to boot from USB storage
> +*/

Is it really a good idea to duplicate the TODO list into four files?
I assume points 1-3 affect the core code that is not moving, so that all
four points could stay in hcd-ohci.c. No one is going to remember to
sync it otherwise.

Also there should probably be a space before */ for alignment. :)

When I create new files I usually add an up-to-date copyright line, not
sure if that's applicable here, depending on amount of changes.

The split itself seems like a good idea, thanks.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg