qemu-ppc
[Top][All Lists]
Advanced

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

Re: [Qemu-ppc] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private


From: BALATON Zoltan
Subject: Re: [Qemu-ppc] [PATCH v2 2/8] ppc4xx_i2c: Move register state to private struct and remove unimplemented sdata and intr registers
Date: Fri, 8 Jun 2018 11:20:50 +0200 (CEST)
User-agent: Alpine 2.21 (BSF 202 2017-01-01)

On Fri, 8 Jun 2018, David Gibson wrote:
On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <address@hidden>

It's not clear to me why this is preferable to having the registers
embedded in the state structure.  The latter is pretty standard
practice for qemu.

Maybe it will be clearer after the next patch in the series. I needed a place to store the bitbang_i2c_interface for the directcntl way of accessing the i2c bus but I can't include bitbang_i2c.h from the public header because it's a local header. So I needed a local extension to the state struct. Once I have that then it's a good place to also store private registers which are now defined in the same file so I don't have to look up them in a different place. This seemed clearer to me and easier to work with. Maybe the spliting of the rewrite did not make this clear.

One thing I'm not sure about though:

---
 hw/i2c/ppc4xx_i2c.c         | 75 +++++++++++++++++++++++++--------------------
 include/hw/i2c/ppc4xx_i2c.h | 19 ++----------
 2 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index d1936db..a68b5f7 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
[...]
@@ -330,7 +335,9 @@ static const MemoryRegionOps ppc4xx_i2c_ops = {
 static void ppc4xx_i2c_init(Object *o)
 {
     PPC4xxI2CState *s = PPC4xx_I2C(o);
+    PPC4xxI2CRegs *r = g_malloc0(sizeof(PPC4xxI2CRegs));

+    s->regs = r;
     memory_region_init_io(&s->iomem, OBJECT(s), &ppc4xx_i2c_ops, s,
                           TYPE_PPC4xx_I2C, PPC4xx_I2C_MEM_SIZE);
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);

I allocate memory here but I'm not sure if it should be g_free'd somewhere and if so where? I was not able to detangle QOM object hierarchies and there seems to be no good docs available or I haven't found them. (PCI devices seem to have unrealize methods but this did not work for I2C objects.)



reply via email to

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