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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
