[U-Boot] [PATCH v4 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter

[Simon Glass]

On Mon, Apr 18, 2011 at 11:51 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
>> +/* Rx status word */
>> +#define RX_STS_FL_                   (0x3FFF0000)    /* Frame Length */
>> +#define RX_STS_ES_                   (0x00008000)    /* Error Summary */
> ...
>
> Please drop the parens around all these constants.  Please fix
> globally.

Done


>> +     ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
>> +     if (ret < 0) {
>> +             debug("Failed to write HW_CFG_BIR_ bit in HW_CFG "
>> +                     "register, ret = %d\n", ret);
>> +             return ret;
>> +     }
>
> You repeat these sequences of smsc95xx_read_reg() resp.
> smsc95xx_write_reg() followed by the "if (ret < 0)" part about 20
> times.
>
> Please move the respective debug() message for the failure case into
> the functions, and consider using a loop here instead.

I have moved the debug messages into the routines. Not sure what you
mean by a loop.

>
>> +     read_buf = DEFAULT_BULK_IN_DELAY;
>> +     ret = smsc95xx_write_reg(dev, BULK_IN_DLY, read_buf);
>> +     if (ret < 0) {
>> +             debug("ret = %d", ret);
>> +             return ret;
>> +     }
>
> Is it intentional to use the read_buf with a smsc95xx_write_reg()
> call?

It doesn't need to - I have changed it to use the value directly.

>> +     ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
>> +     if (ret < 0) {
>> +             debug("Failed to write HW_CFG register, ret=%d\n", ret);
>> +             return ret;
>> +     }
>
> Ditto here?

This is updating a register. It reads the value, modifies it and writes it back.
>
>> +     ret = smsc95xx_write_reg(dev, AFC_CFG, read_buf);
>> +     if (ret < 0) {
>> +             debug("Failed to write AFC_CFG: %d\n", ret);
>> +             return ret;
>> +     }
>
> And here?

Same as above.
>
>> +     ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
>> +     if (ret < 0) {
>> +             debug("Failed to read INT_EP_CTL: %d", ret);
>> +             return ret;
>> +     }
>
> And here we have &read_buf while everywhere else we have read_buf
> only?

That's because it is reading the value here. The read routine needs an
address of a variable to put the value into.

>
>
>
> Best regards,
>
> Wolfgang Denk

Regards,
Simon