On Sun, Apr 19, 2015 at 8:21 PM, Andreas Färber <afaer...@suse.de> 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" <afaer...@suse.de > > <mailto:afaer...@suse.de>> wrote: > > > > Am 19.04.2015 um 12:52 schrieb itamar.t...@gmail.com > > <mailto:itamar.t...@gmail.com>: > > > From: Itamar Tal <ita...@guardicore.com > > <mailto:ita...@guardicore.com>> > > > > > > --- > > > > 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) >
