lkml.org 
[lkml]   [2004]   [Dec]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] 2.6.10 Altix : ioc4 serial driver support
    > I cleaned this up a bit.  There still are a couple of issues. One is
    > that the sgiioc4 driver also "uses" the ioc4 card for ide. So the probe
    > needs to "fail" so that both driver's probes will be called (is there a
    > better way?). Because of that the ->remove function isn't called
    > directly.

    So both claim the same PCI ID? In this case you need to creat a small
    shim driver that exports a pseudo-bus to the serial and ide driver using
    the driver model. You must never return an error from ->probe if you
    actually use that particular device.

    > I still save off the pci_dev ptrs for all cards found, so I can
    > register with the serial core after probe (is there a better way?).
    > Should I register the driver separately for each card ? That seems a
    > bit overkill.

    You should register them with the serial core in ->probe.

    +#include <asm/sn/ioc4.h>

    Please put that header into drivers/serial/ioc4.h or if you need it from
    other drivers in the future to include/linux/ioc4.h

    +/* Various states/options */
    +#define ENABLE_FLOW_CONTROL
    +#define ENABLE_OUTPUT_INTERRUPTS

    So why do you need these ifdefs? Unless there's a very good reason
    kill all these conditionals.

    +/* To use dynamic numbers only and not use the assigned major and minor,
    + * define the following.. */
    +#define USE_DYNAMIC_MINOR 0 /* Don't rely on misc_register dynamic minor */
    +static struct miscdevice misc; /* used with misc_register for dynamic */

    Dito. Please kill the miscdevice code as you seem to have an assigned
    number.

    +/* defining this will force the driver to run in polled mode */
    +//#define POLLING_FOR_CHARACTERS

    Again, what's the need for these conditionals?

    +/* Infinite loop detection.
    + */
    +#define MAXITER 10000000
    +#define SPIN(cond, success) \
    +{ \
    + int spiniter = 0; \
    + success = 1; \
    + while(cond) { \
    + spiniter++; \
    + if (spiniter > MAXITER) { \
    + success = 0; \
    + break; \
    + } \
    + } \
    +}

    Opencoding this in the callers would make the code a lot more readable.

    +static struct pci_dev *Pdevs[IOC4_NUM_CARDS];

    As mentioned above this shouldn't be needed when the driver uses
    proper attachment.

    +/* a table to keep the card names as passed to request_region */
    +static struct {
    + char c_name[20];
    +} Table_o_cards[IOC4_NUM_CARDS];

    Completely superflous. Just pass "ioc4_serial" as argument to request_region.

    +/* Prototypes */
    +static void ioc4_cb_output_lowat(struct ioc4_port *);
    +static void ioc4_cb_post_ncs(struct uart_port *, int);
    +static void receive_chars(struct uart_port *);
    +static void handle_intr(void *arg, uint32_t sio_ir);

    Please try to avoid forward-declarations where possible.

    +
    +/*
    + * support routines for local atomic operations.
    + */
    +
    +static spinlock_t local_lock;
    +
    +static inline unsigned int atomicClearInt(atomic_t * a, unsigned int b)
    +{
    + unsigned long s;
    + unsigned int ret, new;
    +
    + spin_lock_irqsave(&local_lock, s);
    + new = ret = atomic_read(a);
    + new &= ~b;
    + atomic_set(a, new);
    + spin_unlock_irqrestore(&local_lock, s);
    +
    + return ret;
    +}

    There's only a singler caller, so better open-code it with a per-device
    lock and avoid the bogus atomic_t use. It looks like using bitops.h
    functions would help that code aswell.

    +static inline void
    +write_ireg(struct ioc4_soft *ioc4_soft, uint32_t val, int which, int type)
    +{
    + struct ioc4_mem *mem = ioc4_soft->is_ioc4_mem_addr;
    + spinlock_t *lp = &ioc4_soft->is_ir_lock;

    small style nitpick: In general we try to avoid using spinlock_t pointers
    just as local variables as that makes it more clear what's actually locked.

    + unsigned long s;

    normally we call that variable flags. Doing so aswell here makes the
    code easier to read.

    + spin_lock_irqsave(lp, s);
    +
    + switch (type) {
    + case IOC4_SIO_INTR_TYPE:
    + switch (which) {
    + case IOC4_W_IES:
    + writel(val, (void *)&mem->sio_ies_ro);

    The second argumnet to writeX (and readX) is actually void __iomem *,
    but to see the difference you need to run sparse (from sparse.bkbits.net)
    over the driver. Please store all I/O addresses in void __iomem * pointers
    in your structures and avoid the cast here and in all the other places.

    Remember that in 90% of the cases a cast hides a possible bug.

    + return (1);

    Please avoid braces around the return values.

    + if (ioc4_revid < ioc4_revid_min) {
    + printk(KERN_WARNING
    + "IOC4 serial not supported on firmware rev %d on card %d, "
    + "please upgrade to rev %d or higher\n",
    + ioc4_revid, card_number, ioc4_revid_min);
    + return -1;

    Please return meaninfull values from errno.h

    + port = (struct ioc4_port *)kmalloc(sizeof(struct ioc4_port),
    + GFP_KERNEL);

    no need to cast the return value from kmalloc (dito for the other places)

    + if (pci_enable_device(pdev)) {

    pci_enable_device returns an detailed error, please pass it on to the
    caller.

    + /* Map in the ioc4 memory */
    + mem = (struct ioc4_mem *)pci_resource_start(pdev, 0);

    You're missing an ioremap somewhere.

    + pdev->dev.driver_data = (void *)control;

    please use pci_set_drvdata, the cast isn't needed here aswell.

    + control = (struct ioc4_control *)pdev->dev.driver_data;

    please use pca_get_drvdata, again no need for a cast.

    + /* do the pci probing */
    + pci_register_driver(&ioc4_s_driver);

    You need to check the return value from pci_register_driver.

    + if (uart_register_driver(&ioc4_uart) < 0) {

    Please call uart_register_driver before calling pci_register_driver
    so you can register the ports from ->probe.

    + spin_lock_irqsave(&IOC4_lock, flags);
    +#ifdef POLLING_FOR_CHARACTERS
    + del_timer_sync(&IOC4_timer_list);
    +#endif

    del_timer_sync can sleep and must not be called inside a spinlock.

    + pci_unregister_driver(&ioc4_s_driver);

    dito for pci_unregister_driver.

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

    \
     
     \ /
      Last update: 2005-03-22 14:08    [W:0.028 / U:124.792 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site