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

2011/4/13 George Kashperko <george@...u.edu.ua>:
>
> В Срд, 13/04/2011 в 21:39 +0200, Rafał Miłecki пишет:
>> 2011/4/13 Greg KH <greg@...ah.com>:
>> >> diff --git a/drivers/axi/axi_pci_bridge.c b/drivers/axi/axi_pci_bridge.c
>> >> new file mode 100644
>> >> index 0000000..17e882c
>> >> --- /dev/null
>> >> +++ b/drivers/axi/axi_pci_bridge.c
>> >> @@ -0,0 +1,33 @@
>> >> +/*
>> >> + * AXI PCI bridge module
>> >> + *
>> >> + * Licensed under the GNU/GPL. See COPYING for details.
>> >> + */
>> >> +
>> >> +#include "axi_private.h"
>> >> +
>> >> +#include <linux/axi/axi.h>
>> >> +#include <linux/pci.h>
>> >> +
>> >> +static DEFINE_PCI_DEVICE_TABLE(axi_pci_bridge_tbl) = {
>> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4331) },
>> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4353) },
>> >> +     { PCI_DEVICE(PCI_VENDOR_ID_BROADCOM, 0x4727) },
>> >> +     { 0, },
>> >> +};
>> >> +MODULE_DEVICE_TABLE(pci, axi_pci_bridge_tbl);
>> >> +
>> >> +static struct pci_driver axi_pci_bridge_driver = {
>> >> +     .name = "axi-pci-bridge",
>> >> +     .id_table = axi_pci_bridge_tbl,
>> >> +};
>> >> +
>> >> +int __init axi_pci_bridge_init(void)
>> >> +{
>> >> +     return axi_host_pci_register(&axi_pci_bridge_driver);
>> >> +}
>> >> +
>> >> +void __exit axi_pci_bridge_exit(void)
>> >> +{
>> >> +     axi_host_pci_unregister(&axi_pci_bridge_driver);
>> >> +}
>> >
>> > You register a pci driver that does nothing?  That's not right, you need
>> > to then base your axi bus off of that pci device, so it is hooked up
>> > correctly in the /sys/devices/ tree.  Otherwise you are somewhere up in
>> > the virtual location for your axi bus, right?
>>
>> Please take a look at:
>> driver->probe = axi_host_pci_probe;
>> driver->remove = axi_host_pci_remove;
>> return pci_register_driver(driver);
>>
>>
>> >> +bool axi_core_is_enabled(struct axi_device *core)
>> >> +{
>> >> +     if ((axi_aread32(core, AXI_IOCTL) & (AXI_IOCTL_CLK | AXI_IOCTL_FGC))
>> >> +         != AXI_IOCTL_CLK)
>> >> +             return false;
>> >> +     if (axi_aread32(core, AXI_RESET_CTL) & AXI_RESET_CTL_RESET)
>> >> +             return false;
>> >> +     return true;
>> >> +}
>> >> +EXPORT_SYMBOL(axi_core_is_enabled);
>> >
>> > EXPORT_SYMBOL_GPL()?
>> >
>> > What module uses this?  And why would it care?
>> >
>> >> +EXPORT_SYMBOL(axi_core_enable);
>> >
>> > EXPORT_SYMBOL_GPL()?
>> >
>> > Same goes for your other exports, just want you to be sure here.
>>
>> Hm, I'm not sure. Using EXPORT_SYMBOL_GPL will forbid closed source
>> drivers from using our bus driver, right? I'm don't have preferences
>> on this, if you prefer us to force GPL, I can.
>>
>>
>> >> +u32 xaxi_chipco_gpio_control(struct axi_drv_cc *cc, u32 mask, u32 value)
>> >> +{
>> >> +     return axi_cc_write32_masked(cc, AXI_CC_GPIOCTL, mask, value);
>> >> +}
>> >> +EXPORT_SYMBOL(xaxi_chipco_gpio_control);
>> >
>> > "xaxi"?  Shouldn't that be consistant with the other exports and start
>> > with "axi"?
>>
>> Left from old tests/rewrites/splitting. Thanks.
>>
>>
>> >> +static u8 axi_host_pci_read8(struct axi_device *core, u16 offset)
>> >> +{
>> >> +     if (unlikely(core->bus->mapped_core != core))
>> >
>> > Are you sure about the use of unlikely in this, and other functions?
>> > The compiler almost always does a better job than we do for these types
>> > of calls, just let it do it's job.
>> >
>> >> +             axi_host_pci_switch_core(core);
>> >> +     return ioread8(core->bus->mmio + offset);
>> >
>> > I think because of that unlikely, you just slowed down all pci devices,
>> > right?  That's not very nice :)
>>
>> Hm, my logic suggests it is alright, but please consider this once
>> more with me ;)
>>
>> For the most of the time mapped_core (active core) do not change. We
>> perform few hundreds of operations on one core in a row. This way
>> mapped_core points to passed core for most of the time. Condition
>> (mapped_core != core) is unlikely to happen.
>>
>> Is there anything wrong in my logic?
>>
> Yes, there is. You don't need that "if" at all.

Damn, WHY do you make me ask why, why, why, all the time?! Can't you
just write word of explanation without being asked for?

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