[U-Boot] [PATCH 1/2] TI: netdev: add driver for cpsw ethernet device

Ben Warren
2010-09-01T21:47:54Z

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