On Fri, Jan 29, 2016 at 2:28 PM, Andrew Baumann <andrew.baum...@microsoft.com> wrote: >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] >> Sent: Friday, 29 January 2016 14:23 >> >> On Fri, Jan 29, 2016 at 1:50 PM, Andrew Baumann >> <andrew.baum...@microsoft.com> wrote: >> > Hi Peter, >> > >> > Thanks for all the reviews. I should have a respun version on the list >> > shortly. >> There's one minor change to this last patch: >> > >> >> From: Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] >> >> Sent: Thursday, 28 January 2016 23:31 >> >> > On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann >> <andrew.baum...@microsoft.com> wrote: >> > [...] >> >> > +typedef struct RaspiState { >> >> >> >> A quick google search, I see the camel case form for rpi is usually >> >> "RasPi". Should we follow? >> > >> > Ok. >> > >> >> > + union { >> >> >> >> union not needed. >> > >> > I know it's not needed now, but it will be as soon as we add pi1, which I >> hope to address in the next patch series. It will make that diff cleaner if >> we >> keep this here now, so I'm going to leave it as-is. I hope that's ok with >> you. >> > >> >> It sounds like you are implementing an inheritance outside QOM. I'm >> not sure about this, can we just drop the union and figure it out on >> the next series? > > It's nothing nearly that clever/complicated. See > https://github.com/0xabu/qemu/blob/raspi/hw/arm/raspi.c#L116 > > switch (version) { > case 1: > object_initialize(&s->soc.pi1, sizeof(s->soc.pi1), TYPE_BCM2835); > break; > case 2: > object_initialize(&s->soc.pi2, sizeof(s->soc.pi2), TYPE_BCM2836); > break; > } > > And then later, we always refer simply to OBJECT(&s->soc). I suppose I could > use a generic Object pointer instead, and allocate the soc-specific type > elsewhere, but it seemed simpler with the union. >
But this is effectively an upcast to an abstract type, here: + + /* Setup the SOC */ + object_property_add_const_link(OBJECT(&s->soc), "ram", OBJECT(&s->ram), + &error_abort); + object_property_set_int(OBJECT(&s->soc), smp_cpus, "enabled-cpus", + &error_abort); Where that concrete type musts implement "ram" and "enabled-cpus". So I am unsure of having a split implementation of "ram", "enabled-cpus" etc from one SoC to the other as you are using a QOM property name as an abstract interface definition. Regards, Peter > Andrew
