Re: [U-Boot] [PATCH v3 03/11] net: add Faraday FTMAC110 10/100Mbps ethernet support

2013-05-02 Thread Kuo-Jung Su
2013/5/3 Tom Rini :
> On Fri, Apr 26, 2013 at 04:02:32PM +0800, Kuo-Jung Su wrote:
>
>> From: Kuo-Jung Su 
> [snip]
>> + | (phyaddr << PHYCR_ADDR_SHIFT)
>> + | (phyreg  << PHYCR_REG_SHIFT)
>> + | 0x3000;
>
> Magic number.
>
It's the HW debug function, it would be removed at next version.

>> +
>> + writel(tmp, ®s->phycr);
>> +
>> + for (ts = get_timer(0); get_timer(ts) < 1000; ) {
>
> Please define a TIMEOUT and use that insteadof 1000 all the time.
>

Got it, thanks

> [snip]
>> + /* interrupt at every packet transmit/receive */
>> + writel(0x1010, ®s->itc);
>> + /* tx/rx poll interval=5.12us; rx_poll_cnt=1 */
>> + writel(0x0001, ®s->aptc);
>> + /* rx fifo: high=1536, low=512 */
>> + writel(0x0390, ®s->dblac);
>> + /* clear all interrupt status */
>> + writel(0x03FF, ®s->isr);
>
> More magic numbers.  Please fix globally.  Thanks!
>

Got it, thanks

> --
> Tom



--
Best wishes,
Kuo-Jung Su
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH v3 03/11] net: add Faraday FTMAC110 10/100Mbps ethernet support

2013-05-02 Thread Tom Rini
On Fri, Apr 26, 2013 at 04:02:32PM +0800, Kuo-Jung Su wrote:

> From: Kuo-Jung Su 
[snip]
> + | (phyaddr << PHYCR_ADDR_SHIFT)
> + | (phyreg  << PHYCR_REG_SHIFT)
> + | 0x3000;

Magic number.

> +
> + writel(tmp, ®s->phycr);
> +
> + for (ts = get_timer(0); get_timer(ts) < 1000; ) {

Please define a TIMEOUT and use that insteadof 1000 all the time.

[snip]
> + /* interrupt at every packet transmit/receive */
> + writel(0x1010, ®s->itc);
> + /* tx/rx poll interval=5.12us; rx_poll_cnt=1 */
> + writel(0x0001, ®s->aptc);
> + /* rx fifo: high=1536, low=512 */
> + writel(0x0390, ®s->dblac);
> + /* clear all interrupt status */
> + writel(0x03FF, ®s->isr);

More magic numbers.  Please fix globally.  Thanks!

-- 
Tom


signature.asc
Description: Digital signature
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot