On Mon, 15 Feb 2010 09:54:22 +0100 Kurt Van Dijck <[email protected]> wrote:
> On Fri, Feb 12, 2010 at 03:50:53PM -0800, Ira W. Snyder wrote: > > > > The Janz ICAN3 is a MODULbus daughterboard which fits on the Janz CMOD-IO > > PCI carrier board. It is an intelligent CAN controller, with a > > microcontroller and associated firmware. > > > > Signed-off-by: Ira W. Snyder <[email protected]> > > --- > > > [...] > > +/* Maximum number of buffers on a CMOD-IO carrier board */ > > +#define JANZ_MAX_MODULES 4 > > + > > +struct janz_device { > > + struct device *dev; > > + struct pci_dev *pdev; > is dev == &pdev->dev? Yep. Convenience for printing stuff, so we have: dev_dbg(priv->dev, "msg\n"); Instead of: dev_dbg(&priv->pdev->dev, "msg\n"); It really helps on lines that are close to the 80 character limit. If you're worried about speed, the PCI accesses are going to hurt much more than a pointer dereference. > > + > > + void __iomem *modulbus_regs; > > + void __iomem *onboard_regs; > > + > > + /* hex switch position */ > > + u8 hex; > > + > > + /* list of available modules */ > > + struct list_head devices; > dev (see above) already contains a list of devices > You could avoid locking problems by just dropping this list? > see include/linux/device.h for macros walking trough the device list of > a device. I didn't know that. I was copying the drivers/mfd/sm501.c driver. It has a list of devices in it's private data strucure, just like this. Same with the "struct janz_subdevice" below. > > +}; > > + > > +/*----------------------------------------------------------------------------*/ > > +/* Subdevice Support > > */ > > +/*----------------------------------------------------------------------------*/ > > + > > +struct janz_subdevice { > > + struct list_head entry; > > + struct platform_device pdev; > > +}; > and removing this structure in total (when the janz_device.devices is > dropped). Great, I'll do that for the next version. > > + > [...] > > + > > + pdata = pdev->dev.platform_data; > > + pdata->modno = modno; > > + pdev->id = modno; > why twice. I'd rather avoid confusion and assign modno only once. > pdata->modno is part of the "platform data" that gets passed to subdevices. They need to know their MODULbus module number to be able to make use of the interrupt registers (which are shared between all MODULbus modules). > pdev->id should be independant of the board/module number. Right now, a > second carrier board is not possible in the system because of this > assignement to pdev->id. Ok. I thought it was safe to make it the same as the MODULbus module number, but I guess not. I haven't tried the driver with two PCI cards in the system yet. I'll change it to a monotonically increasing integer. > > + dev_dbg(priv->dev, "%s: PDATA %p name %s modno %d\n", __func__, pdata, > > name, pdata->modno); > > + > > + /* MODULbus registers */ > > I think you're getting real close to a proper driver seperation. Good job. > Thanks for the comments. I'll incorporate them in the next version. One quick question: in some systems I have, I expect to have 4 CAN modules present, connected to 4 different sets of devices. Is there a way to choose a specific CAN device (can0, can1, etc.) independent of the PCI enumeration order? In ethernet devices, you can use the MAC address to assign a static name to your ethernet devices, but AFAIK, CAN devices don't have MAC addresses. Note that my boards do have a user-changeable hexadecimal switch on them. You can read it from the PCI BAR on the board. The Janz character driver uses this switch + slot number to identify interfaces. You can move the board to different PCI slots and still have your software work. Thanks, Ira -- Ira Snyder <[email protected]> _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
