On 01/12/2011 09:51 AM, Bhupesh SHARMA wrote:
Hi Marc,
Thanks for your review.
Please see my comments inline:
quoted
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org]
On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
...
quoted
quoted
+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.
On 01/12/2011 10:08 AM, Wolfgang Grandegger wrote:
On 01/12/2011 09:51 AM, Bhupesh SHARMA wrote:
quoted
Hi Marc,
Thanks for your review.
Please see my comments inline:
quoted
-----Original Message-----
From: Marc Kleine-Budde [mailto:mkl@pengutronix.de]
On 01/11/2011 12:49 PM, Bhupesh Sharma wrote:
...
quoted
quoted
quoted
+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.
Okay fine with me.
A nice cleanup might be to introduce something like this:
static inline void * give_me_my_priv_from_sja1000_priv
(struct sja1000_priv *priv)
{
return (void *)(priv + 1);
}
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |