> > > +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'.
Ack.
> >
> > > + 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?
Not really, it's a matter of taste, I guess. Probably the compiler will make
the same sequence out of both versions anyway.
> > > + 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?
Yes. My point is heavily based on a misreading where err is set (at open, not
disable); After thinking a bit more about this issue, I'd think the proper
solution is /me taking a brown paper bag and go hiding somewhere in the
mountains :) Sorry for that!
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
signature.asc
Description: Digital signature
_______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
