Dear Mike Frysinger,
> On Monday, September 06, 2010 10:59:16 Reinhard Meyer wrote:
>> +int enc_miiphy_read(const char *devname, u8 phy_adr, u8 reg, u16 *value)
>> +{
>> + struct eth_device *dev = eth_get_dev_by_name(devname);
>> + if (dev) {
>> + enc_dev_t *enc = to_enc(dev);
>> +
>> + if (phy_adr != 0)
>> + return -1;
>> + spi_claim_bus(enc->slave);
>> + *value = phy_read(enc, reg);
>> + spi_release_bus(enc->slave);
>> + return 0;
>> + }
>> + return -1;
>> +}
>
> seems like this (and the write func) would be nicer if it were:
> if (!dev)
> return -1;
> ...
Umm, no. That would split enc_dev_t *enc = to_enc (dev) into two lines:
Declaration, if, assigment
>
>> +static int enc_init(struct eth_device *dev, bd_t *bis)
>> +{
>> + enc_dev_t *enc = to_enc(dev);
>> + u8 status;
>> + uint64_t etime;
>> +
>> + spi_claim_bus(enc->slave);
>
> check the return value
OK, for sake of completeness in every function using it..
Hmm blows up the code :)
Or should it suffice to do it once in the enc_init() - assuming no one will
likely claim the same bus in between and keep it?
>
> otherwise this looks pretty good ... i'll try and test on my hardware this
> week
Good. However after today's discussion about setting the mac address I had a
closer look at the eth_device structure. I decided to use the priv element
instead of the ugly (IMHO) container_of method. Although it means 2x malloc(),
its the cleaner way. But that need not defer you from testing.
One odd thing I noticed is that without "link up" reading the phy registers
fails with timeout after a few tries. I have found no clue as to why that is.
Can you observe this on your hardware, too?
Best Regards,
Reinhard
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
