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

[Marek Vasut]

On Sunday, February 23, 2014 at 09:16:20 PM, Gerhard Sittig wrote:
> 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.

If you feel like removing the routine, then please remove it .

Best regards,
Marek Vasut