On Mon, Apr 18, 2011 at 11:51 AM, Wolfgang Denk wd@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