On Wed, Jun 13, 2018 at 10:56:59AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Fri, Jun 08, 2018 at 11:20:50AM +0200, BALATON Zoltan wrote:
> > > 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 <balaton@eik.bme.hu>
> > > > 
> > > > 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.
> > 
> > Oh.. right.  There's a better way.
> > 
> > You can just forward declare the bitbang_i2c_interface structure like
> > this in your header:
> > 	typdef struct bitbang_i2c_interface bitbang_i2c_interface;
> > 
> > So you're declaring the existence of the structure, but not its
> > contents - that's sufficient to create a pointer to it.  Then you
> > don't need to creat the substructure and extra level of indirection.
> > 
> > > 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.)
> > 
> > Yes, if you're allocating you definitely should be free()ing.  It
> > should go in the corresponding cleanup routine to where it is
> > allocated.  Since the allocation is in instance_init(), the free()
> > should be in instance_finalize() (which you'd need to add).
> > 
> > Except that the above should let you avoid that.
> > 
> > ..and I guess this won't actually ever be finalized in practice.
> > 
> > ..and there doesn't seem to be a way to free up a bitbang_interface,
> > so even if you added the finalize, it still wouldn't really clean up
> > properly.
> 
> Yes, I suspected it won't matter anyway. I'll try your suggestion to just
> declare the bitbang_i2c_interface in the public header in next version.
> 
> Any more reviews to expect from you for other patches or should I send a v3
> with the changes so far?

Go ahead with v3.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson