Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Pat Gefre]

OK I have a new version.

The code is at:
ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

There is still a bit more testing I need to do but it can still be reviewed.

Thanks,
-- Pat


Here are the review comments:

Matthew Wilcox wrote:
 > 
 > Don't define your own names for standard PCI config space -- use the
 > ones in linux/pci.h instead.  This whole section should be deleted:
 > 
 > +/*
 > + * PCI Configuration Space Register Address Map, use offset from IOC4 PCI
 > + * configuration base such that this can be used for multiple IOC4s
 > + */
 > +#define IOC4_PCI_SCR      0x4  /* Status/Command */
 > +#define IOC4_PCI_REV      0x8  /* Revision */
 > +#define IOC4_PCI_LAT      0xC  /* Latency Timer */
 > +#define IOC4_PCI_BAR0     0x10 /* IOC4 base address 0 */
 > +#define IOC4_PCI_SIDV     0x2c /* Subsys ID and vendor */
 > +#define IOC4_PCI_CAP      0x34 /* Capability pointer */
 > +#define IOC4_PCI_LATGNTINT 0x3c        /* Max_lat, min_gnt, int_pin, int_line */
 > 

OK took this out.


 > 
 > Calling a pci_dev a "pci_handle" is confusing; most code uses "pdev".
 > 
 > +       pci_read_config_dword(pci_handle, IOC4_PCI_SCR, &tmp);
 > +       pci_write_config_dword(pci_handle, IOC4_PCI_SCR,
 > +                              tmp | PCI_COMMAND_MASTER |
 > +                              PCI_COMMAND_MEMORY |
 > +                              PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

OK. Done.


 > 
 > You call pci_set_master() before this which takes care of PCI_COMMAND_MASTER.
 > You also call pci_enable_device() which calls pcibios_enable_device()
 > which ensures PCI_COMMAND_MEMORY is set if it needs to be.
 > 
 > So the code above should be:
 > 
 >         pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
 >         pci_write_config_dword(pdev, PCI_COMMAND, cmd | \
 >                         PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

OK. Done.


 > 
 > Personally, I believe we should be setting PCI_COMMAND_PARITY and
 > PCI_COMMAND_SERR on all devices by default in pcibios_enable_device,
 > if not in pci_enable_device().  But we don't, so it's fine to do it
 > in your driver for the moment.
 > 
 > 
 > You don't need the:
 > 
 > +       if (!ia64_platform_is("sn2"))

OK. Gone.

 > 
 > 
 > since this code will only ever be called if someone has an ioc4 in
 > their system.  If it's not an sn2, something's very strange ;-)
 > 
 > 
 > +struct pci_driver ioc4_s_driver = {
 > +      name     :"IOC4 Serial",
 > +      id_table :ioc4_s_id_table,
 > +      probe    :ioc4_attach,
 > +};
 > 
 > please use C99 initialisers instead

Sure. Sorry. Done.


 > 
 > 
 > +    {PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC4, PCI_ANY_ID, PCI_ANY_ID, 0,0,0},
 > you don't need the trailing zeroes

OK. Done.

 > I don't see why you need valid_icount_path().  Everywhere it's called,
 > you seem to have been handed an ioc4_port back by the kernel core.
 > Are you just checking for data corruption elsewhere, or is this masking
 > a bug elsewhere in the driver?

I fixed this up - it was code that hadn't been completely cleaned up.

> Why put it in arch/ia64/sn/io/sn2/driver/ioc4_serial.c ?!
> drivers/serial/ioc4.c would be the right place for it.  You put the
> Kconfig there -- that should be a clue.

OK. Moved.

> 
> It seems like you're directly dereferencing pointers to io memory instead
> of calling readb and friends.  I know, this driver doesn't need to be
> portable, but it helps any casual reader of this driver figure out what's
> going on.  And you can get rid of the 'volatile' that way ;-)

OK. Fixed.

> 
> Linux Device Drivers, Second edition says you shouldn't use SA_INTERRUPT
> without good reason (http://www.xml.com/ldd/chapter/book/ch09.html#t3)

OK. Removed.



Christoph Hellwig wrote:

> I took a very short look and what spring to mind first is that the
> device probing/remoal is rather bogus.  The ->probe/->remove callbacks
> of a PCI driver can be called at any time, and any initialization /
> teardown actions must happen from those.  A logical consequence of that
> is that a proper PCI driver should have no global state.
> 
> I'd also like to second Matthews commens, please move the driver to
> drivers/serial and use proper readX/writeX accessors.  Please run the
> driver through sparse to find the iomem derferences and possibly other
> issues.

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.

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.

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

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

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Russell King]

On Wed, Dec 22, 2004 at 01:44:23PM +0000, Christoph Hellwig wrote:
> > 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.

You want to register with the serial core before you register with PCI.
Then add each port when you find it via the PCI driver ->probe method.

