On Sat, Oct 03, 2009 at 11:11:41AM +0200, Wolfram Sang wrote: > > Hi Kurt, > > On Fri, Oct 02, 2009 at 05:28:56PM +0200, Kurt Van Dijck wrote: > > This part does the generic sja1000 hook. > > I used 'static inline' functions to keep the open_candev/close_candev > > functions readable. > > > > Signed-off-by: Kurt Van Dijck <[email protected]> > > --- > > Index: drivers/net/can/sja1000/sja1000.h > > =================================================================== > > --- drivers/net/can/sja1000/sja1000.h (revision 1066) > > +++ drivers/net/can/sja1000/sja1000.h (working copy) > > @@ -171,6 +171,14 @@ > > u16 flags; /* custom mode flags */ > > u8 ocr; /* output control register */ > > u8 cdr; /* clock divider register */ > > + > > + /* > > Should have two asterisks if it is meant as nanodoc. I'll take a look at some example. > > > + * enable function pointer: > > + * @dev : the device, parent of the net_device > > + * @active : 1 = enable, 0 = disable > > + * @returns : 0 or -error > > + */ > > + int (*enable)(struct device *dev, int active); > > }; > > > > struct net_device *alloc_sja1000dev(int sizeof_priv); > > Index: drivers/net/can/sja1000/sja1000.c > > =================================================================== > > --- drivers/net/can/sja1000/sja1000.c (revision 1066) > > +++ drivers/net/can/sja1000/sja1000.c (working copy) > > @@ -541,6 +541,24 @@ > > } > > EXPORT_SYMBOL_GPL(sja1000_interrupt); > > > > +static inline int sja1000_ext_enable(struct net_device *dev) > > +{ > > + struct sja1000_priv *priv = netdev_priv(dev); > > + > > + if (!priv->enable) > > + return 0; > > Isn't an errorcode more apropriate? When an 'enable' function has not been supplied, you're in a default state. 'Just do nothing and succeed'. > > > + return priv->enable(dev->dev.parent, 1); > > +} > > + > > +static inline void sja1000_ext_disable(struct net_device *dev) > > +{ > > + struct sja1000_priv *priv = netdev_priv(dev); > > + > > + if (!priv->enable) > > + return; > > + priv->enable(dev->dev.parent, 0); > > Invert the if-logic and save a line here. it now looks more like the ..._enable(). Sure to invert? > > > +} > > + > > static int sja1000_open(struct net_device *dev) > > { > > struct sja1000_priv *priv = netdev_priv(dev); > > @@ -549,10 +567,17 @@ > > /* set chip into reset mode */ > > set_reset_mode(dev); > > > > + /* do external initialization */ > > + err = sja1000_ext_enable(dev); > > + if (err) > > + return err; > > + > > /* common open */ > > err = open_candev(dev); > > - if (err) > > + if (err) { > > + sja1000_ext_disable(dev); > > return err; > > + } > > disable is void. euhm, ??? I don't really understand your point. Can you explain a tiny bit more? > > > > > /* register interrupt handler, if not done by the device driver */ > > if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) { > > @@ -560,6 +585,7 @@ > > dev->name, (void *)dev); > > if (err) { > > close_candev(dev); > > + sja1000_ext_disable(dev); > > return -EAGAIN; > > } > > } > > @@ -589,6 +615,7 @@ > > free_irq(dev->irq, (void *)dev); > > > > close_candev(dev); > > + sja1000_ext_disable(dev); > > > > priv->open_time = 0; > > > > _______________________________________________ > > Socketcan-core mailing list > > [email protected] > > https://lists.berlios.de/mailman/listinfo/socketcan-core > > -- > Pengutronix e.K. | Wolfram Sang | > Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
