Message-Id: <201105081647.22091.arnd@arndb.de>
Date:	Sun, 8 May 2011 16:47:21 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Rafał Miłecki <zajec5@...il.com>
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

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.
 
> > 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.

> > 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.

	Arnd
--
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