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

[Rafał Miłecki]

2011/5/9 Greg KH <greg at kroah.com>:
> On Mon, May 09, 2011 at 04:33:43PM +0200, Rafał Miłecki wrote:
>> 2011/5/8 Arnd Bergmann <arnd at arndb.de>:
>> > On Sunday 08 May 2011 16:59:55 Rafał Miłecki wrote:
>> >> 2011/5/8 Arnd Bergmann <arnd at 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

Ups :) I asked for opinion in:
Date: Tue, 12 Apr 2011 20:52:55 +0200
Message-Id: <1302634375-2378-1-git-send-email-zajec5 at gmail.com>

> Greg: is this what you expected from dynamic allocation and documentation?


But it was hidden in commit comment and there were so many e-mails, it
was probably easy to do not notice it.

-- 
Rafał