From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guennadi Liakhovetski Subject: Re: [PATCH, RFC] Extend smc911x to support LAN921x chips Date: Sat, 11 Oct 2008 22:30:57 +0200 (CEST) Message-ID: References: <87vdx5gjzo.fsf@macbook.be.48ers.dk> <871vynr3yb.fsf@macbook.be.48ers.dk> <87k5cfoz7b.fsf@macbook.be.48ers.dk> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: Steve.Glendinning@smsc.com, netdev@vger.kernel.org, Ian.Saturley@smsc.com To: Peter Korsgaard Return-path: Received: from mail.gmx.net ([213.165.64.20]:50437 "HELO mail.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753924AbYJKUa6 (ORCPT ); Sat, 11 Oct 2008 16:30:58 -0400 In-Reply-To: <87k5cfoz7b.fsf@macbook.be.48ers.dk> Sender: netdev-owner@vger.kernel.org List-ID: On Sat, 11 Oct 2008, Peter Korsgaard wrote: > >>>>> "Guennadi" == Guennadi Liakhovetski writes: > > Hi, > > Guennadi> On Sat, 11 Oct 2008, Peter Korsgaard wrote: > >> >>>>> "Guennadi" == Guennadi Liakhovetski writes: > >> > >> It looks good, but you should definately split out the arm and cleanup > >> stuff from the new IDs. > > Guennadi> You mean remove the new IDs? Why? > > No, just have them in seperate patches - They are independent of each > other, so no need to put in the same patch. No, they are not independent. smc91x.c is comparing the ID read from the hardware with the table, which someone strangely enough for me put in the header. So, even if one were to split them, you would have to make it a patch series and make the patch for .c depend on the one for .h adding IDs. So, no, these two belong into one patch. > >> What's this for? Isn't that handled just above? > > Guennadi> I think, there's a bug in the code there. The break above > Guennadi> terminates the loop, yes, but then it falls through in the > Guennadi> switch statement to the default case and overwrites the > Guennadi> just found PHY. > > Ahh, the good old break-only-escapes-the-innermost-scope. That's a > good fix, but please send it seperately from the patch adding the new > IDs. Ok... it is a simple enough fix, so, one could put them both in one patch and be done with them, just explaining both in the patch description, but ok, I can make it a separate patch too... Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. 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@denx.de