Removal is precisely the reverse order - remove each port in ->remove
method first, then unregister from serial core.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Patrick Gefre]

Russell King wrote:
> On Wed, Dec 22, 2004 at 01:44:23PM +0000, Christoph Hellwig wrote:
> 
>>>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.
> 
> 
> You want to register with the serial core before you register with PCI.
> Then add each port when you find it via the PCI driver ->probe method.
> 
> Removal is precisely the reverse order - remove each port in ->remove
> method first, then unregister from serial core.
> 

How do I know how many ports I have when I register with serial core ? I use the info I got when i 
probed to fill in .nr

-- Pat

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Russell King]

On Wed, Dec 22, 2004 at 09:20:42AM -0600, Patrick Gefre wrote:
> Russell King wrote:
> > You want to register with the serial core before you register with PCI.
> > Then add each port when you find it via the PCI driver ->probe method.
> > 
> > Removal is precisely the reverse order - remove each port in ->remove
> > method first, then unregister from serial core.
> 
> How do I know how many ports I have when I register with serial core ?
> I use the info I got when i probed to fill in .nr

You need to decide on a maximum number of ports and always use that.
You only need to add the ports that you actually have in reality
though.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Patrick Gefre]

Christoph Hellwig wrote:

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

Has this been done before ? Any example I can use ??


> 
> +/* defining this will force the driver to run in polled mode */
> +//#define POLLING_FOR_CHARACTERS
> 
> Again, what's the need for these conditionals?
> 

Most of these are compile options to use/not use particular things/"features". Some were
used during debugging. It's a small thing, I'll delete.


> 
> +/* 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.
> 

WHAT ?!?!?!?  Then I get nice output that actually identifies each card when I have >1. 8^) Not a 
big thing, I'll delete.


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

So then I'd have to declare the end elements as:
void __iomem foo;

They are 32 bit values, so it's OK to assume that void __iomem is 32bits ?

FWIW I did run sparse and it didn't complain about the readX/writeX.....


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

Why is that ? Seems if kmalloc returns a void * and the left side is not, a casting is appropriate ?

-- Pat

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Matthew Wilcox]

On Wed, Dec 22, 2004 at 01:53:28PM -0600, Patrick Gefre wrote:
> Christoph Hellwig wrote:
> >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.
> 
> Has this been done before ? Any example I can use ??

drivers/parisc/superio.c does something similar.  I'm not sure I'd hold
it up as a shining example of how to write a driver ... constructive
criticism welcomed, thought there's some outstanding changes still in
the parisc tree that need to go upstream.

> Why is that ? Seems if kmalloc returns a void * and the left side is not, a 
> casting is appropriate ?

void * is special and different.  This is exactly why it was invented, btw.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

On Wed, Dec 22, 2004 at 01:53:28PM -0600, Patrick Gefre wrote:
> Christoph Hellwig wrote:
> 
> >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.
> >
> 
> Has this been done before ? Any example I can use ??

Well, just about any secondary bus (e.g. usb, iee1394, i2c) works that way,
but I guess all those examples are a little too complicated for your example.
the PPC OCP stuff might be a better example as it's an on-chip pseudo-bus,
otoh it's a top-level bus and not parented by PCI.

> >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.
> >
> 
> So then I'd have to declare the end elements as:
> void __iomem foo;
> 
> They are 32 bit values, so it's OK to assume that void __iomem is 32bits ?

Hmm?  void __iomem must only ever be used as a pointer and passed to
readX/writeX.  Pointer arithmetics are allowed and it's treated equally
to char * for that (GCC extension)

> >no need to cast the return value from kmalloc (dito for the other places)
> >
> 
> Why is that ? Seems if kmalloc returns a void * and the left side is not, a 
> casting is appropriate ?

void * is magic in C and can be assigned to any pointer and vice versa.

Re: [PATCH] Altix : ioc4 serial driver support

[Pat Gefre]

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.


The code is at:
ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Signed-off-by: Patrick Gefre <pfg@sgi.com>

Re: [PATCH] Altix : ioc4 serial driver support

[Christoph Hellwig]

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.

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

Christoph Hellwig wrote:
> On Mon, Jan 31, 2005 at 04:45:05PM -0600, Pat Gefre wrote:
> 
> 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.
> 

I want ioc4_serial_init called before pci_register_driver() if I make it a
module_init() call I have no control over order ??

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

So ide_pci_register_driver is only for legacy devices with certain IDE
problems - I think that is what you are saying (just trying to make sure
I have it right) ??


Thanks for the review,
-- Pat

Re: [PATCH] Altix : ioc4 serial driver support

[Bartlomiej Zolnierkiewicz]

On Wed, 02 Feb 2005 14:36:15 -0600, Patrick Gefre <pfg@sgi.com> wrote:
> Christoph Hellwig wrote:
> > 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.
> >
> 
> So ide_pci_register_driver is only for legacy devices with certain IDE
> problems - I think that is what you are saying (just trying to make sure
> I have it right) ??

ide_pci_register() is needed because of legacy ordering assumptions
(from BIOS and/or Windows) in case of many PCI IDE devices.  If there
is no possibility of other IDE PCI devices on your architecture it is safe to
call pci_register_driver() directly (see ide_scan_pcibus() in setup-pci.c).

BTW IDE part of the patch looks OK.

Thanks,
Bartlomiej

Re: [PATCH] Altix : ioc4 serial driver support

[Christoph Hellwig]

On Wed, Feb 02, 2005 at 02:36:15PM -0600, Patrick Gefre wrote:
> >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.
> >
> 
> I want ioc4_serial_init called before pci_register_driver() if I make it a
> module_init() call I have no control over order ??

For the modular case it'd always be executed before because the module
must be loaded first, for the builtin case it'd depend on the link order.

Let's leave it as-is, it's probably safer.

> >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.
> >
> 
> So ide_pci_register_driver is only for legacy devices with certain IDE
> problems - I think that is what you are saying (just trying to make sure
> I have it right) ??

Yes.

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

Christoph Hellwig wrote:
> On Wed, Feb 02, 2005 at 02:36:15PM -0600, Patrick Gefre wrote:
> 
>>>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.
>>>
>>
>>I want ioc4_serial_init called before pci_register_driver() if I make it a
>>module_init() call I have no control over order ??
> 
> 
> For the modular case it'd always be executed before because the module
> must be loaded first, for the builtin case it'd depend on the link order.
> 
> Let's leave it as-is, it's probably safer.
> 
> 


Latest version with review mods:
ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Signed-off-by: Patrick Gefre <pfg@sgi.com>

Re: [PATCH] Altix : ioc4 serial driver support

[Christoph Hellwig]

On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
> Latest version with review mods:
> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support


 - still not __iomem annotations.
 - still a ->remove method

more comments (mostly nipicks I missed last time, nothing too exciting):


+#define DEVICE_NAME_DYNAMIC "ttyIOC0"	/* need full name for misc_register */

