From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [PATCH 5/8] powerpc: i2c-mpc: make I2C bus speed configurable Date: Wed, 1 Apr 2009 07:30:42 -0600 Message-ID: References: <20090331123727.853787299@denx.de> <20090331124035.908249543@denx.de> <49D31D1D.6020004@grandegger.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <49D31D1D.6020004@grandegger.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@ozlabs.org Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@ozlabs.org To: Wolfgang Grandegger Cc: linuxppc-dev@ozlabs.org, devicetree-discuss@ozlabs.org List-Id: devicetree@vger.kernel.org On Wed, Apr 1, 2009 at 1:51 AM, Wolfgang Grandegger wro= te: > Grant Likely wrote: >> The table definition is more verbose this way, but I think it results >> in more understandable and easier to extend code. =A0It also adds lets >> the compiler do more type checking for you. > > OK but I don't like the callback function to do the settings. We need > backward compatibility with old DTS files including the ugly "dfsrr" > property, right? Then it seems consequent to continue using i2c->flags > for that purpose and not to introduce another method. If we don't need > backward compatibility, we could drop the flags completely and just use > callback functions. I don't understand why you don't like it. It's an elegant solution and it simplifies the code somewhat. After grabbing the callback pointer the compatibility code can simply override it. But I won't belabor the point or oppose the patch if you stick with the flags pointer. >>> --- linux-2.6.orig/arch/powerpc/platforms/52xx/mpc52xx_common.c 2009-03= -31 13:25:08.000000000 +0200 >>> +++ linux-2.6/arch/powerpc/platforms/52xx/mpc52xx_common.c =A0 =A0 =A02= 009-03-31 13:28:54.309718526 +0200 >>> +int fsl_i2c_get_fdr(struct device_node *node, u32 i2c_clock, u32 i2c_f= lags) >>> +{ >>> [...] >>> +} >>> +EXPORT_SYMBOL(fsl_i2c_get_fdr); >> >> does not work on a multiplatform kernel. =A0Both 8xxx and 52xx support >> can be selected at the same time. > > OK, then we need different functions including stubs. I've been thinking about this more. These tables are only ever going to be used by the i2c_mpc driver and so really they are a part of the i2c_mpc driver itself. Putting them into common code doesn't make any sense because it is not common code. I will not merge a patch that puts them into mpc5200 common code. g. -- = Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.