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?
[...]
>> +++ 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.
[...]
>> 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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
