[U-Boot] [PATCH v2 3/7] usb: eth: introduce support for Moschip USB ethernet

[Gerhard Sittig <gsi@denx.de>]

On Mon, Feb 17, 2014 at 21:57 +0100, Marek Vasut wrote:
> 
> On Monday, February 17, 2014 at 08:35:23 PM, Gerhard Sittig wrote:
> 
> [...]
> 
> > +int mcs7830_eth_get_info(struct usb_device *dev, struct ueth_data *ss,
> > +			 struct eth_device *eth)
> > +{
> > +	debug("%s()\n", __func__);
> > +	if (!eth) {
> > +		debug("%s: missing parameter.\n", __func__);
> > +		return 0;
> > +	}
> > +
> > +	snprintf(eth->name, sizeof(eth->name), "%s%d",
> > +		 MCS7830_BASE_NAME, mcs7830_iface_idx++);
> > +	eth->init = mcs7830_init;
> > +	eth->send = mcs7830_send;
> > +	eth->recv = mcs7830_recv;
> > +	eth->halt = mcs7830_halt;
> > +	eth->write_hwaddr = mcs7830_write_mac;
> > +	eth->priv = ss;
> > +
> > +	if (mcs7830_basic_reset(ss))
> > +		return 0;
> > +
> > +#ifdef DEBUG
> > +	(void)mcs7830_read_config(eth);
> 
> So this is debug-only function? You might want to put the entire function into 
> #ifdef DEBUG and then have an #else , where you define the function as an empty 
> one. The GCC shall handle the rest then as well, but you won't have this ugly 
> ifdef in a function.

I thought about this for a while.

Usually you'd expect separate control and status registers in
hardware.  Where you write to control, and read back from status.
Here those two aspects appear to have been mixed into one
"config" register, and only in hindsight the reading became
unused.  It's not so much an intent, but more of a byproduct.

During development (before the driver became operational), I
could not tell whether I had to read-modify-write that config
register.  Following the Linux driver's approach, currently only
fixed values get written to the adapter and nothing gets read
back.

Later the shadow in the driver's private data was introduced,
such that "updates" neither need to read back what was written
before.  And since neither multicast nor promiscuous mode may
apply to bootloader operation, even those updates may never need
not occur.

In the meantime I'd even tend to support the removal of the
config register read routine.  Adding code "just in case" is a
programmer's sin that may not be acceptable in U-Boot, since the
cost outweights the benefit.

The current implementation (v3, with "maybe unused" decoration)
might be acceptable.  But should feedback suggest that v4 is
needed, I will remove that routine as well.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de