On 8/31/2010 8:58 AM, Cyril Chemparathy wrote:
Hi Ben,
[...]
+COBJS-$(CONFIG_DRIVER_TI_CPSW) += cpsw.o
Please don't use the word DRIVER here. If possible, use something more verbose than "CPSW" too.
Will TI_CPSW_SWITCH work better considering that "CPSW" is the name of the hardware block?
Sure.
[...]
+++ b/drivers/net/cpsw.c
Please rename this ti_cpsw.c
Agreed.
[...]
+struct cpsw_priv {
struct eth_device *dev;struct cpsw_platform_data data;int host_port;struct cpsw_regs *regs;void *dma_regs;struct cpsw_host_regs *host_port_regs;void *ale_regs;struct cpdma_desc descs[NUM_DESCS];struct cpdma_desc *desc_free;struct cpdma_chan rx_chan, tx_chan;struct cpsw_slave *slaves;+#define for_each_slave(priv, func, arg...) \
do { \int idx; \for (idx = 0; idx< (priv)->data.slaves; idx++) \(func)((priv)->slaves + idx, ##arg); \} while (0)+};
Can this stuff go in a header file?
This stuff is intended to be private within the driver. I can split this out into drivers/net/ti_cpsw.h.
If it's truly private within the driver and will never be needed by anybody else, the source file is OK, but since you're going to create a new header file in /include/net anyway, maybe you'll want to put some stuff there. Your call...
[...]
diff --git a/include/netdev.h b/include/netdev.h index 94eedfe..009e2f1 100644 --- a/include/netdev.h +++ b/include/netdev.h @@ -180,4 +180,33 @@ struct mv88e61xx_config { int mv88e61xx_switch_initialize(struct mv88e61xx_config *swconfig); #endif /* CONFIG_MV88E61XX_SWITCH */
+#ifdef CONFIG_DRIVER_TI_CPSW
+struct cpsw_slave_data {
u32 slave_reg_ofs;u32 sliver_reg_ofs;int phy_id;+};
+struct cpsw_platform_data {
u32 mdio_base;u32 cpsw_base;int mdio_div;int channels; /* number of cpdma channels (symmetric) */u32 cpdma_reg_ofs; /* cpdma register offset */int slaves; /* number of slave cpgmac ports */u32 ale_reg_ofs; /* address lookup engine reg offset */int ale_entries; /* ale table size */u32 host_port_reg_ofs; /* cpdma host port registers */u32 hw_stats_reg_ofs; /* cpsw hw stats counters */u32 mac_control;struct cpsw_slave_data *slave_data;void (*control)(int enabled);void (*phy_init)(char *name, int addr);+};
This stuff doesn't belong in this file. Definitions specific to your driver should go in /include/net/ti_cpsw.h (note that you'll be creating this directory, but that's where we want driver headers to go moving forward. Also, please call the initialize function 'ti_cpsw_initialize()'. Despite what the readme file recommends, you're initializing something, not registering it. If anything, the 'init()' functions are misnamed, but that's another discussion.
Will do.
We will post an updated v2 series shortly, with these changes.
Thanks Cyril.
regards, Ben