On Mon, Feb 27, 2012 at 11:20:40PM +0100, Jean Delvare wrote:
> Hi Daniel,
>
> Sorry for the late reply.
>
> On Tue, 14 Feb 2012 22:37:21 +0100, Daniel Vetter wrote:
> > i915 has a hw i2c controller (gmbus) but for a bunch of stupid reasons
> > we need to be able to fall back to the bit-banging algo on gpio pins.
> >
> > The current code sets up a 2nd i2c controller for the same i2c bus using
> > the bit-banging algo. This has a bunch of issues, the major one being
> > that userspace can directly access this fallback i2c adaptor behind
> > the drivers back.
> >
> > But we need to frob a few registers before and after using fallback
> > gpio bit-banging, so this horribly fails.
>
> You could use the pre_xfer and post_xfer hooks together with a shared
> mutex to solve the problem. But I agree its much cleaner to expose a
> single adapter in the first place.
Yeah, I've seen these and I think we could use them. Still it's cleaner to
only expose one algo for a single bus.
> If you need to hot-switch between hardware and bit-banged I2C, I suggest
> that you lock the bus while doing so, to avoid switching while a
> transaction is in progress. This can be achieved with
> i2c_lock_adapter() and i2c_unlock_adapter().
The drm/i915 xfer function is currently protected by a single mutex
(because the hw i2c controller can only be used on one bus at a time). So
I think we're covered. Also we do the fallback in our xfer function when
we notice that things don't quite work as they should, so we actually want
to switch while a transfer is in progress. Dunno whether that's the best
approach, but the current code is structured like this.
> > The new plan is to only set up one i2c adaptor and transparently fall
> > back to bit-banging by directly calling the xfer function of the bit-
> > banging algo in the i2c core.
> >
> > To make that possible, export the 2 i2c algo functions.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> > drivers/i2c/algos/i2c-algo-bit.c | 12 +++++++-----
> > include/linux/i2c-algo-bit.h | 4 ++++
> > 2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c
> > index 525c734..ec1651a 100644
> > --- a/drivers/i2c/algos/i2c-algo-bit.c
> > +++ b/drivers/i2c/algos/i2c-algo-bit.c
> > @@ -531,8 +531,8 @@ static int bit_doAddress(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
> > return 0;
> > }
> >
> > -static int bit_xfer(struct i2c_adapter *i2c_adap,
> > - struct i2c_msg msgs[], int num)
> > +int i2c_bit_xfer(struct i2c_adapter *i2c_adap,
> > + struct i2c_msg msgs[], int num)
> > {
> > struct i2c_msg *pmsg;
> > struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> > @@ -598,21 +598,23 @@ bailout:
> > adap->post_xfer(i2c_adap);
> > return ret;
> > }
> > +EXPORT_SYMBOL(i2c_bit_xfer);
> >
> > -static u32 bit_func(struct i2c_adapter *adap)
> > +u32 i2c_bit_func(struct i2c_adapter *adap)
> > {
> > return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> > I2C_FUNC_SMBUS_READ_BLOCK_DATA |
> > I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
> > I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
> > }
> > +EXPORT_SYMBOL(i2c_bit_func);
> >
> >
> > /* -----exported algorithm data: ------------------------------------- */
> >
> > static const struct i2c_algorithm i2c_bit_algo = {
> > - .master_xfer = bit_xfer,
> > - .functionality = bit_func,
> > + .master_xfer = i2c_bit_xfer,
> > + .functionality = i2c_bit_func,
> > };
> >
> > /*
> > diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h
> > index 4f98148..cdd6336 100644
> > --- a/include/linux/i2c-algo-bit.h
> > +++ b/include/linux/i2c-algo-bit.h
> > @@ -47,6 +47,10 @@ struct i2c_algo_bit_data {
> > int timeout; /* in jiffies */
> > };
> >
> > +int i2c_bit_xfer(struct i2c_adapter *i2c_adap,
> > + struct i2c_msg msgs[], int num);
> > +u32 i2c_bit_func(struct i2c_adapter *adap);
> > +
> > int i2c_bit_add_bus(struct i2c_adapter *);
> > int i2c_bit_add_numbered_bus(struct i2c_adapter *);
>
> I have no problem with this in the principle. In the details, I don't
> understand why you don't simply export i2c_bit_algo? This is one fewer
> export and should serve the same purpose - even allowing faster
> initializations in some cases.
I've figured that hunting for magic functions in a struct is a bit to
intransparent and hence exported the 2 functions I needed instead of the
struct. But if you prefer me to export the vtable I'll gladly do so - that
way I can drop a patch to rename some functions in drm/nouveau that
conflict with these here.
> Looking a bit more to the i2c-algo-bit code, I also notice that
> bypassing i2c_bit_add_bus() means you'll miss some of its features,
> namely bus testing, default retries value and warning on non-compliant
> buses. While none of these are mandatory, it may make sense to export a
> new function i2c_bit_add_numbered_bus() which would call
> __i2c_bit_add_bus() with no callback function. If you do that, you may
> not have to export anything else.
I've noticed that the the bit-banging algo does some test upon
registration but figured that it's not worth the fuss to add a new init
function that avoids the registration.
> I leave it up to you which way you want to implement it, all are fine
> with me, and we can always change later if more use cases show up which
> would work better with a different implementation.
Ok, I'll go with just exporting the vtable then.
Thanks for the comments.
Yours, Daniel
--
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
