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

[Po-Yu Chuang]

Dear Joe,

On Thu, Jan 13, 2011 at 11:39 PM, Joe Perches <joe@xxxxxxxxxxx> wrote:
> On Thu, 2011-01-13 at 19:49 +0800, Po-Yu Chuang wrote:
>> From: Po-Yu Chuang <ratbert@xxxxxxxxxxxxxxxx>
>>
>> FTMAC100 Ethernet Media Access Controller supports 10/100 Mbps and
>> MII. ÂThis driver has been working on some ARM/NDS32 SoC including
>> Faraday A320 and Andes AG101.
>
> A couple of trivial comments not already mentioned by others.
>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> []
>> @@ -2014,6 +2014,15 @@ config BCM63XX_ENET
>> Â Â Â Â This driver supports the ethernet MACs in the Broadcom 63xx
>> Â Â Â Â MIPS chipset family (BCM63XX).
>>
>> +config FTMAC100
>> + Â Â tristate "Faraday FTMAC100 10/100 Ethernet support"
>> + Â Â depends on ARM
>> + Â Â select MII
>> + Â Â help
>> + Â Â Â This driver supports the FTMAC100 Ethernet controller from
>> + Â Â Â Faraday. It is used on Faraday A320, Andes AG101, AG101P
>> + Â Â Â and some other ARM/NDS32 SoC's.
>> +
>
> ARM specific net drivers are for now in drivers/net/arm/
> Perhaps it's better to locate these files there?

This controller is used by not only ARM-base platforms, but also
NDS32-base ones.
NDS32 is an ISA designed by Andes tech. Although it is not yet in the mainline,
they plan to push their stuff to mainline and are working on that.

So... I don't know if it is better to put my driver to driver/net/arm/.

Should I leave my driver at driver/net/ or put it in drvier/net/arm/
and let Andes tech guys
move it out of driver/net/arm/ when they use it later?

>> + Â Â if (unlikely(ftmac100_rxdes_rx_error(rxdes))) {
>> + Â Â Â Â Â Â if (printk_ratelimit())
>> + Â Â Â Â Â Â Â Â Â Â dev_info(dev, "rx err\n");
>
> There are many printk_ratelimit() tests.
>
> It's better to use net_ratelimit() or a local ratelimit_state
> so there's less possible suppression of other subsystem
> messages.

OK, fixed.


Thanks a lot for your absolutely non-trivial review. :-)
I'll submit a new version ASAP.

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/