This way we can free up the bus->adaptor.algo_data pointer and make it available for use with the bitbanging fallback algo.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 13 ++++++++----- drivers/gpu/drm/i915/intel_i2c.c | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8e3eb5e..ed40743 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -290,6 +290,13 @@ enum intel_pch { struct intel_fbdev; struct intel_fbc_work;
+struct intel_gmbus { + struct i2c_adapter adapter; + struct i2c_adapter *force_bit; + u32 reg0; + struct drm_i915_private *dev_priv; +}; + typedef struct drm_i915_private { struct drm_device *dev;
@@ -307,11 +314,7 @@ typedef struct drm_i915_private { /** gt_lock is also taken in irq contexts. */ struct spinlock gt_lock;
- struct intel_gmbus { - struct i2c_adapter adapter; - struct i2c_adapter *force_bit; - u32 reg0; - } *gmbus; + struct intel_gmbus *gmbus;
/** gmbus_mutex protects against concurrent usage of the single hw gmbus * controller on different i2c buses. */ diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index fc75d71..f496510 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -232,7 +232,7 @@ gmbus_xfer(struct i2c_adapter *adapter, struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus, adapter); - struct drm_i915_private *dev_priv = adapter->algo_data; + struct drm_i915_private *dev_priv = bus->dev_priv; int i, reg_offset, ret;
mutex_lock(&dev_priv->gmbus_mutex); @@ -401,7 +401,7 @@ int intel_setup_gmbus(struct drm_device *dev) names[i]);
bus->adapter.dev.parent = &dev->pdev->dev; - bus->adapter.algo_data = dev_priv; + bus->dev_priv = dev_priv;
bus->adapter.algo = &gmbus_algorithm; ret = i2c_add_adapter(&bus->adapter); @@ -442,7 +442,7 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
if (force_bit) { if (bus->force_bit == NULL) { - struct drm_i915_private *dev_priv = adapter->algo_data; + struct drm_i915_private *dev_priv = bus->dev_priv; bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff); }
I'd like to export the corresponding functions from the i2c core so that I can use them in fallback bit-banging in i915.ko
Cc: nouveau@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/nouveau/nouveau_i2c.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_i2c.c b/drivers/gpu/drm/nouveau/nouveau_i2c.c index 820ae7f..7a7e751 100644 --- a/drivers/gpu/drm/nouveau/nouveau_i2c.c +++ b/drivers/gpu/drm/nouveau/nouveau_i2c.c @@ -242,7 +242,7 @@ i2c_addr(struct nouveau_i2c_chan *port, struct i2c_msg *msg) }
static int -i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +nouveau_i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct nouveau_i2c_chan *port = (struct nouveau_i2c_chan *)adap; struct i2c_msg *msg = msgs; @@ -272,14 +272,14 @@ i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) }
static u32 -i2c_bit_func(struct i2c_adapter *adap) +nouveau_i2c_bit_func(struct i2c_adapter *adap) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
const struct i2c_algorithm i2c_bit_algo = { - .master_xfer = i2c_bit_xfer, - .functionality = i2c_bit_func + .master_xfer = nouveau_i2c_bit_xfer, + .functionality = nouveau_i2c_bit_func };
static const uint32_t nv50_i2c_port[] = {
Hi Ben,
Can you ack this patch for merging through drm-intel-next? Imo that's the easiest way to get it in (assuming you don't have any objections).
Thanks, Daniel
On Tue, Feb 14, 2012 at 10:37:20PM +0100, Daniel Vetter wrote:
I'd like to export the corresponding functions from the i2c core so that I can use them in fallback bit-banging in i915.ko
Cc: nouveau@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/nouveau/nouveau_i2c.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_i2c.c b/drivers/gpu/drm/nouveau/nouveau_i2c.c index 820ae7f..7a7e751 100644 --- a/drivers/gpu/drm/nouveau/nouveau_i2c.c +++ b/drivers/gpu/drm/nouveau/nouveau_i2c.c @@ -242,7 +242,7 @@ i2c_addr(struct nouveau_i2c_chan *port, struct i2c_msg *msg) }
static int -i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) +nouveau_i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct nouveau_i2c_chan *port = (struct nouveau_i2c_chan *)adap; struct i2c_msg *msg = msgs; @@ -272,14 +272,14 @@ i2c_bit_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) }
static u32 -i2c_bit_func(struct i2c_adapter *adap) +nouveau_i2c_bit_func(struct i2c_adapter *adap) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
const struct i2c_algorithm i2c_bit_algo = {
- .master_xfer = i2c_bit_xfer,
- .functionality = i2c_bit_func
- .master_xfer = nouveau_i2c_bit_xfer,
- .functionality = nouveau_i2c_bit_func
};
static const uint32_t nv50_i2c_port[] = {
1.7.7.5
I'd like to export the corresponding functions from the i2c core so that I can use them in fallback bit-banging in i915.ko
v2: Adapt to new i2c export patch.
Cc: nouveau@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/nouveau/nouveau_i2c.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_i2c.c b/drivers/gpu/drm/nouveau/nouveau_i2c.c index 820ae7f..8f4f914 100644 --- a/drivers/gpu/drm/nouveau/nouveau_i2c.c +++ b/drivers/gpu/drm/nouveau/nouveau_i2c.c @@ -277,7 +277,7 @@ i2c_bit_func(struct i2c_adapter *adap) return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; }
-const struct i2c_algorithm i2c_bit_algo = { +const struct i2c_algorithm nouveau_i2c_bit_algo = { .master_xfer = i2c_bit_xfer, .functionality = i2c_bit_func }; @@ -384,12 +384,12 @@ nouveau_i2c_init(struct drm_device *dev) case 0: /* NV04:NV50 */ port->drive = entry[0]; port->sense = entry[1]; - port->adapter.algo = &i2c_bit_algo; + port->adapter.algo = &nouveau_i2c_bit_algo; break; case 4: /* NV4E */ port->drive = 0x600800 + entry[1]; port->sense = port->drive; - port->adapter.algo = &i2c_bit_algo; + port->adapter.algo = &nouveau_i2c_bit_algo; break; case 5: /* NV50- */ port->drive = entry[0] & 0x0f; @@ -402,7 +402,7 @@ nouveau_i2c_init(struct drm_device *dev) port->drive = 0x00d014 + (port->drive * 0x20); port->sense = port->drive; } - port->adapter.algo = &i2c_bit_algo; + port->adapter.algo = &nouveau_i2c_bit_algo; break; case 6: /* NV50- DP AUX */ port->drive = entry[0];
On Tue, Feb 28, 2012 at 12:42:19AM +0100, Daniel Vetter wrote:
I'd like to export the corresponding functions from the i2c core so that I can use them in fallback bit-banging in i915.ko
v2: Adapt to new i2c export patch.
Cc: nouveau@lists.freedesktop.org Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
I've applied this patch to d-i-n with Dave's irc-acked. -Daniel
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.
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@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 *);
Hi Jean,
Can you please review and if you don't have any objectsion, ack this patch for merging through drm-intel-next? Imo that's the easiest way to merge this series.
Thanks, Daniel
On Tue, Feb 14, 2012 at 10:37:21PM +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.
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@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 *);
-- 1.7.7.5
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.
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 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@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.
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 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.
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@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
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.
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.
v2: As suggested by Jean Delvare, simply export the i2c_bit_algo vtable instead of the individual functions.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/i2c/algos/i2c-algo-bit.c | 3 ++- include/linux/i2c-algo-bit.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c index 525c734..ad0459c 100644 --- a/drivers/i2c/algos/i2c-algo-bit.c +++ b/drivers/i2c/algos/i2c-algo-bit.c @@ -610,10 +610,11 @@ static u32 bit_func(struct i2c_adapter *adap)
/* -----exported algorithm data: ------------------------------------- */
-static const struct i2c_algorithm i2c_bit_algo = { +const struct i2c_algorithm i2c_bit_algo = { .master_xfer = bit_xfer, .functionality = bit_func, }; +EXPORT_SYMBOL(i2c_bit_algo);
/* * registering functions to load algorithms at runtime diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h index 4f98148..584ffa0 100644 --- a/include/linux/i2c-algo-bit.h +++ b/include/linux/i2c-algo-bit.h @@ -49,5 +49,6 @@ struct i2c_algo_bit_data {
int i2c_bit_add_bus(struct i2c_adapter *); int i2c_bit_add_numbered_bus(struct i2c_adapter *); +extern const struct i2c_algorithm i2c_bit_algo;
#endif /* _LINUX_I2C_ALGO_BIT_H */
On Tue, 28 Feb 2012 00:39:39 +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.
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.
v2: As suggested by Jean Delvare, simply export the i2c_bit_algo vtable instead of the individual functions.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/i2c/algos/i2c-algo-bit.c | 3 ++- include/linux/i2c-algo-bit.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c index 525c734..ad0459c 100644 --- a/drivers/i2c/algos/i2c-algo-bit.c +++ b/drivers/i2c/algos/i2c-algo-bit.c @@ -610,10 +610,11 @@ static u32 bit_func(struct i2c_adapter *adap)
/* -----exported algorithm data: ------------------------------------- */
-static const struct i2c_algorithm i2c_bit_algo = { +const struct i2c_algorithm i2c_bit_algo = { .master_xfer = bit_xfer, .functionality = bit_func, }; +EXPORT_SYMBOL(i2c_bit_algo);
/*
- registering functions to load algorithms at runtime
diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h index 4f98148..584ffa0 100644 --- a/include/linux/i2c-algo-bit.h +++ b/include/linux/i2c-algo-bit.h @@ -49,5 +49,6 @@ struct i2c_algo_bit_data {
int i2c_bit_add_bus(struct i2c_adapter *); int i2c_bit_add_numbered_bus(struct i2c_adapter *); +extern const struct i2c_algorithm i2c_bit_algo;
#endif /* _LINUX_I2C_ALGO_BIT_H */
Acked-by: Jean Delvare khali@linux-fr.org
On Tue, Feb 28, 2012 at 09:08:17AM +0100, Jean Delvare wrote:
On Tue, 28 Feb 2012 00:39:39 +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.
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.
v2: As suggested by Jean Delvare, simply export the i2c_bit_algo vtable instead of the individual functions.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/i2c/algos/i2c-algo-bit.c | 3 ++- include/linux/i2c-algo-bit.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/algos/i2c-algo-bit.c b/drivers/i2c/algos/i2c-algo-bit.c index 525c734..ad0459c 100644 --- a/drivers/i2c/algos/i2c-algo-bit.c +++ b/drivers/i2c/algos/i2c-algo-bit.c @@ -610,10 +610,11 @@ static u32 bit_func(struct i2c_adapter *adap)
/* -----exported algorithm data: ------------------------------------- */
-static const struct i2c_algorithm i2c_bit_algo = { +const struct i2c_algorithm i2c_bit_algo = { .master_xfer = bit_xfer, .functionality = bit_func, }; +EXPORT_SYMBOL(i2c_bit_algo);
/*
- registering functions to load algorithms at runtime
diff --git a/include/linux/i2c-algo-bit.h b/include/linux/i2c-algo-bit.h index 4f98148..584ffa0 100644 --- a/include/linux/i2c-algo-bit.h +++ b/include/linux/i2c-algo-bit.h @@ -49,5 +49,6 @@ struct i2c_algo_bit_data {
int i2c_bit_add_bus(struct i2c_adapter *); int i2c_bit_add_numbered_bus(struct i2c_adapter *); +extern const struct i2c_algorithm i2c_bit_algo;
#endif /* _LINUX_I2C_ALGO_BIT_H */
Acked-by: Jean Delvare khali@linux-fr.org
I've applied this patch to drm-intel-next, thanks for the feedback. -Daniel
On Mon, 27 Feb 2012 23:52:23 +0100, Daniel Vetter wrote:
On Mon, Feb 27, 2012 at 11:20:40PM +0100, Jean Delvare wrote:
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.
This seems perfectly sane.
(...) 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.
Note that thanks to the way things are implemented in i2c-algo-bit, you'd really only have to write a new wrapper around __i2c_bit_add_bus(), so this is really only 1 line of code. That being said, I am not insisting, it's really up to you.
When we set up the gpio fallback, we always have a 1:1 relationship with an intel_gmbus. Exploit that to store all gpio related data in there, too. This is a preparation step to merge the tw i2c adapters controlling the same bus into one.
Just mundane code-munging in this patch.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_i2c.c | 144 +++++++++++++++++++------------------- 2 files changed, 72 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ed40743..f137b1d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -294,6 +294,7 @@ struct intel_gmbus { struct i2c_adapter adapter; struct i2c_adapter *force_bit; u32 reg0; + u32 gpio_reg; struct drm_i915_private *dev_priv; };
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index f496510..b9f17cb 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -45,13 +45,6 @@ to_intel_gmbus(struct i2c_adapter *i2c) return container_of(i2c, struct intel_gmbus, adapter); }
-struct intel_gpio { - struct i2c_adapter adapter; - struct i2c_algo_bit_data algo; - struct drm_i915_private *dev_priv; - u32 reg; -}; - void intel_i2c_reset(struct drm_device *dev) { @@ -78,15 +71,15 @@ static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable) I915_WRITE(DSPCLK_GATE_D, val); }
-static u32 get_reserved(struct intel_gpio *gpio) +static u32 get_reserved(struct intel_gmbus *bus) { - struct drm_i915_private *dev_priv = gpio->dev_priv; + struct drm_i915_private *dev_priv = bus->dev_priv; struct drm_device *dev = dev_priv->dev; u32 reserved = 0;
/* On most chips, these bits must be preserved in software. */ if (!IS_I830(dev) && !IS_845G(dev)) - reserved = I915_READ_NOTRACE(gpio->reg) & + reserved = I915_READ_NOTRACE(bus->gpio_reg) & (GPIO_DATA_PULLUP_DISABLE | GPIO_CLOCK_PULLUP_DISABLE);
@@ -95,29 +88,29 @@ static u32 get_reserved(struct intel_gpio *gpio)
static int get_clock(void *data) { - struct intel_gpio *gpio = data; - struct drm_i915_private *dev_priv = gpio->dev_priv; - u32 reserved = get_reserved(gpio); - I915_WRITE_NOTRACE(gpio->reg, reserved | GPIO_CLOCK_DIR_MASK); - I915_WRITE_NOTRACE(gpio->reg, reserved); - return (I915_READ_NOTRACE(gpio->reg) & GPIO_CLOCK_VAL_IN) != 0; + struct intel_gmbus *bus = data; + struct drm_i915_private *dev_priv = bus->dev_priv; + u32 reserved = get_reserved(bus); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved | GPIO_CLOCK_DIR_MASK); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved); + return (I915_READ_NOTRACE(bus->gpio_reg) & GPIO_CLOCK_VAL_IN) != 0; }
static int get_data(void *data) { - struct intel_gpio *gpio = data; - struct drm_i915_private *dev_priv = gpio->dev_priv; - u32 reserved = get_reserved(gpio); - I915_WRITE_NOTRACE(gpio->reg, reserved | GPIO_DATA_DIR_MASK); - I915_WRITE_NOTRACE(gpio->reg, reserved); - return (I915_READ_NOTRACE(gpio->reg) & GPIO_DATA_VAL_IN) != 0; + struct intel_gmbus *bus = data; + struct drm_i915_private *dev_priv = bus->dev_priv; + u32 reserved = get_reserved(bus); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved | GPIO_DATA_DIR_MASK); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved); + return (I915_READ_NOTRACE(bus->gpio_reg) & GPIO_DATA_VAL_IN) != 0; }
static void set_clock(void *data, int state_high) { - struct intel_gpio *gpio = data; - struct drm_i915_private *dev_priv = gpio->dev_priv; - u32 reserved = get_reserved(gpio); + struct intel_gmbus *bus = data; + struct drm_i915_private *dev_priv = bus->dev_priv; + u32 reserved = get_reserved(bus); u32 clock_bits;
if (state_high) @@ -126,15 +119,15 @@ static void set_clock(void *data, int state_high) clock_bits = GPIO_CLOCK_DIR_OUT | GPIO_CLOCK_DIR_MASK | GPIO_CLOCK_VAL_MASK;
- I915_WRITE_NOTRACE(gpio->reg, reserved | clock_bits); - POSTING_READ(gpio->reg); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved | clock_bits); + POSTING_READ(bus->gpio_reg); }
static void set_data(void *data, int state_high) { - struct intel_gpio *gpio = data; - struct drm_i915_private *dev_priv = gpio->dev_priv; - u32 reserved = get_reserved(gpio); + struct intel_gmbus *bus = data; + struct drm_i915_private *dev_priv = bus->dev_priv; + u32 reserved = get_reserved(bus); u32 data_bits;
if (state_high) @@ -143,13 +136,14 @@ static void set_data(void *data, int state_high) data_bits = GPIO_DATA_DIR_OUT | GPIO_DATA_DIR_MASK | GPIO_DATA_VAL_MASK;
- I915_WRITE_NOTRACE(gpio->reg, reserved | data_bits); - POSTING_READ(gpio->reg); + I915_WRITE_NOTRACE(bus->gpio_reg, reserved | data_bits); + POSTING_READ(bus->gpio_reg); }
static struct i2c_adapter * -intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin) +intel_gpio_create(struct intel_gmbus *bus, u32 pin) { + struct drm_i915_private *dev_priv = bus->dev_priv; static const int map_pin_to_reg[] = { 0, GPIOB, @@ -160,65 +154,69 @@ intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin) 0, GPIOF, }; - struct intel_gpio *gpio; + struct i2c_adapter *adapter; + struct i2c_algo_bit_data *algo;
if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) return NULL;
- gpio = kzalloc(sizeof(struct intel_gpio), GFP_KERNEL); - if (gpio == NULL) + adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL); + if (adapter == NULL) return NULL;
- gpio->reg = map_pin_to_reg[pin]; + algo = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL); + if (algo == NULL) + goto out_adap; + + bus->gpio_reg = map_pin_to_reg[pin]; if (HAS_PCH_SPLIT(dev_priv->dev)) - gpio->reg += PCH_GPIOA - GPIOA; - gpio->dev_priv = dev_priv; + bus->gpio_reg += PCH_GPIOA - GPIOA;
- snprintf(gpio->adapter.name, sizeof(gpio->adapter.name), + snprintf(adapter->name, sizeof(adapter->name), "i915 GPIO%c", "?BACDE?F"[pin]); - gpio->adapter.owner = THIS_MODULE; - gpio->adapter.algo_data = &gpio->algo; - gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev; - gpio->algo.setsda = set_data; - gpio->algo.setscl = set_clock; - gpio->algo.getsda = get_data; - gpio->algo.getscl = get_clock; - gpio->algo.udelay = I2C_RISEFALL_TIME; - gpio->algo.timeout = usecs_to_jiffies(2200); - gpio->algo.data = gpio; - - if (i2c_bit_add_bus(&gpio->adapter)) - goto out_free; - - return &gpio->adapter; - -out_free: - kfree(gpio); + adapter->owner = THIS_MODULE; + adapter->algo_data = algo; + adapter->dev.parent = &dev_priv->dev->pdev->dev; + algo->setsda = set_data; + algo->setscl = set_clock; + algo->getsda = get_data; + algo->getscl = get_clock; + algo->udelay = I2C_RISEFALL_TIME; + algo->timeout = usecs_to_jiffies(2200); + algo->data = bus; + + if (i2c_bit_add_bus(adapter)) + goto out_algo; + + return adapter; + +out_algo: + kfree(algo); +out_adap: + kfree(adapter); return NULL; }
static int -intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv, +intel_i2c_quirk_xfer(struct intel_gmbus *bus, struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) { - struct intel_gpio *gpio = container_of(adapter, - struct intel_gpio, - adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; int ret;
intel_i2c_reset(dev_priv->dev);
intel_i2c_quirk_set(dev_priv, true); - set_data(gpio, 1); - set_clock(gpio, 1); + set_data(bus, 1); + set_clock(bus, 1); udelay(I2C_RISEFALL_TIME);
ret = adapter->algo->master_xfer(adapter, msgs, num);
- set_data(gpio, 1); - set_clock(gpio, 1); + set_data(bus, 1); + set_clock(bus, 1); intel_i2c_quirk_set(dev_priv, false);
return ret; @@ -238,8 +236,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) { - ret = intel_i2c_quirk_xfer(dev_priv, - bus->force_bit, msgs, num); + ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); goto out; }
@@ -334,11 +331,11 @@ timeout: I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ - bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff); + bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff); if (!bus->force_bit) ret = -ENOMEM; else - ret = intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num); + ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -412,7 +409,7 @@ int intel_setup_gmbus(struct drm_device *dev) bus->reg0 = i | GMBUS_RATE_100KHZ;
/* XXX force bit banging until GMBUS is fully debugged */ - bus->force_bit = intel_gpio_create(dev_priv, i); + bus->force_bit = intel_gpio_create(bus, i); }
intel_i2c_reset(dev_priv->dev); @@ -442,13 +439,13 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
if (force_bit) { if (bus->force_bit == NULL) { - struct drm_i915_private *dev_priv = bus->dev_priv; - bus->force_bit = intel_gpio_create(dev_priv, + bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff); } } else { if (bus->force_bit) { i2c_del_adapter(bus->force_bit); + kfree(bus->force_bit->algo); kfree(bus->force_bit); bus->force_bit = NULL; } @@ -467,6 +464,7 @@ void intel_teardown_gmbus(struct drm_device *dev) struct intel_gmbus *bus = &dev_priv->gmbus[i]; if (bus->force_bit) { i2c_del_adapter(bus->force_bit); + kfree(bus->force_bit->algo); kfree(bus->force_bit); } i2c_del_adapter(&bus->adapter);
... and directly call the newly exported i2c bit-banging functions.
The code is still pretty convoluted because we only set up the gpio i2c stuff when actually falling back, resulting in more complexity than necessary. This will be fixed up in the next patch.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_i2c.c | 38 +++++--------------------------------- 2 files changed, 7 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f137b1d..14b6e94 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -35,6 +35,7 @@ #include "intel_ringbuffer.h" #include <linux/io-mapping.h> #include <linux/i2c.h> +#include <linux/i2c-algo-bit.h> #include <drm/intel-gtt.h> #include <linux/backlight.h>
@@ -295,6 +296,7 @@ struct intel_gmbus { struct i2c_adapter *force_bit; u32 reg0; u32 gpio_reg; + struct i2c_algo_bit_data bit_algo; struct drm_i915_private *dev_priv; };
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index b9f17cb..6eb68b7 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -154,29 +154,18 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) 0, GPIOF, }; - struct i2c_adapter *adapter; struct i2c_algo_bit_data *algo;
if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) return NULL;
- adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL); - if (adapter == NULL) - return NULL; - - algo = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL); - if (algo == NULL) - goto out_adap; + algo = &bus->bit_algo;
bus->gpio_reg = map_pin_to_reg[pin]; if (HAS_PCH_SPLIT(dev_priv->dev)) bus->gpio_reg += PCH_GPIOA - GPIOA;
- snprintf(adapter->name, sizeof(adapter->name), - "i915 GPIO%c", "?BACDE?F"[pin]); - adapter->owner = THIS_MODULE; - adapter->algo_data = algo; - adapter->dev.parent = &dev_priv->dev->pdev->dev; + bus->adapter.algo_data = algo; algo->setsda = set_data; algo->setscl = set_clock; algo->getsda = get_data; @@ -185,16 +174,7 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) algo->timeout = usecs_to_jiffies(2200); algo->data = bus;
- if (i2c_bit_add_bus(adapter)) - goto out_algo; - - return adapter; - -out_algo: - kfree(algo); -out_adap: - kfree(adapter); - return NULL; + return &bus->adapter; }
static int @@ -213,7 +193,7 @@ intel_i2c_quirk_xfer(struct intel_gmbus *bus, set_clock(bus, 1); udelay(I2C_RISEFALL_TIME);
- ret = adapter->algo->master_xfer(adapter, msgs, num); + ret = i2c_bit_xfer(adapter, msgs, num);
set_data(bus, 1); set_clock(bus, 1); @@ -348,7 +328,7 @@ static u32 gmbus_func(struct i2c_adapter *adapter) adapter);
if (bus->force_bit) - bus->force_bit->algo->functionality(bus->force_bit); + i2c_bit_func(bus->force_bit);
return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ @@ -444,9 +424,6 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) } } else { if (bus->force_bit) { - i2c_del_adapter(bus->force_bit); - kfree(bus->force_bit->algo); - kfree(bus->force_bit); bus->force_bit = NULL; } } @@ -462,11 +439,6 @@ void intel_teardown_gmbus(struct drm_device *dev)
for (i = 0; i < GMBUS_NUM_PORTS; i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i]; - if (bus->force_bit) { - i2c_del_adapter(bus->force_bit); - kfree(bus->force_bit->algo); - kfree(bus->force_bit); - } i2c_del_adapter(&bus->adapter); }
This way we can simplify the setup and teardown a bit.
Because we don't actually allocate anything anymore for the force_bit case, we can now convert that into a boolean.
Also and the functionality supported by the bit-banging together with what gmbus can do, so that this doesn't randomly change any more.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_i2c.c | 50 ++++++++++++++----------------------- 2 files changed, 21 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 14b6e94..31affc0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -293,7 +293,8 @@ struct intel_fbc_work;
struct intel_gmbus { struct i2c_adapter adapter; - struct i2c_adapter *force_bit; + bool force_bit; + bool has_gpio; u32 reg0; u32 gpio_reg; struct i2c_algo_bit_data bit_algo; diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 6eb68b7..9791546 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -140,8 +140,8 @@ static void set_data(void *data, int state_high) POSTING_READ(bus->gpio_reg); }
-static struct i2c_adapter * -intel_gpio_create(struct intel_gmbus *bus, u32 pin) +static bool +intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { struct drm_i915_private *dev_priv = bus->dev_priv; static const int map_pin_to_reg[] = { @@ -157,7 +157,7 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) struct i2c_algo_bit_data *algo;
if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) - return NULL; + return false;
algo = &bus->bit_algo;
@@ -174,12 +174,11 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) algo->timeout = usecs_to_jiffies(2200); algo->data = bus;
- return &bus->adapter; + return true; }
static int intel_i2c_quirk_xfer(struct intel_gmbus *bus, - struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) { @@ -193,7 +192,7 @@ intel_i2c_quirk_xfer(struct intel_gmbus *bus, set_clock(bus, 1); udelay(I2C_RISEFALL_TIME);
- ret = i2c_bit_xfer(adapter, msgs, num); + ret = i2c_bit_xfer(&bus->adapter, msgs, num);
set_data(bus, 1); set_clock(bus, 1); @@ -216,7 +215,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) { - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + ret = intel_i2c_quirk_xfer(bus, msgs, num); goto out; }
@@ -311,11 +310,11 @@ timeout: I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ - bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff); - if (!bus->force_bit) - ret = -ENOMEM; - else - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + ret = -EIO; + if (bus->has_gpio) { + bus->force_bit = true; + ret = intel_i2c_quirk_xfer(bus, msgs, num); + } out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -323,14 +322,8 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) { - struct intel_gmbus *bus = container_of(adapter, - struct intel_gmbus, - adapter); - - if (bus->force_bit) - i2c_bit_func(bus->force_bit); - - return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | + return i2c_bit_func(adapter) && + (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL); @@ -388,8 +381,11 @@ int intel_setup_gmbus(struct drm_device *dev) /* By default use a conservative clock rate */ bus->reg0 = i | GMBUS_RATE_100KHZ;
+ bus->has_gpio = intel_gpio_setup(bus, i); + /* XXX force bit banging until GMBUS is fully debugged */ - bus->force_bit = intel_gpio_create(bus, i); + if (bus->has_gpio) + bus->force_bit = true; }
intel_i2c_reset(dev_priv->dev); @@ -417,16 +413,8 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) { struct intel_gmbus *bus = to_intel_gmbus(adapter);
- if (force_bit) { - if (bus->force_bit == NULL) { - bus->force_bit = intel_gpio_create(bus, - bus->reg0 & 0xff); - } - } else { - if (bus->force_bit) { - bus->force_bit = NULL; - } - } + if (bus->has_gpio) + bus->force_bit = force_bit; }
void intel_teardown_gmbus(struct drm_device *dev)
This way we can simplify the setup and teardown a bit.
Because we don't actually allocate anything anymore for the force_bit case, we can now convert that into a boolean.
Also and the functionality supported by the bit-banging together with what gmbus can do, so that this doesn't randomly change any more.
v2: Chris Wilson noticed that I've mixed up && and & ...
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_i2c.c | 50 ++++++++++++++----------------------- 2 files changed, 21 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 14b6e94..31affc0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -293,7 +293,8 @@ struct intel_fbc_work;
struct intel_gmbus { struct i2c_adapter adapter; - struct i2c_adapter *force_bit; + bool force_bit; + bool has_gpio; u32 reg0; u32 gpio_reg; struct i2c_algo_bit_data bit_algo; diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 6eb68b7..5e89680 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -140,8 +140,8 @@ static void set_data(void *data, int state_high) POSTING_READ(bus->gpio_reg); }
-static struct i2c_adapter * -intel_gpio_create(struct intel_gmbus *bus, u32 pin) +static bool +intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { struct drm_i915_private *dev_priv = bus->dev_priv; static const int map_pin_to_reg[] = { @@ -157,7 +157,7 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) struct i2c_algo_bit_data *algo;
if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) - return NULL; + return false;
algo = &bus->bit_algo;
@@ -174,12 +174,11 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) algo->timeout = usecs_to_jiffies(2200); algo->data = bus;
- return &bus->adapter; + return true; }
static int intel_i2c_quirk_xfer(struct intel_gmbus *bus, - struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) { @@ -193,7 +192,7 @@ intel_i2c_quirk_xfer(struct intel_gmbus *bus, set_clock(bus, 1); udelay(I2C_RISEFALL_TIME);
- ret = i2c_bit_xfer(adapter, msgs, num); + ret = i2c_bit_xfer(&bus->adapter, msgs, num);
set_data(bus, 1); set_clock(bus, 1); @@ -216,7 +215,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) { - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + ret = intel_i2c_quirk_xfer(bus, msgs, num); goto out; }
@@ -311,11 +310,11 @@ timeout: I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ - bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff); - if (!bus->force_bit) - ret = -ENOMEM; - else - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + ret = -EIO; + if (bus->has_gpio) { + bus->force_bit = true; + ret = intel_i2c_quirk_xfer(bus, msgs, num); + } out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -323,14 +322,8 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) { - struct intel_gmbus *bus = container_of(adapter, - struct intel_gmbus, - adapter); - - if (bus->force_bit) - i2c_bit_func(bus->force_bit); - - return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | + return i2c_bit_func(adapter) & + (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL); @@ -388,8 +381,11 @@ int intel_setup_gmbus(struct drm_device *dev) /* By default use a conservative clock rate */ bus->reg0 = i | GMBUS_RATE_100KHZ;
+ bus->has_gpio = intel_gpio_setup(bus, i); + /* XXX force bit banging until GMBUS is fully debugged */ - bus->force_bit = intel_gpio_create(bus, i); + if (bus->has_gpio) + bus->force_bit = true; }
intel_i2c_reset(dev_priv->dev); @@ -417,16 +413,8 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) { struct intel_gmbus *bus = to_intel_gmbus(adapter);
- if (force_bit) { - if (bus->force_bit == NULL) { - bus->force_bit = intel_gpio_create(bus, - bus->reg0 & 0xff); - } - } else { - if (bus->force_bit) { - bus->force_bit = NULL; - } - } + if (bus->has_gpio) + bus->force_bit = force_bit; }
void intel_teardown_gmbus(struct drm_device *dev)
On Tue, Feb 14, 2012 at 19:37, Daniel Vetter daniel.vetter@ffwll.ch wrote:
/* Hardware may not support GMBUS over these pins? Try GPIObitbanging instead. */
bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff);if (!bus->force_bit)ret = -ENOMEM;elseret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num);
ret = -EIO;if (bus->has_gpio) {bus->force_bit = true;ret = intel_i2c_quirk_xfer(bus, msgs, num);}
<bikeshedding> Wouldn't it be cleaner and more consistent with the rest of the code to use:
if (!bus->has_gpio) ret = -EIO; else { bus->force_bit = true; ret = intel_i2c_quirk_xfer(bus, msgs, num); }
instead? </bikeshedding>
Other than that, it looks correct to me, and certainly makes code more clean.
Reviewed-by: Eugeni Dodonov eugeni.dodonov@intel.com
This way we can simplify the setup and teardown a bit.
Because we don't actually allocate anything anymore for the force_bit case, we can now convert that into a boolean.
Also and the functionality supported by the bit-banging together with what gmbus can do, so that this doesn't randomly change any more.
v2: Chris Wilson noticed that I've mixed up && and & ...
v3: Clarify an if block as suggested by Eugeni Dodonov.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/i915_drv.h | 3 +- drivers/gpu/drm/i915/intel_i2c.c | 51 +++++++++++++++----------------------- 2 files changed, 22 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 14b6e94..31affc0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -293,7 +293,8 @@ struct intel_fbc_work;
struct intel_gmbus { struct i2c_adapter adapter; - struct i2c_adapter *force_bit; + bool force_bit; + bool has_gpio; u32 reg0; u32 gpio_reg; struct i2c_algo_bit_data bit_algo; diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 6eb68b7..44d1755 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -140,8 +140,8 @@ static void set_data(void *data, int state_high) POSTING_READ(bus->gpio_reg); }
-static struct i2c_adapter * -intel_gpio_create(struct intel_gmbus *bus, u32 pin) +static bool +intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { struct drm_i915_private *dev_priv = bus->dev_priv; static const int map_pin_to_reg[] = { @@ -157,7 +157,7 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) struct i2c_algo_bit_data *algo;
if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) - return NULL; + return false;
algo = &bus->bit_algo;
@@ -174,12 +174,11 @@ intel_gpio_create(struct intel_gmbus *bus, u32 pin) algo->timeout = usecs_to_jiffies(2200); algo->data = bus;
- return &bus->adapter; + return true; }
static int intel_i2c_quirk_xfer(struct intel_gmbus *bus, - struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) { @@ -193,7 +192,7 @@ intel_i2c_quirk_xfer(struct intel_gmbus *bus, set_clock(bus, 1); udelay(I2C_RISEFALL_TIME);
- ret = i2c_bit_xfer(adapter, msgs, num); + ret = i2c_bit_xfer(&bus->adapter, msgs, num);
set_data(bus, 1); set_clock(bus, 1); @@ -216,7 +215,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) { - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + ret = intel_i2c_quirk_xfer(bus, msgs, num); goto out; }
@@ -311,11 +310,12 @@ timeout: I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ - bus->force_bit = intel_gpio_create(bus, bus->reg0 & 0xff); - if (!bus->force_bit) - ret = -ENOMEM; - else - ret = intel_i2c_quirk_xfer(bus, bus->force_bit, msgs, num); + if (!bus->has_gpio) { + ret = -EIO; + } else { + bus->force_bit = true; + ret = intel_i2c_quirk_xfer(bus, msgs, num); + } out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -323,14 +323,8 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) { - struct intel_gmbus *bus = container_of(adapter, - struct intel_gmbus, - adapter); - - if (bus->force_bit) - i2c_bit_func(bus->force_bit); - - return (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | + return i2c_bit_func(adapter) & + (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL); @@ -388,8 +382,11 @@ int intel_setup_gmbus(struct drm_device *dev) /* By default use a conservative clock rate */ bus->reg0 = i | GMBUS_RATE_100KHZ;
+ bus->has_gpio = intel_gpio_setup(bus, i); + /* XXX force bit banging until GMBUS is fully debugged */ - bus->force_bit = intel_gpio_create(bus, i); + if (bus->has_gpio) + bus->force_bit = true; }
intel_i2c_reset(dev_priv->dev); @@ -417,16 +414,8 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) { struct intel_gmbus *bus = to_intel_gmbus(adapter);
- if (force_bit) { - if (bus->force_bit == NULL) { - bus->force_bit = intel_gpio_create(bus, - bus->reg0 & 0xff); - } - } else { - if (bus->force_bit) { - bus->force_bit = NULL; - } - } + if (bus->has_gpio) + bus->force_bit = force_bit; }
void intel_teardown_gmbus(struct drm_device *dev)
On Mon, Feb 27, 2012 at 15:22, Daniel Vetter daniel.vetter@ffwll.ch wrote:
This way we can simplify the setup and teardown a bit.
Because we don't actually allocate anything anymore for the force_bit case, we can now convert that into a boolean.
Also and the functionality supported by the bit-banging together with what gmbus can do, so that this doesn't randomly change any more.
v2: Chris Wilson noticed that I've mixed up && and & ...
v3: Clarify an if block as suggested by Eugeni Dodonov.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Eugeni Dodonov eugeni.dodonov@intel.com
With the rework to merge the bit-banging fallback into the gmbus i2c adapter we've gotten rid of the deadlock possibility that originally lead to the disabling of this code.
This reverts the revert
commit 826c7e4147f902737b281e8a5a7d7aa33fd63316 Author: Jean Delvare khali@linux-fr.org Date: Sat Jun 4 19:34:56 2011 +0000
Revert "drm/i915: Enable GMBUS for post-gen2 chipsets"
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=35572 Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/i915/intel_i2c.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 9791546..2ec5f0d 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -384,7 +384,7 @@ int intel_setup_gmbus(struct drm_device *dev) bus->has_gpio = intel_gpio_setup(bus, i);
/* XXX force bit banging until GMBUS is fully debugged */ - if (bus->has_gpio) + if (bus->has_gpio && IS_GEN2(dev)) bus->force_bit = true; }
On Tue, Feb 14, 2012 at 19:37, Daniel Vetter daniel.vetter@ffwll.ch wrote:
This way we can free up the bus->adaptor.algo_data pointer and make it available for use with the bitbanging fallback algo.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Eugeni Dodonov eugeni.dodonov@intel.com
On Mon, Feb 27, 2012 at 02:53:37PM -0300, Eugeni Dodonov wrote:
On Tue, Feb 14, 2012 at 19:37, Daniel Vetter daniel.vetter@ffwll.ch wrote:
This way we can free up the bus->adaptor.algo_data pointer and make it available for use with the bitbanging fallback algo.
Signed-Off-by: Daniel Vetter daniel.vetter@ffwll.ch
Reviewed-by: Eugeni Dodonov eugeni.dodonov@intel.com
I've slurped this series into -next, thanks for reviewing. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Eugeni Dodonov -
Jean Delvare