Date:	Sun, 8 May 2011 16:59:55 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	linux-wireless@...r.kernel.org,
	"John W. Linville" <linville@...driver.com>,
	b43-dev@...ts.infradead.org, Greg KH <greg@...ah.com>,
	Michael Büsch <mb@...sch.de>,
	Larry Finger <Larry.Finger@...inger.net>,
	George Kashperko <george@...u.edu.ua>,
	Arend van Spriel <arend@...adcom.com>,
	linux-arm-kernel@...ts.infradead.org,
	Russell King <rmk@....linux.org.uk>,
	Andy Botting <andy@...ybotting.com>,
	linuxdriverproject <devel@...uxdriverproject.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH][WAS:bcmai,axi] bcma: add Broadcom specific AMBA bus driver

2011/5/8 Arnd Bergmann <arnd@...db.de>:
> On Friday 06 May 2011 16:50:30 Rafał Miłecki wrote:
>> 2011/5/6 Arnd Bergmann <arnd@...db.de>:
>
>> > This really needs a changelog. You've probably written all of
>> > it before, just explain above the Cc what bcma is, where it's
>> > used, why you use a bus_type. This will be the place where
>> > people look when they want to find out what it is, so try
>> > to make a good description.
>>
>> What do you mean by changelog? Is README unsufficient? It contains
>> almost everything you mentioned...
>
> The changelog is the text at the start of the email, which is
> what 'git log' shows you after the patch gets applied.

By changelog I understood differences between V1, V2, ..., V6.
I read "above the Cc" as "to above Cc". I thought you want me to
explain ppl from Cc what BCMA is.


>> > This would be a good start for the changelog. You don't actually need the
>> > readme in the code directory, it's better to put the information somewhere
>> > in the Documentation/ directory.
>>
>> I guess Documentation/ can be a good idea, but I'd like to make it
>> later if nobody really minds. It's no fun to post more and more
>> versions of patches, just to update some single description.
>
> Having the text in Documentation/ is optional except for user
> interfaces, but generally considered a good idea. The changelog
> in the email text is not optional.
>
>> >> 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.


>> > I don't know if we discussed this before. Normally, you should not need such
>> > udelay() calls, at least if you have the correct I/O barriers in place.
>>
>> I believe we didn't.
>>
>> We had to use such a delays in ssb to let devices react to the
>> changes. Did you maybe have a talk with hardware guys at Broadcom
>> about this? Are you sure this is not needed for BCMA?
>
> Normally what you should have is a register which you can poll
> to find out of the device is ready. In some cases the mmio read
> gets stalled until the hardware is done, in other cases, you have
> to do repeated reads until a register goes from 1 to 0 or the
> opposite. I would be surprised if BCMA didn't have this, but
> it's possible.
>
>> >> +#include "bcma_private.h"
>> >> +#include <linux/bcma/bcma.h>
>> >> +
>> >> +MODULE_DESCRIPTION("Broadcom's specific AMBA driver");
>> >> +MODULE_LICENSE("GPL");
>> >> +
>> >> +static int bcma_bus_match(struct device *dev, struct device_driver *drv);
>> >> +static int bcma_device_probe(struct device *dev);
>> >> +static int bcma_device_remove(struct device *dev);
>> >
>> > Try to reorder your functions so you don't need any forward declarations.
>>
>> That's needed because I put bus-closely-related stuff at the
>> beginning. I did this for readability, I don't think it really hurts
>> anyone or is against coding style or sth.
>
> When I'm reading a source file, I usually start at the end
> because that is where the important stuff gets registered to
> other subsystems. It's really confusing when one source file
> does it in a different order.
>
> Further, it's not obvious that the code is recursion free if
> you have forward declarations in the beginning.
>
>> >> +const char *bcma_device_name(u16 coreid)
>> >> +{
>> >> +     switch (coreid) {
>> >> +     case BCMA_CORE_OOB_ROUTER:
>> >> +             return "OOB Router";
>> >> +     case BCMA_CORE_INVALID:
>> >> +             return "Invalid";
>> >> +     case BCMA_CORE_CHIPCOMMON:
>> >> +             return "ChipCommon";
>> >> +     case BCMA_CORE_ILINE20:
>> >> +             return "ILine 20";
>> >
>> > It's better to make that a data structure than a switch() statement,
>> > both from readability and efficiency aspects.
>>
>> Well, maybe. We call it only once, at init time. In any case we're
>> still waiting for Broadcom to clarify which cores are really used for
>> BCMA.
>
> Readability is really what counts here. With efficiency, I mostly
> referred to code size, not execution time. As a general rule, use
> data structures instead of code where they are equivalent.
>
>> >> +/* 1) It is not allowed to put struct device statically in bcma_device
>> >> + * 2) We can not just use pointer to struct device because we use container_of
>> >> + * 3) We do not have pointer to struct bcma_device in struct device
>> >> + * Solution: use such a dummy wrapper
>> >> + */
>> >> +struct __bcma_dev_wrapper {
>> >> +     struct device dev;
>> >> +     struct bcma_device *core;
>> >> +};
>> >> +
>> >> +struct bcma_device {
>> >> +     struct bcma_bus *bus;
>> >> +     struct bcma_device_id id;
>> >> +
>> >> +     struct device *dev;
>> >> +
>> >> +     u8 core_index;
>> >> +
>> >> +     u32 addr;
>> >> +     u32 wrap;
>> >> +
>> >> +     void *drvdata;
>> >> +};
>> >
>> > Something went wrong here, maybe you misunderstood the API, or I
>> > misunderstood what you are trying to do. When you define your own bus
>> > type, the private device (struct bcma_device) should definitely contain
>> > a struct device as a member, and you allocate that structure dynamically
>> > when probing the bus. I don't see any reason for that wrapper.
>>
>> Having "stuct device" in my "struct bcma_device" let me walk from
>> bcma_device to device. Not the opposite. In case of:
>> manuf_show
>> id_show
>> rev_show
>> class_show
>> I've to go this opposite way. I've "stuct device" but I need to get
>> corresponding "struct bcma_device".
>
> 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").

-- 
Rafał
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists