On 4 July 2011 23:39, andrzej zaborowski <balr...@gmail.com> wrote:
> Patch looks good overall, but for consistency we should rename
> functions which start with omap2_gpio_module_ to omap2_gpio_ if the
> state pointer passed is no longer the module pointer but instead the
> whole thing pointer. But maybe it would make more sense for each
> module to be a device? It looks like vmsaving/vmloading the union
> structure may be problematic and we can also save some cycles.
Yes, I think you're right and we should just have two separate
devices rather than one with a union.
> Similarly omap_l4_base would be better named omap_l4_region_base or
> similar.
Happy to change this, I'm not very attached to the name.
> I'd also prefer that omap2_gpio_init remains a function that
> does all the qdev magic in omap_gpio.c and with the current signature
I'm not convinced about this. Implementing the GPIO module as
a sysbus device seems like the right thing, and it's the job
of omap2.c to know how to map and wire the resources the sysbus
device provides. Hiding that in a helper function in omap_gpio.c
doesn't seem quite right.
> (so for example clocks usage can be kept track of).
You're right, we probably should do something with the clocks (even
though omap_gpio doesn't use them). We'd probably wind up with a
'wire up clocks' function per device type though, unless we want to
try to make all omap devices inherit from a common thing that knows
about clocks (so we could have an omap_connect_clk() function like
sysbus_connect_irq()). [This is about the only thing inclining
me in favour of an _init routine. I guess you touch on the idea
of an omap-specific bus below.]
PS: on the subject of clocks, this is how a later qdevification patch
in my stack does the wiring up of the uart:
s->uart[0] = qdev_create(NULL, "omap_uart");
s->uart[0]->id = "uart1";
qdev_prop_set_uint32(s->uart[0], "mmio_size", 0x1000);
qdev_prop_set_uint32(s->uart[0], "baudrate",
omap_clk_getrate(omap_findclk(s, "uart1_fclk")) / 16);
qdev_prop_set_chr(s->uart[0], "chardev", serial_hds[0]);
qdev_init_nofail(s->uart[0]);
busdev = sysbus_from_qdev(s->uart[0]);
sysbus_connect_irq(busdev, 0, s->irq[0][OMAP_INT_24XX_UART1_IRQ]);
sysbus_connect_irq(busdev, 1, s->drq[OMAP24XX_DMA_UART1_TX]);
sysbus_connect_irq(busdev, 2, s->drq[OMAP24XX_DMA_UART1_RX]);
sysbus_mmio_map(busdev, 0, omap_l4_base(omap_l4ta(s->l4, 19), 0));
...it would be useful to know if you're going to object to how it's
dealing with the clock there so I can fix it in advance and avoid
a round of review.
> Would it make more sense to pass the ta structures instead of base
> adresses to the qdev? Have you considered how difficult l4 would be
> to convert to a QBus?
Hmm. I hadn't thought about that. Does it buy us anything useful?
I'm not sure how it would interact with wanting to be able to
put plain-old-sysbus devices onto the bus (eg the OHCI USB).
Thanks for the review.
-- PMM
