Re: [PATCH][WAS:bcmai, axi] bcma: add Broadcom specific AMBA bus driver

[Greg KH <greg@kroah.com>]

On Mon, May 09, 2011 at 04:33:43PM +0200, Rafał Miłecki wrote:
> 2011/5/8 Arnd Bergmann <arnd@arndb.de>:
> > On Sunday 08 May 2011 16:59:55 Rafał Miłecki wrote:
> >> 2011/5/8 Arnd Bergmann <arnd@arndb.de>:
> >> >> >> diff --git a/drivers/bcma/TODO b/drivers/bcma/TODO
> >> >> >> new file mode 100644
> >> >> >> index 0000000..45eadc9
> >> >> >> --- /dev/null
> >> >> >> +++ b/drivers/bcma/TODO
> >> >> >> @@ -0,0 +1,3 @@
> >> >> >> +- Interrupts
> >> >> >> +- Defines for PCI core driver
> >> >> >> +- Convert bcma_bus->cores into linked list
> >> >> >
> >> >> > The last item doesn't make sense to me. Since you are using the regular
> >> >> > driver model, you can simply iterate over all child devices of any
> >> >> > dev.
> >> >>
> >> >> It's about optimization. Right now bcma_bus->cores is static array, we
> >> >> probably never will use all entries.
> >> >
> >> > Oh, I see. You should probably have neither of them. Instead allocate
> >> > the devices dynamically as you find them and do a device_register,
> >> > which will add the device into linked list.
> >>
> >> As I said, and wrote: TODO.
> >
> > Well, I think getting this part right is essential before the
> > patch can get merged.
> >
> >> > Maybe you didn't understand what I said: This should be
> >> >
> >> > struct bcma_device {
> >> >     struct bcma_bus *bus;
> >> >     struct bcma_device_id id;
> >> >     struct device dev;
> >> >     u8 core_index;
> >> >
> >> >     u32 addr;
> >> >     u32 wrap;
> >> >
> >> >     void *drvdata;
> >> > };
> >> >
> >> > Here, bcma_device is the device, no need to follow pointers
> >> > around. It's how all bus_types work, you should just do the same.
> >>
> >> We can not use static "struct device", see Greg's comments in:
> >> [RFC][PATCH V3] axi: add AXI bus driver
> >> (not to mention we would have unused "struct device" in ChipCommon's
> >> and PCI's "struct bcma_device").
> >
> > Please reread what Greg explained, it's actually the same as what
> > I said here: Don't make the device static (you already don't),
> > don't put the device structure as a member in the bus structure
> > (as discussed above). Make the device a member of bcma_device,
> > so you get proper reference counting for it, in the way that
> > Greg explained.
> 
> Thanks for help & explaining! Unfortunately Greg didn't answer if my
> changed implementation is fine. I'll fix this!

Greg didn't know that you changed it, or that you wanted review comments
on it.

thanks,

greg "please be specific when asking for review" k-h