Re: [Qemu-devel] [PATCH 1/3] sh4: mmio based CF support on r2d board.

[takasi-y]

Thank you for your review.

> I'd add a space between if and (.
...
> Please add a space between the comma after opaque and target_phys_addr_t.
OK. I fix the styles.

> It would be better to make the init function take instead of mmio
> pointer, two target_phys_addr_t parameters and do the physical memory
> registration there.

I wrote it because I thought it was simple and flexible.
But, after browsing other device init functions, I understand "callee register"
 is basic procedure in this program.

Looking at isa_ide_init()
# void isa_ide_init(int iobase, int iobase2, qemu_irq irq,
#                   BlockDriverState *hd0, BlockDriverState *hd1);
as a reference, I changed the function args like following.

void mmio_ide_init(target_phys_addr_t membase, target_phys_addr_t membase2,
                   qemu_irq irq, int shift,
                   BlockDriverState *hd0, BlockDriverState *hd1)
This one is not bad at all.

> This should be in a .h file (maybe pc.h), without "extern" and the
> parameter names should be included.
Sure. But, I wonder which file is suitable. 
The system I accustomed to is like
- ide.c define function
- ide.h declare prototype
- pc.c include ide.h and refer the function.
But in QEMU, it looks like
- ide.c define function
- pc.h declare prototype
- pc.c include pc.h and refer the function.
Is it mean the prototype declaration should go into new file "r2d.h" ?

Cheers,
/yoshii