[U-Boot] [PATCH v2] NET: add ENC28J60 driver using SPI framework

[Reinhard Meyer]

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