From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Deucher Subject: Re: Using algo-bit in another i2c algo Date: Wed, 10 Mar 2010 11:58:38 -0500 Message-ID: References: <20100310104401.57fc43b2@hyperion.delvare> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20100310104401.57fc43b2-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> Sender: linux-i2c-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jean Delvare Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-i2c@vger.kernel.org On Wed, Mar 10, 2010 at 4:44 AM, Jean Delvare wrot= e: > Hi Alex, > > Sorry for the late answer. > > On Mon, 11 Jan 2010 17:13:50 -0500, Alex Deucher wrote: >> Hi, >> >> =A0 =A0 I'm adding support for the i2c controllers on radeon hardwar= e and >> I have a few questions. =A0I have a radeon-algo that encapsulates al= l >> the various hw i2c controller functionality, however, it uses a >> bit-algo bus internally for cases where you have to use bit-banging >> rather than the hardware i2c engines. =A0Also, for bit banging to wo= rk >> properly, you need to do some things before the bit-algo transaction >> (basically masking the gpios for software use). > > There used to be a patch floating around that added pre- and post-xfe= r > hooks to i2c-algo-bit... I couldn't find it again, so I rewrote it: > > From: Jean Delvare > Subject: i2c-algo-bit: Add pre- and post-xfer hooks > > Drivers might have to do random things before and/or after I2C > transfers. Add hooks to the i2c-algo-bit implementation to let them d= o > so. > > Signed-off-by: Jean Delvare > --- > =A0drivers/i2c/algos/i2c-algo-bit.c | =A0 =A09 +++++++++ > =A0include/linux/i2c-algo-bit.h =A0 =A0 | =A0 =A02 ++ > =A02 files changed, 11 insertions(+) > > --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c =A0 =A0 =A0= 2009-06-10 05:05:27.000000000 +0200 > +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c =A0 2010-03-09 = 18:52:50.000000000 +0100 > @@ -522,6 +522,12 @@ static int bit_xfer(struct i2c_adapter * > =A0 =A0 =A0 =A0int i, ret; > =A0 =A0 =A0 =A0unsigned short nak_ok; > > + =A0 =A0 =A0 if (adap->pre_xfer) { > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D adap->pre_xfer(i2c_adap, adap->= data); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret < 0) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret; > + =A0 =A0 =A0 } > + > =A0 =A0 =A0 =A0bit_dbg(3, &i2c_adap->dev, "emitting start condition\n= "); > =A0 =A0 =A0 =A0i2c_start(adap); > =A0 =A0 =A0 =A0for (i =3D 0; i < num; i++) { > @@ -570,6 +576,9 @@ static int bit_xfer(struct i2c_adapter * > =A0bailout: > =A0 =A0 =A0 =A0bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n"= ); > =A0 =A0 =A0 =A0i2c_stop(adap); > + > + =A0 =A0 =A0 if (adap->post_xfer) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 adap->post_xfer(i2c_adap, adap->data); > =A0 =A0 =A0 =A0return ret; > =A0} > > --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h =A02009-06-10 = 05:05:27.000000000 +0200 > +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h =A0 =A0 =A0 2010-03= -09 18:53:06.000000000 +0100 > @@ -32,6 +32,8 @@ > =A0*/ > =A0struct i2c_algo_bit_data { > =A0 =A0 =A0 =A0void *data; =A0 =A0 =A0 =A0 =A0 =A0 /* private data fo= r lowlevel routines */ > + =A0 =A0 =A0 int =A0(*pre_xfer) =A0(struct i2c_adapter *i2c_adap, vo= id *data); > + =A0 =A0 =A0 void (*post_xfer) (struct i2c_adapter *i2c_adap, void *= data); > =A0 =A0 =A0 =A0void (*setsda) (void *data, int state); > =A0 =A0 =A0 =A0void (*setscl) (void *data, int state); > =A0 =A0 =A0 =A0int =A0(*getsda) (void *data); > > Would it help? > I think that would do the trick. >> Right now we use >> bit-algo i2c for the ddc buses, but they won't work externally to th= e >> driver without the proper gpio masking prior to using them. =A0In th= e >> radeon-algo patches, I use bit algo internally when I cannot use the >> hardware i2c engines, or in cases where I haven't implemented suppor= t >> yet for the hardware engine (as most gpios can be driven by sw or th= e >> hw engine). =A0The problem is, this exposes the i2c bit-algo buses a= s >> well as the radeon-algo buses. > > This is bad, as it defeats the slave address uniqueness mechanism. Ba= d > things could happen. Would the patch above be sufficient to solve you= r > case? yes. > >> Is there a way to not expose the bit-algo buses that are used >> internally? > > No, this is not possible at the time being. That being said, it > shouldn't be difficult, if the need exists. Looking at i2c-algo-bit, > the registration part is split into a preparation step and the actual > registration with i2c-core. The preparation step if done by > i2c_bit_prepare_bus(). If we exported this function, then you could > easily call it to create an internal i2c-algo-bit-based adapter, whic= h > you wouldn't have to register if you don't want to: > > From: Jean Delvare > Subject: i2c-algo-bit: Export i2c_bit_prepare_bus > > This lets drivers make use of the i2c-algo-bit implementation without > actually registering the i2c adapter in question. This is useful to > drivers which need more than the i2c-algo-bit driver offers. > > Signed-off-by: Jean Delvare > --- > =A0drivers/i2c/algos/i2c-algo-bit.c | =A0 =A03 ++- > =A0include/linux/i2c-algo-bit.h =A0 =A0 | =A0 =A01 + > =A02 files changed, 3 insertions(+), 1 deletion(-) > > --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c =A0 =A0 =A0= 2010-03-10 08:09:03.000000000 +0100 > +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c =A0 2010-03-10 = 10:23:51.000000000 +0100 > @@ -601,7 +601,7 @@ static const struct i2c_algorithm i2c_bi > =A0/* > =A0* registering functions to load algorithms at runtime > =A0*/ > -static int i2c_bit_prepare_bus(struct i2c_adapter *adap) > +int i2c_bit_prepare_bus(struct i2c_adapter *adap) > =A0{ > =A0 =A0 =A0 =A0struct i2c_algo_bit_data *bit_adap =3D adap->algo_data= ; > > @@ -617,6 +617,7 @@ static int i2c_bit_prepare_bus(struct i2 > > =A0 =A0 =A0 =A0return 0; > =A0} > +EXPORT_SYMBOL(i2c_bit_prepare_bus); > > =A0int i2c_bit_add_bus(struct i2c_adapter *adap) > =A0{ > --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h =A02010-03-10 = 08:09:03.000000000 +0100 > +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h =A0 =A0 =A0 2010-03= -10 10:24:03.000000000 +0100 > @@ -47,6 +47,7 @@ struct i2c_algo_bit_data { > =A0 =A0 =A0 =A0int timeout; =A0 =A0 =A0 =A0 =A0 =A0/* in jiffies */ > =A0}; > > +int i2c_bit_prepare_bus(struct i2c_adapter *); > =A0int i2c_bit_add_bus(struct i2c_adapter *); > =A0int i2c_bit_add_numbered_bus(struct i2c_adapter *); > > > With this possibility, I guess you wouldn't even need the first patch > any longer? If both are needed, that's fine with me, but if only one = is > needed, that's even better. > >> I've attached the patches for reference. > > Please let me know if either or both of my 2 proposals above fit your > needs. I think either of these patches should do the trick. I'll test them out this week. I'm inclined to take the second one as it requires less work in my driver, and I can fall back to big banging in my algo if there is a problem with the hw i2c engine. However there may be devices out there that would prefer the first method. I suppose we can cross that bridge when the time comes. Alex