On Fri, 27 Jul 2007 18:57:20 +0200
Michael Buesch <mb@xxxxxxxxx> wrote:
> The Sonics Silicon Backplane is a mini-bus used on
> various Broadcom chips and embedded devices.
> Devices using the SSB include b44, bcm43xx and various
> Broadcom based wireless routers.
> A b44 and bcm43xx port and a SSB based OHCI driver is available.
>
hm, slim pickings for me here. Nice looking code.
checkpatch finds a few coding style glitches. Lots of 80-col violations
which you might choose ignore (comments on #defines in headers are
especially hard) but these guys:
ERROR: "foo * bar" should be "foo *bar"
#4156: FILE: drivers/ssb/ssb_private.h:119:
+extern struct ssb_bus * ssb_pci_dev_to_bus(struct pci_dev *pdev);
are worth addressing.
> ...
>
> --- /dev/null
> +++ b/drivers/ssb/driver_pcicore.c
> @@ -0,0 +1,564 @@
>
> ...
>
> +u32 pci_iobase = 0x100;
> +u32 pci_membase = SSB_PCI_DMA;
These are quite poor names for global symbols.
> +static struct resource ssb_pcicore_mem_resource = {
> + .name = "SSB PCIcore external memory",
> + .start = SSB_PCI_DMA,
> + .end = (u32)SSB_PCI_DMA + (u32)SSB_PCI_DMA_SZ - 1,
> + .flags = IORESOURCE_MEM,
> +};
start and end here are resource_size_t. Forcing them to u32 seems
inappropriate.
> +void ssb_pcicore_init(struct ssb_pcicore *pc)
Please check that all newly-added global symbols do indeed need global scope.
> +static void ssb_pcie_mdio_write(struct ssb_pcicore *pc, u8 device,
> + u8 address, u16 data)
> +{
> + const u16 mdio_control = 0x128;
> + const u16 mdio_data = 0x12C;
> + u32 v;
> + int i;
> +
> + v = 0x80; /* Enable Preamble Sequence */
> + v |= 0x2; /* MDIO Clock Divisor */
> + pcicore_write32(pc, mdio_control, v);
> +
> + v = (1 << 30); /* Start of Transaction */
> + v |= (1 << 28); /* Write Transaction */
> + v |= (1 << 17); /* Turnaround */
> + v |= (u32)device << 22;
> + v |= (u32)address << 18;
> + v |= data;
> + pcicore_write32(pc, mdio_data, v);
> + udelay(10);
mystery udelays are usually worth a comment.
> + for (i = 0; i < 10; i++) {
> + v = pcicore_read32(pc, mdio_control);
> + if (v & 0x100 /* Trans complete */)
> + break;
> + msleep(1);
> + }
> + pcicore_write32(pc, mdio_control, 0);
> +}
>
> ...
>
> +
> +static void ssb_buses_lock(void)
> +{
> + if (!ssb_is_early_boot)
> + mutex_lock(&buses_mutex);
> +}
> +
> +static void ssb_buses_unlock(void)
> +{
> + if (!ssb_is_early_boot)
> + mutex_unlock(&buses_mutex);
> +}
Some comments are neeeded here to explain why we're jumping through this
hoop. It's normally OK to take a mutex early in boot, so one wonders
what's happening.
> +EXPORT_SYMBOL(ssb_clockspeed);
Please check that all newly-added exports really need to be exported (I'm
sure they do, but..)
> +static int ssb_wait_bit(struct ssb_device *dev, u16 reg, u32 bitmask,
> + int timeout, int set)
> +{
> + int i;
> + u32 val;
> +
> + for (i = 0; i < timeout; i++) {
> + val = ssb_read32(dev, reg);
> + if (set) {
> + if (val & bitmask)
> + return 0;
> + } else {
> + if (!(val & bitmask))
> + return 0;
> + }
> + udelay(10);
> + }
> + printk(KERN_ERR PFX "Timeout waiting for bitmask %08X on "
> + "register %04X to %s.\n",
> + bitmask, reg, (set ? "set" : "clear"));
> +
> + return -ETIMEDOUT;
> +}
So `timeout' is in units of ten-microseconds. Unusual.
> +void ssb_device_disable(struct ssb_device *dev, u32 core_specific_flags)
> +{
> + u32 reject;
> +
> + if (ssb_read32(dev, SSB_TMSLOW) & SSB_TMSLOW_RESET)
> + return;
> +
> + reject = ssb_tmslow_reject_bitmask(dev);
> + ssb_write32(dev, SSB_TMSLOW, reject | SSB_TMSLOW_CLOCK);
> + ssb_wait_bit(dev, SSB_TMSLOW, reject, 1000, 1);
> + ssb_wait_bit(dev, SSB_TMSHIGH, SSB_TMSHIGH_BUSY, 1000, 0);
> + ssb_write32(dev, SSB_TMSLOW,
> + SSB_TMSLOW_FGC | SSB_TMSLOW_CLOCK |
> + reject | SSB_TMSLOW_RESET |
> + core_specific_flags);
> + /* flush */
> + ssb_read32(dev, SSB_TMSLOW);
> + udelay(1);
> +
> + ssb_write32(dev, SSB_TMSLOW,
> + reject | SSB_TMSLOW_RESET |
> + core_specific_flags);
> + /* flush */
> + ssb_read32(dev, SSB_TMSLOW);
> + udelay(1);
mystery udelay.
> +}
> +EXPORT_SYMBOL(ssb_device_disable);
>
> ...
>
> +const struct ssb_bus_ops ssb_pci_ops = {
> + .read16 = ssb_pci_read16,
> + .read32 = ssb_pci_read32,
> + .write16 = ssb_pci_write16,
> + .write32 = ssb_pci_write32,
> +};
static?
> +static ssize_t ssb_pci_attr_sprom_show(struct device *pcidev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct pci_dev *pdev = container_of(pcidev, struct pci_dev, dev);
> + struct ssb_bus *bus;
> + u16 *sprom;
> + int err = -ENODEV;
> + ssize_t count = 0;
> +
> + bus = ssb_pci_dev_to_bus(pdev);
> + if (!bus)
> + goto out;
> + err = -ENOMEM;
> + sprom = kcalloc(SSB_SPROMSIZE_WORDS, sizeof(u16), GFP_KERNEL);
> + if (!sprom)
> + goto out;
> +
> + err = -ERESTARTSYS;
> + if (mutex_lock_interruptible(&bus->pci_sprom_mutex))
> + goto out_kfree;
> + sprom_do_read(bus, sprom);
> + mutex_unlock(&bus->pci_sprom_mutex);
> +
> + count = sprom2hex(sprom, buf, PAGE_SIZE);
> + err = 0;
> +
> +out_kfree:
> + kfree(sprom);
> +out:
> + return err ? err : count;
> +}
The mutex_lock_interruptible() looks fishy. Some commented explanation of
what it's doing would be good here. It's quite unobvious to this reader.
Cheesy deadlock avoidance? Hope not.
> +
> +/* These are the main device register access functions.
> + * do_select_core is inline to have the likely hotpath inline.
> + * All unlikely codepaths are out-of-line. */
> +static inline int do_select_core(struct ssb_bus *bus,
> + struct ssb_device *dev,
> + u16 *offset)
> +{
> + int err;
> + u8 need_seg = (*offset >= 0x800) ? 1 : 0;
> +
> + if (unlikely(dev != bus->mapped_device)) {
> + err = ssb_pcmcia_switch_core(bus, dev);
> + if (unlikely(err))
> + return err;
> + }
> + if (unlikely(need_seg != bus->mapped_pcmcia_seg)) {
> + err = ssb_pcmcia_switch_segment(bus, need_seg);
> + if (unlikely(err))
> + return err;
> + }
> + if (need_seg == 1)
> + *offset -= 0x800;
> +
> + return 0;
> +}
Looks too large for inlining.
> +const struct ssb_bus_ops ssb_pcmcia_ops = {
> + .read16 = ssb_pcmcia_read16,
> + .read32 = ssb_pcmcia_read32,
> + .write16 = ssb_pcmcia_write16,
> + .write32 = ssb_pcmcia_write32,
> +};
static?
-
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
