On 01/12/2011 09:51 AM, Bhupesh SHARMA wrote: > Hi Marc, > > Thanks for your review. > Please see my comments inline: > >> -----Original Message----- >> From: Marc Kleine-Budde [mailto:[email protected]] >> On 01/11/2011 12:49 PM, Bhupesh Sharma wrote: ... >>> +static int c_can_close(struct net_device *dev) { >>> + struct c_can_priv *priv = netdev_priv(dev); >>> + >>> + netif_stop_queue(dev); >>> + napi_disable(&priv->napi); >>> + c_can_stop(dev); >>> + free_irq(dev->irq, dev); >>> + close_candev(dev); >>> + >>> + return 0; >>> +} >>> + >>> +struct net_device *alloc_c_can_dev(void) >> >> Please model after alloc_sja1000_dev: >> >> struct net_device *alloc_sja1000dev(int sizeof_priv) >> >> The private for the _user_ of alloc_c_can_dev is behind the sja1000 >> private, so you can get rid of the void *priv member in the struct >> c_can_priv. (see below) > > Ok. > But Wolfgang also suggested to use the *priv* member inside c_can_priv struct > for board specific details. In my c_can platform driver I use it to > store the *clk* variable. Do I need to change that as well?
Marc is referring to: http://lxr.linux.no/#linux+v2.6.37/drivers/net/can/sja1000/sja1000.c#L582 But also there "priv->priv" is used to store a pointer to the board specific details. Looking to the SJA1000 drivers, only *one* uses "sizeof_priv > 0", but many other attach a separately allocated structure to "priv->priv". For that reason, I'm fine with your current implementation. Wolfgang. _______________________________________________ Socketcan-core mailing list [email protected] https://lists.berlios.de/mailman/listinfo/socketcan-core