this one is completely unused.

+#define PENDING(_p)	readl(&(_p)->ip_mem->sio_ir) & _p->ip_ienb

probably wants some braces around the macro body

+static struct ioc4_port *get_ioc4_port(struct uart_port *the_port)
+{
+	struct ioc4_control *control = dev_get_drvdata(the_port->dev);
+	int ii;
+
+	if (control) {
+		for ( ii = 0; ii < IOC4_NUM_SERIAL_PORTS; ii++ ) {
+			if (!control->ic_port[ii].icp_port)
+				continue;
+			if (the_port == control->ic_port[ii].icp_port->ip_port)
+				return control->ic_port[ii].icp_port;
+		}
+	}
+	return (struct ioc4_port *)0;

just return NULL here.

+static irqreturn_t ioc4_intr(int irq, void *arg, struct pt_regs *regs)
+{
+	struct ioc4_soft *soft;
+	uint32_t this_ir, this_mir;
+	int xx, num_intrs = 0;
+	int intr_type;
+	int handled = 0;
+	struct ioc4_intr_info *ii;
+
+	soft = (struct ioc4_soft *)arg;
+	if (!soft)
+		return IRQ_NONE;	/* Polled but no ioc4 registered */

no need to cast.  and it can't be NULL either.

+	spin_lock_irqsave(&port->ip_lock, port_flags);
+	wake_up_interruptible(&info->delta_msr_wait);
+	spin_unlock_irqrestore(&port->ip_lock, port_flags);

no need to lock around a wake_up()

+	/* Start up the serial port */
+	spin_lock_irqsave(&port->ip_lock, port_flags);
+	retval = ic4_startup_local(the_port);
+	if (retval) {
+		spin_unlock_irqrestore(&port->ip_lock, port_flags);
+		return retval;
+	}
+	spin_unlock_irqrestore(&port->ip_lock, port_flags);
+	return 0;

what about just

	spin_lock_irqsave(&port->ip_lock, port_flags);
	retval = ic4_startup_local(the_port);
	spin_unlock_irqrestore(&port->ip_lock, port_flags);
	return reval;

?
	
+	struct ioc4_port *port = get_ioc4_port(the_port);
+	unsigned long port_flags;
+
+	spin_lock_irqsave(&port->ip_lock, port_flags);
+	ioc4_change_speed(the_port, termios, old_termios);
+	spin_unlock_irqrestore(&port->ip_lock, port_flags);
+	return;

no need for empty returns at the end of void functions

+static struct uart_driver ioc4_uart = {
+	.owner		= THIS_MODULE,
+	.driver_name	= "ioc4_serial",
+	.dev_name	= DEVICE_NAME,
+	.major		= DEVICE_MAJOR,
+	.minor		= DEVICE_MINOR,
+	.nr		= IOC4_NUM_CARDS * IOC4_NUM_SERIAL_PORTS,
+	.cons		= NULL,
+};

no need to initialize .cons to zero, the compiler does that for you.

+	if ( !request_region(tmp_addr, sizeof(struct ioc4_mem), "sioc4_mem")) {

superflous space before the !

+	if (!request_irq(pdev->irq, ioc4_intr, SA_SHIRQ,
+				"sgi-ioc4serial", (void *)soft)) {
+		control->ic_irq = pdev->irq;
+	} else {
+		printk(KERN_WARNING
+		    "%s : request_irq fails for IRQ 0x%x\n ",
+			__FUNCTION__, pdev->irq);
+	}

Can the driver work without an irq?

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

I've update the patch with changes from the comments below.

ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support




Christoph Hellwig wrote:
> On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
> 
>>Latest version with review mods:
>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> 
> 
> 
>  - still not __iomem annotations.

I *think* I have this right ??



>  - still a ->remove method
> 
> more comments (mostly nipicks I missed last time, nothing too exciting):
> 
> 
> +#define DEVICE_NAME_DYNAMIC "ttyIOC0"	/* need full name for misc_register */
> 
> this one is completely unused.
> 
> +#define PENDING(_p)	readl(&(_p)->ip_mem->sio_ir) & _p->ip_ienb
> 
> probably wants some braces around the macro body
> 
> +static struct ioc4_port *get_ioc4_port(struct uart_port *the_port)
> +{
> +	struct ioc4_control *control = dev_get_drvdata(the_port->dev);
> +	int ii;
> +
> +	if (control) {
> +		for ( ii = 0; ii < IOC4_NUM_SERIAL_PORTS; ii++ ) {
> +			if (!control->ic_port[ii].icp_port)
> +				continue;
> +			if (the_port == control->ic_port[ii].icp_port->ip_port)
> +				return control->ic_port[ii].icp_port;
> +		}
> +	}
> +	return (struct ioc4_port *)0;
> 
> just return NULL here.
> 
> +static irqreturn_t ioc4_intr(int irq, void *arg, struct pt_regs *regs)
> +{
> +	struct ioc4_soft *soft;
> +	uint32_t this_ir, this_mir;
> +	int xx, num_intrs = 0;
> +	int intr_type;
> +	int handled = 0;
> +	struct ioc4_intr_info *ii;
> +
> +	soft = (struct ioc4_soft *)arg;
> +	if (!soft)
> +		return IRQ_NONE;	/* Polled but no ioc4 registered */
> 
> no need to cast.  and it can't be NULL either.
> 
> +	spin_lock_irqsave(&port->ip_lock, port_flags);
> +	wake_up_interruptible(&info->delta_msr_wait);
> +	spin_unlock_irqrestore(&port->ip_lock, port_flags);
> 
> no need to lock around a wake_up()
> 
> +	/* Start up the serial port */
> +	spin_lock_irqsave(&port->ip_lock, port_flags);
> +	retval = ic4_startup_local(the_port);
> +	if (retval) {
> +		spin_unlock_irqrestore(&port->ip_lock, port_flags);
> +		return retval;
> +	}
> +	spin_unlock_irqrestore(&port->ip_lock, port_flags);
> +	return 0;
> 
> what about just
> 
> 	spin_lock_irqsave(&port->ip_lock, port_flags);
> 	retval = ic4_startup_local(the_port);
> 	spin_unlock_irqrestore(&port->ip_lock, port_flags);
> 	return reval;
> 
> ?
> 	
> +	struct ioc4_port *port = get_ioc4_port(the_port);
> +	unsigned long port_flags;
> +
> +	spin_lock_irqsave(&port->ip_lock, port_flags);
> +	ioc4_change_speed(the_port, termios, old_termios);
> +	spin_unlock_irqrestore(&port->ip_lock, port_flags);
> +	return;
> 
> no need for empty returns at the end of void functions
> 
> +static struct uart_driver ioc4_uart = {
> +	.owner		= THIS_MODULE,
> +	.driver_name	= "ioc4_serial",
> +	.dev_name	= DEVICE_NAME,
> +	.major		= DEVICE_MAJOR,
> +	.minor		= DEVICE_MINOR,
> +	.nr		= IOC4_NUM_CARDS * IOC4_NUM_SERIAL_PORTS,
> +	.cons		= NULL,
> +};
> 
> no need to initialize .cons to zero, the compiler does that for you.
> 
> +	if ( !request_region(tmp_addr, sizeof(struct ioc4_mem), "sioc4_mem")) {
> 
> superflous space before the !
> 
> +	if (!request_irq(pdev->irq, ioc4_intr, SA_SHIRQ,
> +				"sgi-ioc4serial", (void *)soft)) {
> +		control->ic_irq = pdev->irq;
> +	} else {
> +		printk(KERN_WARNING
> +		    "%s : request_irq fails for IRQ 0x%x\n ",
> +			__FUNCTION__, pdev->irq);
> +	}
> 
> Can the driver work without an irq?

Not in its current state.

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

I've update the patch with changes from the comments below.

ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

As usual forgot this:

Signed-off-by: Patrick Gefre <pfg@sgi.com>




Patrick Gefre wrote:
> I've update the patch with changes from the comments below.
> 
> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> 
> 
> 
> 
> Christoph Hellwig wrote:
> 
>> On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
>>
>>> Latest version with review mods:
>>> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>
>>
>>
>>
>>  - still not __iomem annotations.
> 
> 
> I *think* I have this right ??
> 
> 
> 
>>  - still a ->remove method
>>
>> more comments (mostly nipicks I missed last time, nothing too exciting):
>>
>>
>> +#define DEVICE_NAME_DYNAMIC "ttyIOC0"    /* need full name for 
>> misc_register */
>>
>> this one is completely unused.
>>
>> +#define PENDING(_p)    readl(&(_p)->ip_mem->sio_ir) & _p->ip_ienb
>>
>> probably wants some braces around the macro body
>>
>> +static struct ioc4_port *get_ioc4_port(struct uart_port *the_port)
>> +{
>> +    struct ioc4_control *control = dev_get_drvdata(the_port->dev);
>> +    int ii;
>> +
>> +    if (control) {
>> +        for ( ii = 0; ii < IOC4_NUM_SERIAL_PORTS; ii++ ) {
>> +            if (!control->ic_port[ii].icp_port)
>> +                continue;
>> +            if (the_port == control->ic_port[ii].icp_port->ip_port)
>> +                return control->ic_port[ii].icp_port;
>> +        }
>> +    }
>> +    return (struct ioc4_port *)0;
>>
>> just return NULL here.
>>
>> +static irqreturn_t ioc4_intr(int irq, void *arg, struct pt_regs *regs)
>> +{
>> +    struct ioc4_soft *soft;
>> +    uint32_t this_ir, this_mir;
>> +    int xx, num_intrs = 0;
>> +    int intr_type;
>> +    int handled = 0;
>> +    struct ioc4_intr_info *ii;
>> +
>> +    soft = (struct ioc4_soft *)arg;
>> +    if (!soft)
>> +        return IRQ_NONE;    /* Polled but no ioc4 registered */
>>
>> no need to cast.  and it can't be NULL either.
>>
>> +    spin_lock_irqsave(&port->ip_lock, port_flags);
>> +    wake_up_interruptible(&info->delta_msr_wait);
>> +    spin_unlock_irqrestore(&port->ip_lock, port_flags);
>>
>> no need to lock around a wake_up()
>>
>> +    /* Start up the serial port */
>> +    spin_lock_irqsave(&port->ip_lock, port_flags);
>> +    retval = ic4_startup_local(the_port);
>> +    if (retval) {
>> +        spin_unlock_irqrestore(&port->ip_lock, port_flags);
>> +        return retval;
>> +    }
>> +    spin_unlock_irqrestore(&port->ip_lock, port_flags);
>> +    return 0;
>>
>> what about just
>>
>>     spin_lock_irqsave(&port->ip_lock, port_flags);
>>     retval = ic4_startup_local(the_port);
>>     spin_unlock_irqrestore(&port->ip_lock, port_flags);
>>     return reval;
>>
>> ?
>>     
>> +    struct ioc4_port *port = get_ioc4_port(the_port);
>> +    unsigned long port_flags;
>> +
>> +    spin_lock_irqsave(&port->ip_lock, port_flags);
>> +    ioc4_change_speed(the_port, termios, old_termios);
>> +    spin_unlock_irqrestore(&port->ip_lock, port_flags);
>> +    return;
>>
>> no need for empty returns at the end of void functions
>>
>> +static struct uart_driver ioc4_uart = {
>> +    .owner        = THIS_MODULE,
>> +    .driver_name    = "ioc4_serial",
>> +    .dev_name    = DEVICE_NAME,
>> +    .major        = DEVICE_MAJOR,
>> +    .minor        = DEVICE_MINOR,
>> +    .nr        = IOC4_NUM_CARDS * IOC4_NUM_SERIAL_PORTS,
>> +    .cons        = NULL,
>> +};
>>
>> no need to initialize .cons to zero, the compiler does that for you.
>>
>> +    if ( !request_region(tmp_addr, sizeof(struct ioc4_mem), 
>> "sioc4_mem")) {
>>
>> superflous space before the !
>>
>> +    if (!request_irq(pdev->irq, ioc4_intr, SA_SHIRQ,
>> +                "sgi-ioc4serial", (void *)soft)) {
>> +        control->ic_irq = pdev->irq;
>> +    } else {
>> +        printk(KERN_WARNING
>> +            "%s : request_irq fails for IRQ 0x%x\n ",
>> +            __FUNCTION__, pdev->irq);
>> +    }
>>
>> Can the driver work without an irq?
> 
> 
> Not in its current state.
> 
> -
> 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/

Re: [PATCH] Altix : ioc4 serial driver support

[Jesse Barnes]

On Tuesday, February 8, 2005 8:52 am, Patrick Gefre wrote:
> I've update the patch with changes from the comments below.
>
> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>
> Christoph Hellwig wrote:
> > On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
> >>Latest version with review mods:
> >>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> >
> >  - still not __iomem annotations.
>
> I *think* I have this right ??

Here's the output from 'make C=1' with your driver patched in (you if you want
to run it yourself, just copy tomahawk.engr:~jbarnes/bin/sparse to somewhere
in your path and run 'make C=1').  I think most of these warning would be
fixed up if the structure fields referring to registers were declared as
__iomem, but I haven't looked carefully.

Jesse

  CHECK   drivers/serial/ioc4_serial.c
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:796:11: warning: incorrect type in argument 1 (different address spaces)
drivers/serial/ioc4_serial.c:796:11:    expected void const volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:796:11:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:796:11: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:796:11:    expected void const volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:796:11:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:800:11: warning: incorrect type in argument 1 (different address spaces)
drivers/serial/ioc4_serial.c:800:11:    expected void const volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:800:11:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:800:11: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:800:11:    expected void const volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:800:11:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2722:6: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:2722:6:    expected struct ioc4_mem *mem
drivers/serial/ioc4_serial.c:2722:6:    got void [noderef] *<asn:2>
drivers/serial/ioc4_serial.c:2742:9: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:2742:9:    expected struct ioc4_serial *serial
drivers/serial/ioc4_serial.c:2742:9:    got void [noderef] *<asn:2>
drivers/serial/ioc4_serial.c:2786:2: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:2786:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2786:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2786:2: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:2786:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2786:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2789:2: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:2789:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2789:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2789:2: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:2789:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2789:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2795:2: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:2795:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2795:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2795:2: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:2795:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2795:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:693:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:693:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:693:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:697:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:697:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:697:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2798:2: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:2798:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2798:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:2798:2: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:2798:2:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:2798:2:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:1106:16: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:1106:16:    expected struct ioc4_mem [noderef] *ip_mem<asn:2>
drivers/serial/ioc4_serial.c:1106:16:    got struct ioc4_mem *<noident>
drivers/serial/ioc4_serial.c:1107:19: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:1107:19:    expected struct ioc4_serial [noderef] *ip_serial<asn:2>
drivers/serial/ioc4_serial.c:1107:19:    got struct ioc4_serial *<noident>
drivers/serial/ioc4_serial.c:875:11: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:875:11:    expected unsigned int [usertype] *sbbr_l
drivers/serial/ioc4_serial.c:875:11:    got unsigned int [unsigned] [noderef] [usertype] *<noident><asn:2>
drivers/serial/ioc4_serial.c:876:11: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:876:11:    expected unsigned int [usertype] *sbbr_h
drivers/serial/ioc4_serial.c:876:11:    got unsigned int [unsigned] [noderef] [usertype] *<noident><asn:2>
drivers/serial/ioc4_serial.c:878:11: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:878:11:    expected unsigned int [usertype] *sbbr_l
drivers/serial/ioc4_serial.c:878:11:    got unsigned int [unsigned] [noderef] [usertype] *<noident><asn:2>
drivers/serial/ioc4_serial.c:879:11: warning: incorrect type in assignment (different address spaces)
drivers/serial/ioc4_serial.c:879:11:    expected unsigned int [usertype] *sbbr_h
drivers/serial/ioc4_serial.c:879:11:    got unsigned int [unsigned] [noderef] [usertype] *<noident><asn:2>
drivers/serial/ioc4_serial.c:886:3: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:886:3:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:886:3:    got unsigned int [usertype] *sbbr_h
drivers/serial/ioc4_serial.c:886:3: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:886:3:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:886:3:    got unsigned int [usertype] *sbbr_h
drivers/serial/ioc4_serial.c:887:3: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:887:3:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:887:3:    got unsigned int [usertype] *sbbr_l
drivers/serial/ioc4_serial.c:887:3: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:887:3:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:887:3:    got unsigned int [usertype] *sbbr_l
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:681:4: warning: incorrect type in initializer (different address spaces)
drivers/serial/ioc4_serial.c:681:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:681:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: incorrect type in argument 2 (different address spaces)
drivers/serial/ioc4_serial.c:685:4:    expected void volatile [noderef] *addr<asn:2>
drivers/serial/ioc4_serial.c:685:4:    got unsigned int [unsigned] [usertype] *<noident>
drivers/serial/ioc4_serial.c:685:4: warning: too many warnings

Re: [PATCH] Altix : ioc4 serial driver support

[Christoph Hellwig]

On Thu, Feb 10, 2005 at 11:09:43AM -0800, Jesse Barnes wrote:
> On Tuesday, February 8, 2005 8:52 am, Patrick Gefre wrote:
> > I've update the patch with changes from the comments below.
> >
> > ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> >
> > Christoph Hellwig wrote:
> > > On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
> > >>Latest version with review mods:
> > >>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> > >
> > >  - still not __iomem annotations.
> >
> > I *think* I have this right ??
> 
> Here's the output from 'make C=1' with your driver patched in (you if you want
> to run it yourself, just copy tomahawk.engr:~jbarnes/bin/sparse to somewhere
> in your path and run 'make C=1').  I think most of these warning would be
> fixed up if the structure fields referring to registers were declared as
> __iomem, but I haven't looked carefully.

Actually the pointers to the struct need to be declared __iomem.  

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

I updated again with more __iomem tags.

ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Signed-off-by: Patrick Gefre <pfg@sgi.com>



Christoph Hellwig wrote:
> On Thu, Feb 10, 2005 at 11:09:43AM -0800, Jesse Barnes wrote:
> 
>>On Tuesday, February 8, 2005 8:52 am, Patrick Gefre wrote:
>>
>>>I've update the patch with changes from the comments below.
>>>
>>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>>
>>>Christoph Hellwig wrote:
>>>
>>>>On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
>>>>
>>>>>Latest version with review mods:
>>>>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>>>
>>>> - still not __iomem annotations.
>>>
>>>I *think* I have this right ??
>>
>>Here's the output from 'make C=1' with your driver patched in (you if you want
>>to run it yourself, just copy tomahawk.engr:~jbarnes/bin/sparse to somewhere
>>in your path and run 'make C=1').  I think most of these warning would be
>>fixed up if the structure fields referring to registers were declared as
>>__iomem, but I haven't looked carefully.
> 
> 
> Actually the pointers to the struct need to be declared __iomem.  

Re: [PATCH] Altix : ioc4 serial driver support

[Patrick Gefre]

Andrew,

Since there don't seem to be any more suggestions, can you take this - or at least queue it up ???

This is a resend:

I updated again with more __iomem tags.

ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Signed-off-by: Patrick Gefre <pfg@sgi.com>




> 
> Christoph Hellwig wrote:
> 
>> On Thu, Feb 10, 2005 at 11:09:43AM -0800, Jesse Barnes wrote:
>>
>>> On Tuesday, February 8, 2005 8:52 am, Patrick Gefre wrote:
>>>
>>>> I've update the patch with changes from the comments below.
>>>>
>>>> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>>>
>>>> Christoph Hellwig wrote:
>>>>
>>>>> On Mon, Feb 07, 2005 at 09:58:33AM -0600, Patrick Gefre wrote:
>>>>>
>>>>>> Latest version with review mods:
>>>>>> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>>>>
>>>>>
>>>>> - still not __iomem annotations.
>>>>
>>>>
>>>> I *think* I have this right ??
>>>
>>>
>>> Here's the output from 'make C=1' with your driver patched in (you if 
>>> you want
>>> to run it yourself, just copy tomahawk.engr:~jbarnes/bin/sparse to 
>>> somewhere
>>> in your path and run 'make C=1').  I think most of these warning 
>>> would be
>>> fixed up if the structure fields referring to registers were declared as
>>> __iomem, but I haven't looked carefully.
>>
>>
>>
>> Actually the pointers to the struct need to be declared __iomem.  
> 
> 

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

> I'm not sure what you mean here. I don't have an entry for ->remove and the 
> driver is self-contained.

In a PCI driver (well, just about any driver for a modern bus) you have
an probe and an remove entry.  All code to setup and teardown a device must
happen in those functions, and must not use global state but only the device
instance pointers. 

btw, no need to Cc linux-ia64, there's nothing ia64-specific in this driver.

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Patrick Gefre]

Christoph Hellwig wrote:
> On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> 
>>I have a serial driver for Altix I'd like to submit.
>>
>>The code is at:
>>ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
>>
>>Signed-off-by: Patrick Gefre <pfg@sgi.com>
> 
> 
> I took a very short look and what spring to mind first is that the
> device probing/remoal is rather bogus.  The ->probe/->remove callbacks
> of a PCI driver can be called at any time, and any initialization /
> teardown actions must happen from those.  A logical consequence of that
> is that a proper PCI driver should have no global state.
> 

Christoph,

I'm not sure what you mean here. I don't have an entry for ->remove and the driver is self-contained.

-- Pat

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Matthew Wilcox]

On Thu, Dec 16, 2004 at 11:15:19PM +0000, Christoph Hellwig wrote:
> I took a very short look and what spring to mind first is that the

Rats, I'd hoped you'd have time to do a more thorough review.  Here's
some more comments:

Don't define your own names for standard PCI config space -- use the
ones in linux/pci.h instead.  This whole section should be deleted:

+/*
+ * PCI Configuration Space Register Address Map, use offset from IOC4 PCI
+ * configuration base such that this can be used for multiple IOC4s
+ */
+#define IOC4_PCI_SCR	   0x4	/* Status/Command */
+#define IOC4_PCI_REV	   0x8	/* Revision */
+#define IOC4_PCI_LAT	   0xC	/* Latency Timer */
+#define IOC4_PCI_BAR0	   0x10	/* IOC4 base address 0 */
+#define IOC4_PCI_SIDV	   0x2c	/* Subsys ID and vendor */
+#define IOC4_PCI_CAP	   0x34	/* Capability pointer */
+#define IOC4_PCI_LATGNTINT 0x3c	/* Max_lat, min_gnt, int_pin, int_line */


Calling a pci_dev a "pci_handle" is confusing; most code uses "pdev".

+	pci_read_config_dword(pci_handle, IOC4_PCI_SCR, &tmp);
+	pci_write_config_dword(pci_handle, IOC4_PCI_SCR,
+			       tmp | PCI_COMMAND_MASTER |
+			       PCI_COMMAND_MEMORY |
+			       PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

You call pci_set_master() before this which takes care of PCI_COMMAND_MASTER.
You also call pci_enable_device() which calls pcibios_enable_device()
which ensures PCI_COMMAND_MEMORY is set if it needs to be.

So the code above should be:

	pci_read_config_dword(pdev, PCI_COMMAND, &cmd);
	pci_write_config_dword(pdev, PCI_COMMAND, cmd | \
			PCI_COMMAND_PARITY | PCI_COMMAND_SERR);

Personally, I believe we should be setting PCI_COMMAND_PARITY and
PCI_COMMAND_SERR on all devices by default in pcibios_enable_device,
if not in pci_enable_device().  But we don't, so it's fine to do it
in your driver for the moment.


You don't need the:

+	if (!ia64_platform_is("sn2"))
+		return -ENODEV;

since this code will only ever be called if someone has an ioc4 in
their system.  If it's not an sn2, something's very strange ;-)


+struct pci_driver ioc4_s_driver = {
+      name	:"IOC4 Serial",
+      id_table	:ioc4_s_id_table,
+      probe	:ioc4_attach,
+};

please use C99 initialisers instead


+    {PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC4, PCI_ANY_ID, PCI_ANY_ID, 0,0,0},
you don't need the trailing zeroes


I don't see why you need valid_icount_path().  Everywhere it's called,
you seem to have been handed an ioc4_port back by the kernel core.
Are you just checking for data corruption elsewhere, or is this masking
a bug elsewhere in the driver?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Christoph Hellwig]

On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> I have a serial driver for Altix I'd like to submit.
> 
> The code is at:
> ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support
> 
> Signed-off-by: Patrick Gefre <pfg@sgi.com>

I took a very short look and what spring to mind first is that the
device probing/remoal is rather bogus.  The ->probe/->remove callbacks
of a PCI driver can be called at any time, and any initialization /
teardown actions must happen from those.  A logical consequence of that
is that a proper PCI driver should have no global state.

I'd also like to second Matthews commens, please move the driver to
drivers/serial and use proper readX/writeX accessors.  Please run the
driver through sparse to find the iomem derferences and possibly other
issues.

Re: [PATCH] 2.6.10 Altix : ioc4 serial driver support

[Matthew Wilcox]

On Thu, Dec 16, 2004 at 04:24:26PM -0600, Pat Gefre wrote:
> I have a serial driver for Altix I'd like to submit.

Why put it in arch/ia64/sn/io/sn2/driver/ioc4_serial.c ?!
drivers/serial/ioc4.c would be the right place for it.  You put the
Kconfig there -- that should be a clue.

It seems like you're directly dereferencing pointers to io memory instead
of calling readb and friends.  I know, this driver doesn't need to be
portable, but it helps any casual reader of this driver figure out what's
going on.  And you can get rid of the 'volatile' that way ;-)

Linux Device Drivers, Second edition says you shouldn't use SA_INTERRUPT
without good reason (http://www.xml.com/ldd/chapter/book/ch09.html#t3)

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[PATCH] 2.6.10 Altix : ioc4 serial driver support

[Pat Gefre]

I have a serial driver for Altix I'd like to submit.

The code is at:
ftp://oss.sgi.com/projects/sn2/sn2-update/033-ioc4-support

Signed-off-by: Patrick Gefre <pfg@sgi.com>


-- Pat

Patrick Gefre
Silicon Graphics, Inc.                     (E-Mail)  pfg@sgi.com
2750 Blue Water Rd                         (Voice)   (651) 683-3127
Eagan, MN 55121-1400                       (FAX)     (651) 683-3054