From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gerhard Sittig Date: Sun, 23 Feb 2014 21:16:20 +0100 Subject: [U-Boot] [PATCH v2 3/7] usb: eth: introduce support for Moschip USB ethernet In-Reply-To: <201402172157.04438.marex@denx.de> References: <1392665727-5734-1-git-send-email-gsi@denx.de> <1392665727-5734-4-git-send-email-gsi@denx.de> <201402172157.04438.marex@denx.de> Message-ID: <20140223201620.GH3327@book.gsilab.sittig.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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