On 10/09/2010 09:39 PM, Andre B. Oliveira wrote: >>> 2010/10/6 Wolfram Sang: >> 2010/10/6 Wolfgang Grandegger: > > Thank you very much for your reviews! > Here is the new patch with your suggestions, down below.
Please send a standalone patch to the netdev mailing list with a cc to the socketcan-core maling list. > >> + outb(val, (int)priv->reg_base + reg); >> Is the cast really needed? > > Yes. :-( > priv->reg_base is of type void __iomem * and outb/inb expect an int. > Without the cast, gcc generates a warning: passing argument 2 of > ‘outb’ makes integer from pointer without a cast OK, but outb/inb requires "unsigned long". Therefore s/int/unsigned long/ here and in other places. > >>> + /* Probe and register SJA1000 */ >>> + priv->reg_base = (void __iomem *)sja1000; >>> + if (!register_sja1000dev(netdev)) >>> + break; >>> Hmm, we lose the return value of that function here... Do we really need >>> to continue if the region was succesfully requested and the sja1000dev >>> not? > > Yes, the region may be free in Linux but may be disabled in hardware > (for example, the BIOS of the PC104 CPU board I tested this driver > with only enables a short few I/O port address ranges). The idea is to > keep trying all 8 possible addresses. > >> Hm, does the simple probing of sja1000_probe_chip() work reliably? > > Yes! :-) > >> You can find a more intelligent test in the plx_pci driver. > > I agree, but to avoid duplicating code, maybe we should create one > good probe function in sja1000.c that all modules could use, or maybe > simply replace sja1000_probe_chip() by plx_pci_check_sja1000() ? Yes, that would be best, indeed. Please feel free to send a patch. I added it to my TODO list. For the time being please add you own probing function. >> Anyway, why do you need to probe the IO port? >> Can't it be derived from the JP1:JP2 jumper settings? > > I don't think so. There are 4x8=32 possible IO address configurations, > and only 2, maybe 3 or 4 max, jumpers available. And I didn't want to > use module parameters, I'd like this driver to be completely > automatic. OK. > Thanks! > Here is the patch: > >>From be8727e70f13198589cc1676f48560aa614ea1d0 Mon Sep 17 00:00:00 2001 > From: Andre B. Oliveira <[email protected]> > Date: Sat, 9 Oct 2010 19:28:02 +0100 > Subject: [PATCH net-next-2.6] can: tscan1: add driver for TS-CAN1 boards Please add a short commit message and re-send the patch to the *netdev* mailing list with a cc to the socketcan-core mailing list. Thanks, Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
