Re: [PATCH v2] net: add Faraday FTMAC100 10/100 Ethernet driver

[Po-Yu Chuang]

Dear Joe,

On Tue, Jan 18, 2011 at 1:19 AM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Mon, 2011-01-17 at 17:21 +0800, Po-Yu Chuang wrote:
>
>> + * priveate data
>
> private

Fixed.

>> +static void ftmac100_enable_all_int(struct ftmac100 *priv)
>> +{
>> + Â Â unsigned int imr;
>> +
>> + Â Â imr = FTMAC100_INT_RPKT_FINISH | FTMAC100_INT_NORXBUF
>> + Â Â Â Â Â| FTMAC100_INT_XPKT_OK | FTMAC100_INT_XPKT_LOST
>> + Â Â Â Â Â| FTMAC100_INT_RPKT_LOST | FTMAC100_INT_AHB_ERR
>> + Â Â Â Â Â| FTMAC100_INT_PHYSTS_CHG;
>
> This could be a #define.

OK, done.

>> + Â Â maccr = FTMAC100_MACCR_XMT_EN |
>> + Â Â Â Â Â Â FTMAC100_MACCR_RCV_EN |
>> + Â Â Â Â Â Â FTMAC100_MACCR_XDMA_EN |
>> + Â Â Â Â Â Â FTMAC100_MACCR_RDMA_EN |
>> + Â Â Â Â Â Â FTMAC100_MACCR_CRC_APD |
>> + Â Â Â Â Â Â FTMAC100_MACCR_FULLDUP |
>> + Â Â Â Â Â Â FTMAC100_MACCR_RX_RUNT |
>> + Â Â Â Â Â Â FTMAC100_MACCR_RX_BROADPKT;
>
> Here too.

OK, done.

>> +static int ftmac100_rx_packet_error(struct ftmac100 *priv,
>> + Â Â Â Â Â Â struct ftmac100_rxdes *rxdes)
> []
>> + Â Â if (unlikely(ftmac100_rxdes_frame_too_long(rxdes))) {
>> + Â Â Â Â Â Â if (net_ratelimit())
>> + Â Â Â Â Â Â Â Â Â Â netdev_info(netdev, "rx frame too long\n");
>> +
>> + Â Â Â Â Â Â netdev->stats.rx_length_errors++;
>> + Â Â Â Â Â Â error = 1;
>> + Â Â }
>> +
>> + Â Â if (unlikely(ftmac100_rxdes_runt(rxdes))) {
>
> else if ?

OK, fixed.

>> +static int ftmac100_rx_packet(struct ftmac100 *priv, int *processed)
>> +{
>> + Â Â struct net_device *netdev = priv->netdev;
>> + Â Â struct ftmac100_rxdes *rxdes;
>> + Â Â struct sk_buff *skb;
>> + Â Â int length;
>> + Â Â int copied = 0;
>> + Â Â int done = 0;
>
> You could use bool/true/false here for copied and done
> and all the other uses of an int for a logical bool.

OK, fixed.

>> +static void ftmac100_txdes_set_dma_own(struct ftmac100_txdes *txdes)
>> +{
>> + Â Â /*
>> + Â Â Â* Make sure dma own bit will not be set before any other
>> + Â Â Â* descriptor fiels.
>
> field/fields

Fixed.

>> +static int ftmac100_mdio_read(struct net_device *netdev, int phy_id, int reg)
>> +{
>> + Â Â struct ftmac100 *priv = netdev_priv(netdev);
>> + Â Â int phycr;
>> + Â Â int i;
>> +
>> + Â Â phycr = FTMAC100_PHYCR_PHYAD(phy_id) |
>> + Â Â Â Â Â Â FTMAC100_PHYCR_REGAD(reg) |
>> + Â Â Â Â Â Â FTMAC100_PHYCR_MIIRD;
>> +
>> + Â Â iowrite32(phycr, priv->base + FTMAC100_OFFSET_PHYCR);
>> + Â Â for (i = 0; i < 10; i++) {
>> + Â Â Â Â Â Â phycr = ioread32(priv->base + FTMAC100_OFFSET_PHYCR);
>> +
>> + Â Â Â Â Â Â if ((phycr & FTMAC100_PHYCR_MIIRD) == 0)
>> + Â Â Â Â Â Â Â Â Â Â return phycr & FTMAC100_PHYCR_MIIRDATA;
>> +
>> + Â Â Â Â Â Â usleep_range(100, 1000);
>> + Â Â }
>> +
>> + Â Â netdev_err(netdev, "mdio read timed out\n");
>> + Â Â return 0xffff;
>
> 0xffff is a rather odd return, perhaps a #define?

After a little digging in drivers/net/mii.c, it seems that mii lib does not
check return value if it is error. So I guess I should return 0 if error.

>> +/******************************************************************************
>> + * initialization / finalization
>> + *****************************************************************************/
>> +static int __init ftmac100_init(void)
>> +{
>> + Â Â printk(KERN_INFO "Loading " DRV_NAME ": version " DRV_VERSION " ...\n");
>
> You could use
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> before any #include and
> Â Â Â Âpr_info("Loading version " DRV_VERSION " ...\n");

OK

> One last comment on split long line indentation style
> and long function declarations.
>
> There's no required style so you can use what you are
> most comfortable doing.
>
> Most of drivers/net uses an alignment to open parenthesis
> using maximal tabs and minimal necessary spaces instead of
> an extra tabstop.
>
> Like:
>
> static int some_long_function(type var1, type var2...
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtype varN)
> and
> Â Â Â Âsome_long_function(var1, var2, ...
> Â Â Â Â Â Â Â Â Â Â Â Â Â varN);
>
> not
> static int some_long_function(type var1, type var2...
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âtype varN)
> and
> Â Â Â Âsome_long_function(var1, var2, ...
> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂvarN);

Well, TBH, I don't like this style because if I changed the
function name, the indentation might need to be adjusted.

Even worse, I got an infeasible case :-(

static struct ftmac100_rxdes *ftmac100_rx_locate_first_segment(
							       struct ftmac100 *priv)

I know my function names are quite long, but I like them to be descriptive.
Do you really insist on it?

Thanks,
Po-Yu Chuang
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/