[PATCH 1/7] drm/i915: add dev_priv to intel_gmbus

[PATCH 1/7] drm/i915: add dev_priv to intel_gmbus

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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);
 		}
-- 
1.7.7.5

[PATCH 2/7] drm/nouveau: do a better job at hiding the NIH i2c bit-banging algo

[Daniel Vetter]

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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Signed-off-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
---
 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

Re: [PATCH 2/7] drm/nouveau: do a better job at hiding the NIH i2c bit-banging algo

[Daniel Vetter]

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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Signed-off-by: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
> ---
>  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
> 

-- 
Daniel Vetter
Mail: daniel-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48

[PATCH 3/7] i2c: export bit-banging algo functions

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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

Re: [PATCH 3/7] i2c: export bit-banging algo functions

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
> ---
>  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
> 

-- 
Daniel Vetter
Mail: daniel-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/7] i2c: export bit-banging algo functions

[Jean Delvare]

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-/w4YWyX8dFk@public.gmane.org>
> ---
>  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.

-- 
Jean Delvare

Re: [PATCH 3/7] i2c: export bit-banging algo functions

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
> > ---
> >  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-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48

[PATCH] i2c: export bit-banging algo functions

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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 */
-- 
1.7.7.5

Re: [PATCH] i2c: export bit-banging algo functions

[Jean Delvare]

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-/w4YWyX8dFk@public.gmane.org>
> ---
>  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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

Re: [PATCH] i2c: export bit-banging algo functions

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
> > ---
> >  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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

I've applied this patch to drm-intel-next, thanks for the feedback.
-Daniel
-- 
Daniel Vetter
Mail: daniel-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48

Re: [PATCH 3/7] i2c: export bit-banging algo functions

[Jean Delvare]

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.

-- 
Jean Delvare

[PATCH 4/7] drm/i915: merge struct intel_gpio into struct intel_gmbus

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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);
-- 
1.7.7.5

[PATCH 5/7] drm/i915: merge gmbus and gpio i2c adpater into one

[Daniel Vetter]

... 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-/w4YWyX8dFk@public.gmane.org>
---
 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);
 	}
 
-- 
1.7.7.5

[PATCH 6/7] drm/i915: i2c: unconditionally set up gpio fallback

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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)
-- 
1.7.7.5

[PATCH] drm/i915: i2c: unconditionally set up gpio fallback

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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)
-- 
1.7.7.5

Re: [PATCH 6/7] drm/i915: i2c: unconditionally set up gpio fallback

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 992 bytes --]

On Tue, Feb 14, 2012 at 19:37, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

>        /* 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);
> +       }
>

<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>

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1465 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/i915: i2c: unconditionally set up gpio fallback

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org>
---
 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)
-- 
1.7.7.5

Re: [PATCH] drm/i915: i2c: unconditionally set up gpio fallback

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 685 bytes --]

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>

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1185 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915: reenable gmbus on gen3+ again

[Daniel Vetter]

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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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-/w4YWyX8dFk@public.gmane.org>
---
 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;
 	}
 
-- 
1.7.7.5

Re: [Intel-gfx] [PATCH 1/7] drm/i915: add dev_priv to intel_gmbus

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 371 bytes --]

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>

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 787 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/7] drm/i915: add dev_priv to intel_gmbus

[Daniel Vetter]

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-/w4YWyX8dFk@public.gmane.org> 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-/w4YWyX8dFk@public.gmane.org>
> >
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

I've slurped this series into -next, thanks for reviewing.
-Daniel
-- 
Daniel Vetter
Mail: daniel-/w4YWyX8dFk@public.gmane.org
Mobile: +41 (0)79 365 57 48