lkml.org 
[lkml]   [2005]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Altix : ioc4 serial driver support
On Mon, Jan 31, 2005 at 04:45:05PM -0600, Pat Gefre wrote:
>
> I've updated this patch with suggestions from the reviews. And moved it
> up the latest 2.6 (since it has been awhile...). I'm also adding
> Bartlomiej as a CC since there are IDE mods involved.

This looks much better, thanks.

Please kill ioc4_ide_init as it's completely unused and make ioc4_serial_init
a normal module_init() handler in ioc4_serial, there's no need to call
them from the generic driver.

Also yo should need to implement ioc4_remove_one (yet) as the ide driver
can't be removed yet. It might make sense to keep it and
ioc4_serial_remove_one around #if 0'ed as that might be implemented soon.

Do you need to use ide_pci_register_driver? IOC4 doesn't have the legacy
IDE problems, and it's never used together with such devices in a system,
so a plain pci_register_driver should do it.

In ioc4_ide_attach_one you can kill the if and return pci_init_sgiioc4(..)
directly.

In ioc4.c please include <asm/sn/ioc4_common.h> after <linux/ide.h>.

Also ioc4_common.h should probably move to include/linux as ioc4 is more
or less just a pci device and not that SN-specific.

The ioc4_serial driver looks more or less good to me, but you seem to
miss __iomem annotation and there's a few things that it'd have cought,
like casting the return value from ioremap (it's a void __iomem * so it
can be assigned to any pointer directly (or any __iomem pointer in sparse).

ioc4_soft.is_intr_type[].is_intr_ents_free should be an unsigned long
so test_and_clear_bit can operate on it directly. But I fail to see where
we set bits in at all (?)

ioc4_serial_attach_one has various resource leaks when parts of the
initialization fail. Try to follow the goto-based cleanup model most pci
drivers use instead of returning directly on failures.

-
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:10    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site