qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v02] add 1394 bus support


From: Itamar Tal
Subject: Re: [Qemu-devel] [PATCH v02] add 1394 bus support
Date: Mon, 20 Apr 2015 14:59:14 +0300



On Sun, Apr 19, 2015 at 8:21 PM, Andreas Färber <address@hidden> wrote:
Hi,

Am 19.04.2015 um 18:38 schrieb Itamar Tal:
> I've set it just after the subject field in the patch? Should I add it
> somewhere else and resubmit?

Please compare other QEMU or Linux patches: It needs to go before ---
into the commit message. git commit --amend -s will place it correctly.

OK - I'll do that 

BTW I'm missing a type_init() and type_register_static() in the bottom
of hcd-ohci.c and wonder how you managed to test it without.
 I test it adding the following commands the command line
-chardev socket,host=127.0.0.1,port=13940,nodelay,id=fw0 -device ohci-1394,chardev=fw0

which creates a new ohci-1394 device which I can work with and connect between machines.

Did you copy this code from some other codebase? For one, hcd-ohci.c has
"address@hidden", which looks like you copied it from some mailing list
archive (then you need to replicate those authors' Signed-off-bys), and
for another the Coding Style does not exactly match that of QEMU, in
particular you are using a lot of custom _t types whereas QEMU uses
CamelCase type names. BTW if you consistently used typedefs for your
unions, you could avoid those macros declaring phy etc. unions inline, no?

- I've copied the patch from the last reply I've sent to qemu-dev so the address is hidden.
- I ran checkpatch.pl to align the code with the QEMU code before submitting (no errors/warnings). Other than the XXX_t types, anything else? I'll fix it, no problem

Please also drop the empty last line in the new files you add. Did you
run scripts/checkpatch.pl? Running git-am on a clean branch can also
help highlight whitespace errors.
- Did checkpatch.pl. I'll also try git-am.

hcd-ohci.h is missing a license/copyright header.
- OK, I'll add it 

hcd_pci_exit() has commented-out code - please fix or remove.
- no problem
hcd_pci_init() has the opening brace placed wrong.
- no problem

It looks like a generic PCI device, so why are you placing the config
option into i386 and x86_64 configs only rather than pci.mak?
- I've followed the suggestion by made by Thomas Huth, what do you suggest? When trying to build it with other architectures, sometimes it worked but many times I was missing some qemu dependencies APIs.

Regards,
Andreas

P.S. Please don't top-post.
- gotcha 

> On Apr 19, 2015 6:47 PM, "Andreas Färber" <address@hidden
> <mailto:address@hidden>> wrote:
>
>     Am 19.04.2015 um 12:52 schrieb address@hidden
>     <mailto:address@hidden>:
>     > From: Itamar Tal <address@hidden
>     <mailto:address@hidden>>
>     >
>     > ---
>
>     Still no Signed-off-by...
>
>     Regards,
>     Andreas
>
>     --
>     SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>     GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
>     Graham Norton; HRB 21284 (AG Nürnberg)
>


--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)


reply via email to

[Prev in Thread] Current Thread [Next in Thread]