Messages in this thread | | | Date | Mon, 1 Jun 2009 22:08:54 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 2/9] add support for the TI VLYNQ bus |
| |
On Mon, 1 Jun 2009 13:58:27 +0200 Florian Fainelli <florian@openwrt.org> wrote:
> This patch adds support for the TI VLYNQ high-speed, > serial and packetized bus. This bus allows external > devices to be connected to the System-on-Chip and > appear in the main system memory just like any memory > mapped peripheral. It is widely used in TI's networking > and mutlimedia SoC, including the AR7 SoC. > > > ... > > +struct vlynq_regs { > + u32 revision; > + u32 control; > + u32 status; > + u32 int_prio; > + u32 int_status; > + u32 int_pending; > + u32 int_ptr; > + u32 tx_offset; > + struct vlynq_mapping rx_mapping[4]; > + u32 chip; > + u32 autonego; > + u32 unused[6]; > + u32 int_device[8]; > +}; > + > +#define vlynq_reg_read(reg) readl(&(reg)) > +#define vlynq_reg_write(reg, val) writel(val, &(reg))
grumble. These just make the code harder to follow. it'd be better to open-code readl() and writel() at the callsites.
> +static int __vlynq_enable_device(struct vlynq_device *dev); > + > +#ifdef VLYNQ_DEBUG > +static void vlynq_dump_regs(struct vlynq_device *dev) > +{ > + int i; > + > + printk(KERN_DEBUG "VLYNQ local=%p remote=%p\n", > + dev->local, dev->remote); > + for (i = 0; i < 32; i++) { > + printk(KERN_DEBUG "VLYNQ: local %d: %08x\n", > + i + 1, ((u32 *)dev->local)[i]); > + printk(KERN_DEBUG "VLYNQ: remote %d: %08x\n", > + i + 1, ((u32 *)dev->remote)[i]); > + } > +} > + > +static void vlynq_dump_mem(u32 *base, int count) > +{ > + int i; > + > + for (i = 0; i < (count + 3) / 4; i++) { > + if (i % 4 == 0) > + printk(KERN_DEBUG "\nMEM[0x%04x]:", i * 4); > + printk(KERN_DEBUG " 0x%08x", *(base + i)); > + } > + printk(KERN_DEBUG "\n"); > +}
lib/hexdump.c?
> +#endif > + > +int vlynq_linked(struct vlynq_device *dev)
afacit this didn't need to be a kernel-wide symbol?
Please review the patchset for any unnecessarily-global symbols.
> +{ > + int i; > + > + for (i = 0; i < 100; i++) > + if (vlynq_reg_read(dev->local->status) & VLYNQ_STATUS_LINK) > + return 1; > + else > + cpu_relax(); > + > + return 0; > +} > + > > ... > > +static void vlynq_remote_ack(unsigned int irq) > +{ > + struct vlynq_device *dev = get_irq_chip_data(irq); > + > + u32 status = vlynq_reg_read(dev->remote->status); > + > + if (printk_ratelimit()) > + printk(KERN_DEBUG "%s: remote status: 0x%08x\n", > + dev_name(&dev->dev), status); > + vlynq_reg_write(dev->remote->status, status); > +}
This code seems to do rather a lot of printks for production code?
> > ... > > +static int vlynq_device_match(struct device *dev, > + struct device_driver *drv) > +{ > + struct vlynq_device *vdev = to_vlynq_device(dev); > + struct vlynq_driver *vdrv = to_vlynq_driver(drv); > + struct vlynq_device_id *ids = vdrv->id_table; > + > + while (ids->id) { > + if (ids->id == vdev->dev_id) { > + vdev->divisor = ids->divisor; > + vlynq_set_drvdata(vdev, ids); > + printk(KERN_INFO "Driver found for VLYNQ " \ > + "device: %08x\n", vdev->dev_id); > + return 1; > + } > + printk(KERN_DEBUG "Not using the %08x VLYNQ device's driver" \ > + " for VLYNQ device: %08x\n", ids->id, vdev->dev_id);
The backslashes here are unneeded and unconventional.
> + ids++; > + } > + return 0; > +} > + > +static int vlynq_device_probe(struct device *dev) > +{ > + struct vlynq_device *vdev = to_vlynq_device(dev); > + struct vlynq_driver *drv = to_vlynq_driver(dev->driver); > + struct vlynq_device_id *id = vlynq_get_drvdata(vdev); > + int result = -ENODEV; > + > + get_device(dev); > + if (drv && drv->probe) > + result = drv->probe(vdev, id);
Can `drv' really be NULL here?
> + if (result) > + put_device(dev); > + return result; > +} > + > +static int vlynq_device_remove(struct device *dev) > +{ > + struct vlynq_driver *drv = to_vlynq_driver(dev->driver); > + if (drv && drv->remove) > + drv->remove(to_vlynq_device(dev));
ditto.
> + put_device(dev); > + return 0; > +} > + > +int __vlynq_register_driver(struct vlynq_driver *driver, struct module *owner) > +{ > + driver->driver.name = driver->name; > + driver->driver.bus = &vlynq_bus_type; > + return driver_register(&driver->driver); > +} > +EXPORT_SYMBOL(__vlynq_register_driver); > + > +void vlynq_unregister_driver(struct vlynq_driver *driver) > +{ > + driver_unregister(&driver->driver); > +} > +EXPORT_SYMBOL(vlynq_unregister_driver); > + > +static int __vlynq_try_remote(struct vlynq_device *dev) > +{ > + int i; > + > + vlynq_reset(dev); > + for (i = dev->dev_id ? vlynq_rdiv2 : vlynq_rdiv8; dev->dev_id ? > + i <= vlynq_rdiv8 : i >= vlynq_rdiv2; > + dev->dev_id ? i++ : i--) {
Wow.
> + if (!vlynq_linked(dev)) > + break; > + > + vlynq_reg_write(dev->remote->control, > + (vlynq_reg_read(dev->remote->control) & > + ~VLYNQ_CTRL_CLOCK_MASK) | > + VLYNQ_CTRL_CLOCK_INT | > + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1)); > + vlynq_reg_write(dev->local->control, > + ((vlynq_reg_read(dev->local->control) > + & ~(VLYNQ_CTRL_CLOCK_INT | > + VLYNQ_CTRL_CLOCK_MASK)) | > + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_rdiv1))); > + > + if (vlynq_linked(dev)) { > + printk(KERN_DEBUG > + "%s: using remote clock divisor %d\n", > + dev_name(&dev->dev), i - vlynq_rdiv1 + 1); > + dev->divisor = i; > + return 0; > + } else { > + vlynq_reset(dev); > + } > + } > + > + return -ENODEV; > +}
This code could do with a few comments.
See, this reader of your code doesn't know what vlynq_linked() does, so I don't have a hope of working out whey this function returns -ENODEV if it couldn't find any "linked" devices. What does this _mean_ in terms of the underlying hardware and its configuration?
Dunno. But it would be nice to be able to work that out from reading the code, no?
> +static int __vlynq_try_local(struct vlynq_device *dev) > +{ > + int i; > + > + vlynq_reset(dev); > + > + for (i = dev->dev_id ? vlynq_ldiv2 : vlynq_ldiv8; dev->dev_id ? > + i <= vlynq_ldiv8 : i >= vlynq_ldiv2; > + dev->dev_id ? i++ : i--) { > + > + vlynq_reg_write(dev->local->control, > + (vlynq_reg_read(dev->local->control) & > + ~VLYNQ_CTRL_CLOCK_MASK) | > + VLYNQ_CTRL_CLOCK_INT | > + VLYNQ_CTRL_CLOCK_DIV(i - vlynq_ldiv1)); > + > + if (vlynq_linked(dev)) { > + printk(KERN_DEBUG > + "%s: using local clock divisor %d\n", > + dev_name(&dev->dev), i - vlynq_ldiv1 + 1); > + dev->divisor = i; > + return 0; > + } else { > + vlynq_reset(dev); > + } > + } > + > + return -ENODEV; > +} > + > +static int __vlynq_try_external(struct vlynq_device *dev) > +{ > + vlynq_reset(dev); > + if (!vlynq_linked(dev)) > + return -ENODEV; > + > + vlynq_reg_write(dev->remote->control, > + (vlynq_reg_read(dev->remote->control) & > + ~VLYNQ_CTRL_CLOCK_INT)); > + > + vlynq_reg_write(dev->local->control, > + (vlynq_reg_read(dev->local->control) & > + ~VLYNQ_CTRL_CLOCK_INT)); > + > + if (vlynq_linked(dev)) { > + printk(KERN_DEBUG "%s: using external clock\n", > + dev_name(&dev->dev)); > + dev->divisor = vlynq_div_external; > + return 0; > + } > + > + return -ENODEV; > +}
"remote", "local", "external". What do these terms mean? Perhaps there's a TI datasheet somewhere?
> +static int __vlynq_enable_device(struct vlynq_device *dev) > +{ > + int result; > + struct plat_vlynq_ops *ops = dev->dev.platform_data; > + > + result = ops->on(dev); > + if (result) > + return result; > + > + switch (dev->divisor) { > + case vlynq_div_external: > + case vlynq_div_auto: > + /* When the device is brought from reset it should have clock > + generation negotiated by hardware. > + Check which device is generating clocks and perform setup > + accordingly */
Preferred comment layout format is
/* * When the device is brought from reset it should have clock * generation negotiated by hardware. Check which device is * generating clocks and perform setup accordingly */
> + if (vlynq_linked(dev) && vlynq_reg_read(dev->remote->control) & > + VLYNQ_CTRL_CLOCK_INT) { > + if (!__vlynq_try_remote(dev) || > + !__vlynq_try_local(dev) || > + !__vlynq_try_external(dev)) > + return 0; > + } else { > + if (!__vlynq_try_external(dev) || > + !__vlynq_try_local(dev) || > + !__vlynq_try_remote(dev)) > + return 0; > + } > + break; > + case vlynq_ldiv1: case vlynq_ldiv2: case vlynq_ldiv3: case vlynq_ldiv4: > + case vlynq_ldiv5: case vlynq_ldiv6: case vlynq_ldiv7: case vlynq_ldiv8:
One `case' per line, please.
> + vlynq_reg_write(dev->local->control, > + VLYNQ_CTRL_CLOCK_INT | > + VLYNQ_CTRL_CLOCK_DIV(dev->divisor - > + vlynq_ldiv1)); > + vlynq_reg_write(dev->remote->control, 0); > + if (vlynq_linked(dev)) { > + printk(KERN_DEBUG > + "%s: using local clock divisor %d\n", > + dev_name(&dev->dev), > + dev->divisor - vlynq_ldiv1 + 1); > + return 0; > + } > + break; > + case vlynq_rdiv1: case vlynq_rdiv2: case vlynq_rdiv3: case vlynq_rdiv4: > + case vlynq_rdiv5: case vlynq_rdiv6: case vlynq_rdiv7: case vlynq_rdiv8: > + vlynq_reg_write(dev->local->control, 0); > + vlynq_reg_write(dev->remote->control, > + VLYNQ_CTRL_CLOCK_INT | > + VLYNQ_CTRL_CLOCK_DIV(dev->divisor - > + vlynq_rdiv1)); > + if (vlynq_linked(dev)) { > + printk(KERN_DEBUG > + "%s: using remote clock divisor %d\n", > + dev_name(&dev->dev), > + dev->divisor - vlynq_rdiv1 + 1); > + return 0; > + } > + break; > + } > + > + ops->off(dev); > + return -ENODEV; > +} > + > +int vlynq_enable_device(struct vlynq_device *dev) > +{ > + struct plat_vlynq_ops *ops = dev->dev.platform_data; > + int result = -ENODEV; > + > + result = __vlynq_enable_device(dev); > + if (result) > + return result; > + > + result = vlynq_setup_irq(dev); > + if (result) > + ops->off(dev);
It's strange that this function directly calls __vlynq_enable_device(), and undoes that via ops->off() if vlynq_setup_irq() failed.
I'd have expected to see a call to ops->off() used as a cancallation for ops->on()?
> + dev->enabled = !result; > + return result; > +} > +EXPORT_SYMBOL(vlynq_enable_device); > + > + > > ... > > +struct vlynq_device { > + u32 id, dev_id; > + int local_irq; > + int remote_irq; > + enum vlynq_divisor divisor; > + u32 regs_start, regs_end; > + u32 mem_start, mem_end;
This doesn't look 64-bit-bus friendly.
> + u32 irq_start, irq_end; > + int irq; > + int enabled; > + struct vlynq_regs *local; > + struct vlynq_regs *remote; > + struct device dev; > +}; > > ... >
| |