Using algo-bit in another i2c algo

Using algo-bit in another i2c algo

[Alex Deucher]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

Hi,

    I'm adding support for the i2c controllers on radeon hardware and
I have a few questions.  I have a radeon-algo that encapsulates all
the various hw i2c controller functionality, however, it uses a
bit-algo bus internally for cases where you have to use bit-banging
rather than the hardware i2c engines.  Also, for bit banging to work
properly, you need to do some things before the bit-algo transaction
(basically masking the gpios for software use).  Right now we use
bit-algo i2c for the ddc buses, but they won't work externally to the
driver without the proper gpio masking prior to using them.  In the
radeon-algo patches, I use bit algo internally when I cannot use the
hardware i2c engines, or in cases where I haven't implemented support
yet for the hardware engine (as most gpios can be driven by sw or the
hw engine).  The problem is, this exposes the i2c bit-algo buses as
well as the radeon-algo buses.  Is there a way to not expose the
bit-algo buses that are used internally?  I've attached the patches
for reference.  Please cc: me as I'm not subscribed to this list.

Alex

[-- Attachment #2: 0001-drm-radeon-kms-add-radeon-i2c-algo.patch --]
[-- Type: application/mbox, Size: 13269 bytes --]

[-- Attachment #3: 0002-drm-radeon-kms-add-support-for-hw-i2c-on-r1xx-r5xx.patch --]
[-- Type: application/mbox, Size: 22287 bytes --]

Re: Using algo-bit in another i2c algo

[Jean Delvare]

Hi Alex,

Sorry for the late answer.

On Mon, 11 Jan 2010 17:13:50 -0500, Alex Deucher wrote:
> Hi,
> 
>     I'm adding support for the i2c controllers on radeon hardware and
> I have a few questions.  I have a radeon-algo that encapsulates all
> the various hw i2c controller functionality, however, it uses a
> bit-algo bus internally for cases where you have to use bit-banging
> rather than the hardware i2c engines.  Also, for bit banging to work
> properly, you need to do some things before the bit-algo transaction
> (basically masking the gpios for software use).

There used to be a patch floating around that added pre- and post-xfer
hooks to i2c-algo-bit... I couldn't find it again, so I rewrote it:

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Subject: i2c-algo-bit: Add pre- and post-xfer hooks

Drivers might have to do random things before and/or after I2C
transfers. Add hooks to the i2c-algo-bit implementation to let them do
so.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-bit.c |    9 +++++++++
 include/linux/i2c-algo-bit.h     |    2 ++
 2 files changed, 11 insertions(+)

--- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c	2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c	2010-03-09 18:52:50.000000000 +0100
@@ -522,6 +522,12 @@ static int bit_xfer(struct i2c_adapter *
 	int i, ret;
 	unsigned short nak_ok;
 
+	if (adap->pre_xfer) {
+		ret = adap->pre_xfer(i2c_adap, adap->data);
+		if (ret < 0)
+			return ret;
+	}
+
 	bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
 	i2c_start(adap);
 	for (i = 0; i < num; i++) {
@@ -570,6 +576,9 @@ static int bit_xfer(struct i2c_adapter *
 bailout:
 	bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
 	i2c_stop(adap);
+
+	if (adap->post_xfer)
+		adap->post_xfer(i2c_adap, adap->data);
 	return ret;
 }
 
--- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h	2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h	2010-03-09 18:53:06.000000000 +0100
@@ -32,6 +32,8 @@
  */
 struct i2c_algo_bit_data {
 	void *data;		/* private data for lowlevel routines */
+	int  (*pre_xfer)  (struct i2c_adapter *i2c_adap, void *data);
+	void (*post_xfer) (struct i2c_adapter *i2c_adap, void *data);
 	void (*setsda) (void *data, int state);
 	void (*setscl) (void *data, int state);
 	int  (*getsda) (void *data);

Would it help?

> Right now we use
> bit-algo i2c for the ddc buses, but they won't work externally to the
> driver without the proper gpio masking prior to using them.  In the
> radeon-algo patches, I use bit algo internally when I cannot use the
> hardware i2c engines, or in cases where I haven't implemented support
> yet for the hardware engine (as most gpios can be driven by sw or the
> hw engine).  The problem is, this exposes the i2c bit-algo buses as
> well as the radeon-algo buses.

This is bad, as it defeats the slave address uniqueness mechanism. Bad
things could happen. Would the patch above be sufficient to solve your
case?

> Is there a way to not expose the bit-algo buses that are used
> internally?

No, this is not possible at the time being. That being said, it
shouldn't be difficult, if the need exists. Looking at i2c-algo-bit,
the registration part is split into a preparation step and the actual
registration with i2c-core. The preparation step if done by
i2c_bit_prepare_bus(). If we exported this function, then you could
easily call it to create an internal i2c-algo-bit-based adapter, which
you wouldn't have to register if you don't want to:

From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Subject: i2c-algo-bit: Export i2c_bit_prepare_bus

This lets drivers make use of the i2c-algo-bit implementation without
actually registering the i2c adapter in question. This is useful to
drivers which need more than the i2c-algo-bit driver offers.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/algos/i2c-algo-bit.c |    3 ++-
 include/linux/i2c-algo-bit.h     |    1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

--- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-03-10 08:09:03.000000000 +0100
+++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c	2010-03-10 10:23:51.000000000 +0100
@@ -601,7 +601,7 @@ static const struct i2c_algorithm i2c_bi
 /*
  * registering functions to load algorithms at runtime
  */
-static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
+int i2c_bit_prepare_bus(struct i2c_adapter *adap)
 {
 	struct i2c_algo_bit_data *bit_adap = adap->algo_data;
 
@@ -617,6 +617,7 @@ static int i2c_bit_prepare_bus(struct i2
 
 	return 0;
 }
+EXPORT_SYMBOL(i2c_bit_prepare_bus);
 
 int i2c_bit_add_bus(struct i2c_adapter *adap)
 {
--- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h	2010-03-10 08:09:03.000000000 +0100
+++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h	2010-03-10 10:24:03.000000000 +0100
@@ -47,6 +47,7 @@ struct i2c_algo_bit_data {
 	int timeout;		/* in jiffies */
 };
 
+int i2c_bit_prepare_bus(struct i2c_adapter *);
 int i2c_bit_add_bus(struct i2c_adapter *);
 int i2c_bit_add_numbered_bus(struct i2c_adapter *);
 

With this possibility, I guess you wouldn't even need the first patch
any longer? If both are needed, that's fine with me, but if only one is
needed, that's even better.

> I've attached the patches for reference.

Please let me know if either or both of my 2 proposals above fit your
needs.

-- 
Jean Delvare

Re: Using algo-bit in another i2c algo

[Alex Deucher]

On Wed, Mar 10, 2010 at 4:44 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Alex,
>
> Sorry for the late answer.
>
> On Mon, 11 Jan 2010 17:13:50 -0500, Alex Deucher wrote:
>> Hi,
>>
>>     I'm adding support for the i2c controllers on radeon hardware and
>> I have a few questions.  I have a radeon-algo that encapsulates all
>> the various hw i2c controller functionality, however, it uses a
>> bit-algo bus internally for cases where you have to use bit-banging
>> rather than the hardware i2c engines.  Also, for bit banging to work
>> properly, you need to do some things before the bit-algo transaction
>> (basically masking the gpios for software use).
>
> There used to be a patch floating around that added pre- and post-xfer
> hooks to i2c-algo-bit... I couldn't find it again, so I rewrote it:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Add pre- and post-xfer hooks
>
> Drivers might have to do random things before and/or after I2C
> transfers. Add hooks to the i2c-algo-bit implementation to let them do
> so.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    9 +++++++++
>  include/linux/i2c-algo-bit.h     |    2 ++
>  2 files changed, 11 insertions(+)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-09 18:52:50.000000000 +0100
> @@ -522,6 +522,12 @@ static int bit_xfer(struct i2c_adapter *
>        int i, ret;
>        unsigned short nak_ok;
>
> +       if (adap->pre_xfer) {
> +               ret = adap->pre_xfer(i2c_adap, adap->data);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
>        bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
>        i2c_start(adap);
>        for (i = 0; i < num; i++) {
> @@ -570,6 +576,9 @@ static int bit_xfer(struct i2c_adapter *
>  bailout:
>        bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
>        i2c_stop(adap);
> +
> +       if (adap->post_xfer)
> +               adap->post_xfer(i2c_adap, adap->data);
>        return ret;
>  }
>
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-09 18:53:06.000000000 +0100
> @@ -32,6 +32,8 @@
>  */
>  struct i2c_algo_bit_data {
>        void *data;             /* private data for lowlevel routines */
> +       int  (*pre_xfer)  (struct i2c_adapter *i2c_adap, void *data);
> +       void (*post_xfer) (struct i2c_adapter *i2c_adap, void *data);
>        void (*setsda) (void *data, int state);
>        void (*setscl) (void *data, int state);
>        int  (*getsda) (void *data);
>
> Would it help?
>

I think that would do the trick.

>> Right now we use
>> bit-algo i2c for the ddc buses, but they won't work externally to the
>> driver without the proper gpio masking prior to using them.  In the
>> radeon-algo patches, I use bit algo internally when I cannot use the
>> hardware i2c engines, or in cases where I haven't implemented support
>> yet for the hardware engine (as most gpios can be driven by sw or the
>> hw engine).  The problem is, this exposes the i2c bit-algo buses as
>> well as the radeon-algo buses.
>
> This is bad, as it defeats the slave address uniqueness mechanism. Bad
> things could happen. Would the patch above be sufficient to solve your
> case?

yes.

>
>> Is there a way to not expose the bit-algo buses that are used
>> internally?
>
> No, this is not possible at the time being. That being said, it
> shouldn't be difficult, if the need exists. Looking at i2c-algo-bit,
> the registration part is split into a preparation step and the actual
> registration with i2c-core. The preparation step if done by
> i2c_bit_prepare_bus(). If we exported this function, then you could
> easily call it to create an internal i2c-algo-bit-based adapter, which
> you wouldn't have to register if you don't want to:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Export i2c_bit_prepare_bus
>
> This lets drivers make use of the i2c-algo-bit implementation without
> actually registering the i2c adapter in question. This is useful to
> drivers which need more than the i2c-algo-bit driver offers.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    3 ++-
>  include/linux/i2c-algo-bit.h     |    1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-10 10:23:51.000000000 +0100
> @@ -601,7 +601,7 @@ static const struct i2c_algorithm i2c_bi
>  /*
>  * registering functions to load algorithms at runtime
>  */
> -static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
> +int i2c_bit_prepare_bus(struct i2c_adapter *adap)
>  {
>        struct i2c_algo_bit_data *bit_adap = adap->algo_data;
>
> @@ -617,6 +617,7 @@ static int i2c_bit_prepare_bus(struct i2
>
>        return 0;
>  }
> +EXPORT_SYMBOL(i2c_bit_prepare_bus);
>
>  int i2c_bit_add_bus(struct i2c_adapter *adap)
>  {
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-10 10:24:03.000000000 +0100
> @@ -47,6 +47,7 @@ struct i2c_algo_bit_data {
>        int timeout;            /* in jiffies */
>  };
>
> +int i2c_bit_prepare_bus(struct i2c_adapter *);
>  int i2c_bit_add_bus(struct i2c_adapter *);
>  int i2c_bit_add_numbered_bus(struct i2c_adapter *);
>
>
> With this possibility, I guess you wouldn't even need the first patch
> any longer? If both are needed, that's fine with me, but if only one is
> needed, that's even better.
>
>> I've attached the patches for reference.
>
> Please let me know if either or both of my 2 proposals above fit your
> needs.

I think either of these patches should do the trick.  I'll test them
out this week.  I'm inclined to take the second one as it requires
less work in my driver, and I can fall back to big banging in my algo
if there is a problem with the hw i2c engine.  However there may be
devices out there that would prefer the first method.  I suppose we
can cross that bridge when the time comes.

Alex

Re: Using algo-bit in another i2c algo

[Alex Deucher]

On Wed, Mar 10, 2010 at 4:44 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Alex,
>
> Sorry for the late answer.
>
> On Mon, 11 Jan 2010 17:13:50 -0500, Alex Deucher wrote:
>> Hi,
>>
>>     I'm adding support for the i2c controllers on radeon hardware and
>> I have a few questions.  I have a radeon-algo that encapsulates all
>> the various hw i2c controller functionality, however, it uses a
>> bit-algo bus internally for cases where you have to use bit-banging
>> rather than the hardware i2c engines.  Also, for bit banging to work
>> properly, you need to do some things before the bit-algo transaction
>> (basically masking the gpios for software use).
>
> There used to be a patch floating around that added pre- and post-xfer
> hooks to i2c-algo-bit... I couldn't find it again, so I rewrote it:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Add pre- and post-xfer hooks
>
> Drivers might have to do random things before and/or after I2C
> transfers. Add hooks to the i2c-algo-bit implementation to let them do
> so.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    9 +++++++++
>  include/linux/i2c-algo-bit.h     |    2 ++
>  2 files changed, 11 insertions(+)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-09 18:52:50.000000000 +0100
> @@ -522,6 +522,12 @@ static int bit_xfer(struct i2c_adapter *
>        int i, ret;
>        unsigned short nak_ok;
>
> +       if (adap->pre_xfer) {
> +               ret = adap->pre_xfer(i2c_adap, adap->data);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
>        bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
>        i2c_start(adap);
>        for (i = 0; i < num; i++) {
> @@ -570,6 +576,9 @@ static int bit_xfer(struct i2c_adapter *
>  bailout:
>        bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
>        i2c_stop(adap);
> +
> +       if (adap->post_xfer)
> +               adap->post_xfer(i2c_adap, adap->data);
>        return ret;
>  }
>
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-09 18:53:06.000000000 +0100
> @@ -32,6 +32,8 @@
>  */
>  struct i2c_algo_bit_data {
>        void *data;             /* private data for lowlevel routines */
> +       int  (*pre_xfer)  (struct i2c_adapter *i2c_adap, void *data);
> +       void (*post_xfer) (struct i2c_adapter *i2c_adap, void *data);
>        void (*setsda) (void *data, int state);
>        void (*setscl) (void *data, int state);
>        int  (*getsda) (void *data);
>
> Would it help?
>
>> Right now we use
>> bit-algo i2c for the ddc buses, but they won't work externally to the
>> driver without the proper gpio masking prior to using them.  In the
>> radeon-algo patches, I use bit algo internally when I cannot use the
>> hardware i2c engines, or in cases where I haven't implemented support
>> yet for the hardware engine (as most gpios can be driven by sw or the
>> hw engine).  The problem is, this exposes the i2c bit-algo buses as
>> well as the radeon-algo buses.
>
> This is bad, as it defeats the slave address uniqueness mechanism. Bad
> things could happen. Would the patch above be sufficient to solve your
> case?
>
>> Is there a way to not expose the bit-algo buses that are used
>> internally?
>
> No, this is not possible at the time being. That being said, it
> shouldn't be difficult, if the need exists. Looking at i2c-algo-bit,
> the registration part is split into a preparation step and the actual
> registration with i2c-core. The preparation step if done by
> i2c_bit_prepare_bus(). If we exported this function, then you could
> easily call it to create an internal i2c-algo-bit-based adapter, which
> you wouldn't have to register if you don't want to:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Export i2c_bit_prepare_bus
>
> This lets drivers make use of the i2c-algo-bit implementation without
> actually registering the i2c adapter in question. This is useful to
> drivers which need more than the i2c-algo-bit driver offers.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    3 ++-
>  include/linux/i2c-algo-bit.h     |    1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-10 10:23:51.000000000 +0100
> @@ -601,7 +601,7 @@ static const struct i2c_algorithm i2c_bi
>  /*
>  * registering functions to load algorithms at runtime
>  */
> -static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
> +int i2c_bit_prepare_bus(struct i2c_adapter *adap)
>  {
>        struct i2c_algo_bit_data *bit_adap = adap->algo_data;
>
> @@ -617,6 +617,7 @@ static int i2c_bit_prepare_bus(struct i2
>
>        return 0;
>  }
> +EXPORT_SYMBOL(i2c_bit_prepare_bus);
>
>  int i2c_bit_add_bus(struct i2c_adapter *adap)
>  {
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-10 10:24:03.000000000 +0100
> @@ -47,6 +47,7 @@ struct i2c_algo_bit_data {
>        int timeout;            /* in jiffies */
>  };
>
> +int i2c_bit_prepare_bus(struct i2c_adapter *);
>  int i2c_bit_add_bus(struct i2c_adapter *);
>  int i2c_bit_add_numbered_bus(struct i2c_adapter *);
>
>
> With this possibility, I guess you wouldn't even need the first patch
> any longer? If both are needed, that's fine with me, but if only one is
> needed, that's even better.
>
>> I've attached the patches for reference.
>
> Please let me know if either or both of my 2 proposals above fit your
> needs.

I tested the second patch with a trivial patch to the radeon drm:

--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -892,9 +892,9 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
drm_device *dev,
         * make this, 2 jiffies is a lot more reliable */
        i2c->algo.radeon.bit_data.timeout = 2;
        i2c->algo.radeon.bit_data.data = i2c;
-       ret = i2c_bit_add_bus(&i2c->algo.radeon.bit_adapter);
+       ret = i2c_bit_prepare_bus(&i2c->algo.radeon.bit_adapter);
        if (ret) {
-               DRM_ERROR("Failed to register internal bit i2c %s\n", name);
+               DRM_ERROR("Failed to prepare internal bit i2c %s\n", name);
                goto out_free;
        }
        /* set the radeon i2c adapter */

However, just calling i2c_bit_prepare_bus does not appear to be
enough. as I get the following oops:

[ 1443.236030] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 1443.236125] IP: [<ffffffff8139913d>] __mutex_lock_slowpath+0xad/0x180
[ 1443.236192] PGD 677cd067 PUD 68d2c067 PMD 0
[ 1443.236306] Oops: 0002 [#1] SMP
[ 1443.236392] last sysfs file:
/sys/devices/platform/radeon_cp.0/firmware/radeon_cp.0/loading
[ 1443.236429] CPU 0
[ 1443.236486] Modules linked in: radeon(+) ttm drm_kms_helper drm
cfbcopyarea i2c_algo_bit i2c_core cfbimgblt cfbfillrect binfmt_misc
ipv6 powernow_k8 cpufreq_stats cpufreq_conservative cpufreq_userspace
cpufreq_ondemand freq_table cpufreq_powersave video output container
power_meter pci_slot fan sbs acpi_pad sbshc battery iptable_filter
ip_tables x_tables ac sbp2 fuse snd_hda_codec_atihdmi
snd_hda_codec_realtek snd_pcm_oss snd_hda_intel snd_hda_codec
snd_mixer_oss snd_pcm snd_seq_dummy snd_seq_oss usbhid snd_seq_midi
hid snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device
amd64_edac_mod snd ehci_hcd ohci_hcd ohci1394 pata_acpi pcspkr
edac_core soundcore snd_page_alloc evdev ata_generic usbcore sg r8169
mii ieee1394 thermal button processor [last unloaded: cfbfillrect]
[ 1443.238565] Pid: 16998, comm: work_for_cpu Not tainted 2.6.32 #252
System Product Name
[ 1443.238602] RIP: 0010:[<ffffffff8139913d>]  [<ffffffff8139913d>]
__mutex_lock_slowpath+0xad/0x180
[ 1443.238669] RSP: 0018:ffff880068e75b00  EFLAGS: 00010246
[ 1443.238702] RAX: ffff880068e75b10 RBX: ffff88006769ea24 RCX: 0000000000007e40
[ 1443.238736] RDX: 0000000000000000 RSI: ffff880068e75cd0 RDI: ffff88006769ea24
[ 1443.238770] RBP: ffff880068e75b60 R08: 0000000000000000 R09: 0000000000000400
[ 1443.238804] R10: 000000000000000a R11: 0000000000000000 R12: ffff88006769ea20
[ 1443.238837] R13: ffff88006769ea38 R14: ffff88006769ea28 R15: 0000000000000000
[ 1443.238871] FS:  00007f69514f36e0(0000) GS:ffff880001a00000(0000)
knlGS:0000000000000000
[ 1443.238908] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[ 1443.238941] CR2: 0000000000000000 CR3: 000000006610f000 CR4: 00000000000006f0
[ 1443.238974] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1443.239008] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 1443.239043] Process work_for_cpu (pid: 16998, threadinfo
ffff880068e74000, task ffff880065f65b40)
[ 1443.239079] Stack:
[ 1443.239109]  ffff880068e75fd8 ffff880065f65b40 ffff88006769ea28
ffffffff811fe7b8
[ 1443.239225] <0> 0000003000000020 ffff880068e75c00 ffff880068e75b40
ffff88006769ea20
[ 1443.239395] <0> 0000000000000002 00000000ffffffa1 ffff880068e75cd0
0000000000000002
[ 1443.239595] Call Trace:
[ 1443.239630]  [<ffffffff811fe7b8>] ? sprintf+0x68/0x70
[ 1443.239665]  [<ffffffff81399066>] mutex_lock+0x26/0x50
[ 1443.239701]  [<ffffffffa0182638>] i2c_transfer+0xe8/0xf0 [i2c_core]
[ 1443.239763]  [<ffffffffa04e94c5>] radeon_sw_i2c_xfer+0x45/0x70 [radeon]
[ 1443.239818]  [<ffffffffa04e957e>] radeon_i2c_xfer+0x8e/0x16e0 [radeon]
[ 1443.239855]  [<ffffffffa0182604>] i2c_transfer+0xb4/0xf0 [i2c_core]
[ 1443.239901]  [<ffffffffa048d148>] drm_do_probe_ddc_edid+0x48/0x60 [drm]
[ 1443.239943]  [<ffffffffa048e128>] drm_ddc_read_edid+0x38/0xb0 [drm]
[ 1443.239984]  [<ffffffffa048e23a>] drm_get_edid+0x9a/0x180 [drm]
[ 1443.240006]  [<ffffffffa04e6cd4>] radeon_modeset_init+0x494/0x7b0 [radeon]
[ 1443.240006]  [<ffffffffa051b9c0>] ? r600_init+0x220/0x390 [radeon]
[ 1443.240006]  [<ffffffffa04c625d>] radeon_driver_load_kms+0xdd/0x1b0 [radeon]
[ 1443.240006]  [<ffffffffa048388a>] drm_get_dev+0x35a/0x500 [drm]
[ 1443.240006]  [<ffffffff810748f0>] ? do_work_for_cpu+0x0/0x30
[ 1443.240006]  [<ffffffffa053778e>] radeon_pci_probe+0x10/0x263 [radeon]
[ 1443.240006]  [<ffffffff8120f4c2>] local_pci_probe+0x12/0x20
[ 1443.240006]  [<ffffffff81074903>] do_work_for_cpu+0x13/0x30
[ 1443.240006]  [<ffffffff810748f0>] ? do_work_for_cpu+0x0/0x30
[ 1443.240006]  [<ffffffff810787ce>] kthread+0x8e/0xa0
[ 1443.240006]  [<ffffffff81012faa>] child_rip+0xa/0x20
[ 1443.240006]  [<ffffffff81078740>] ? kthread+0x0/0xa0
[ 1443.240006]  [<ffffffff81012fa0>] ? child_rip+0x0/0x20
[ 1443.240006] Code: a8 83 78 20 63 7f b7 49 8d 5c 24 04 4d 8d 74 24
08 48 89 df e8 25 14 00 00 49 8b 54 24 10 48 8d 45 b0 4c 89 75 b0 49
89 44 24 10 <48> 89 02 48 8b 45 a8 48 89 55 b8 48 c7 c2 ff ff ff ff 48
89 45
[ 1443.241788] RIP  [<ffffffff8139913d>] __mutex_lock_slowpath+0xad/0x180
[ 1443.241788]  RSP <ffff880068e75b00>
[ 1443.241788] CR2: 0000000000000000
[ 1443.242594] ---[ end trace af8d6cef4b61feb3 ]---


Alex

Re: Using algo-bit in another i2c algo

[Alex Deucher]

On Wed, Mar 10, 2010 at 4:44 AM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Alex,
>
> Sorry for the late answer.
>
> On Mon, 11 Jan 2010 17:13:50 -0500, Alex Deucher wrote:
>> Hi,
>>
>>     I'm adding support for the i2c controllers on radeon hardware and
>> I have a few questions.  I have a radeon-algo that encapsulates all
>> the various hw i2c controller functionality, however, it uses a
>> bit-algo bus internally for cases where you have to use bit-banging
>> rather than the hardware i2c engines.  Also, for bit banging to work
>> properly, you need to do some things before the bit-algo transaction
>> (basically masking the gpios for software use).
>
> There used to be a patch floating around that added pre- and post-xfer
> hooks to i2c-algo-bit... I couldn't find it again, so I rewrote it:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Add pre- and post-xfer hooks
>
> Drivers might have to do random things before and/or after I2C
> transfers. Add hooks to the i2c-algo-bit implementation to let them do
> so.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    9 +++++++++
>  include/linux/i2c-algo-bit.h     |    2 ++
>  2 files changed, 11 insertions(+)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-09 18:52:50.000000000 +0100
> @@ -522,6 +522,12 @@ static int bit_xfer(struct i2c_adapter *
>        int i, ret;
>        unsigned short nak_ok;
>
> +       if (adap->pre_xfer) {
> +               ret = adap->pre_xfer(i2c_adap, adap->data);
> +               if (ret < 0)
> +                       return ret;
> +       }
> +
>        bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
>        i2c_start(adap);
>        for (i = 0; i < num; i++) {
> @@ -570,6 +576,9 @@ static int bit_xfer(struct i2c_adapter *
>  bailout:
>        bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
>        i2c_stop(adap);
> +
> +       if (adap->post_xfer)
> +               adap->post_xfer(i2c_adap, adap->data);
>        return ret;
>  }
>
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2009-06-10 05:05:27.000000000 +0200
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-09 18:53:06.000000000 +0100
> @@ -32,6 +32,8 @@
>  */
>  struct i2c_algo_bit_data {
>        void *data;             /* private data for lowlevel routines */
> +       int  (*pre_xfer)  (struct i2c_adapter *i2c_adap, void *data);
> +       void (*post_xfer) (struct i2c_adapter *i2c_adap, void *data);
>        void (*setsda) (void *data, int state);
>        void (*setscl) (void *data, int state);
>        int  (*getsda) (void *data);
>
> Would it help?

This patch seems works fine.  I've updated the radeon driver to
register a bit algo adapter if the i2c line is not hw capable.  I'll
send out updated radeon patches today or tomorrow once I've given it
further testing.

Alex

>
>> Right now we use
>> bit-algo i2c for the ddc buses, but they won't work externally to the
>> driver without the proper gpio masking prior to using them.  In the
>> radeon-algo patches, I use bit algo internally when I cannot use the
>> hardware i2c engines, or in cases where I haven't implemented support
>> yet for the hardware engine (as most gpios can be driven by sw or the
>> hw engine).  The problem is, this exposes the i2c bit-algo buses as
>> well as the radeon-algo buses.
>
> This is bad, as it defeats the slave address uniqueness mechanism. Bad
> things could happen. Would the patch above be sufficient to solve your
> case?
>
>> Is there a way to not expose the bit-algo buses that are used
>> internally?
>
> No, this is not possible at the time being. That being said, it
> shouldn't be difficult, if the need exists. Looking at i2c-algo-bit,
> the registration part is split into a preparation step and the actual
> registration with i2c-core. The preparation step if done by
> i2c_bit_prepare_bus(). If we exported this function, then you could
> easily call it to create an internal i2c-algo-bit-based adapter, which
> you wouldn't have to register if you don't want to:
>
> From: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Subject: i2c-algo-bit: Export i2c_bit_prepare_bus
>
> This lets drivers make use of the i2c-algo-bit implementation without
> actually registering the i2c adapter in question. This is useful to
> drivers which need more than the i2c-algo-bit driver offers.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/i2c/algos/i2c-algo-bit.c |    3 ++-
>  include/linux/i2c-algo-bit.h     |    1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> --- linux-2.6.34-rc1.orig/drivers/i2c/algos/i2c-algo-bit.c      2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/drivers/i2c/algos/i2c-algo-bit.c   2010-03-10 10:23:51.000000000 +0100
> @@ -601,7 +601,7 @@ static const struct i2c_algorithm i2c_bi
>  /*
>  * registering functions to load algorithms at runtime
>  */
> -static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
> +int i2c_bit_prepare_bus(struct i2c_adapter *adap)
>  {
>        struct i2c_algo_bit_data *bit_adap = adap->algo_data;
>
> @@ -617,6 +617,7 @@ static int i2c_bit_prepare_bus(struct i2
>
>        return 0;
>  }
> +EXPORT_SYMBOL(i2c_bit_prepare_bus);
>
>  int i2c_bit_add_bus(struct i2c_adapter *adap)
>  {
> --- linux-2.6.34-rc1.orig/include/linux/i2c-algo-bit.h  2010-03-10 08:09:03.000000000 +0100
> +++ linux-2.6.34-rc1/include/linux/i2c-algo-bit.h       2010-03-10 10:24:03.000000000 +0100
> @@ -47,6 +47,7 @@ struct i2c_algo_bit_data {
>        int timeout;            /* in jiffies */
>  };
>
> +int i2c_bit_prepare_bus(struct i2c_adapter *);
>  int i2c_bit_add_bus(struct i2c_adapter *);
>  int i2c_bit_add_numbered_bus(struct i2c_adapter *);
>
>
> With this possibility, I guess you wouldn't even need the first patch
> any longer? If both are needed, that's fine with me, but if only one is
> needed, that's even better.
>
>> I've attached the patches for reference.
>
> Please let me know if either or both of my 2 proposals above fit your
> needs.
>
> --
> Jean Delvare
>

Re: Using algo-bit in another i2c algo

[Jean Delvare]

On Wed, 10 Mar 2010 14:16:18 -0500, Alex Deucher wrote:
> I tested the second patch with a trivial patch to the radeon drm:
> 
> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
> @@ -892,9 +892,9 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
> drm_device *dev,
>          * make this, 2 jiffies is a lot more reliable */
>         i2c->algo.radeon.bit_data.timeout = 2;
>         i2c->algo.radeon.bit_data.data = i2c;
> -       ret = i2c_bit_add_bus(&i2c->algo.radeon.bit_adapter);
> +       ret = i2c_bit_prepare_bus(&i2c->algo.radeon.bit_adapter);
>         if (ret) {
> -               DRM_ERROR("Failed to register internal bit i2c %s\n", name);
> +               DRM_ERROR("Failed to prepare internal bit i2c %s\n", name);
>                 goto out_free;
>         }
>         /* set the radeon i2c adapter */
> 
> However, just calling i2c_bit_prepare_bus does not appear to be
> enough. as I get the following oops:
> (...)

Hmm, sorry, I have probably not been clear about the intent of
i2c_bit_prepare_bus(). It does _not_ give you a usable i2c_adapter. The
actual initialization happens in i2c_register_adapter(), in i2c-core.
What it gives you is a usable i2c _algorithm_. That is, you can call
bit_adapter.algo->master_xfer(), presumably from within your actual
(registered) adapter's master_xfer() function.

Now, I have to admit that it is pretty confusing that you call a
"prepare" function on an i2c_adapter and you can't use it. And looking
a bit more into it... Even calling bit_adapter.algo->master_xfer()
assumes an initialized i2c_adapter, at least for error messages. So my
proposal was probably not such a good idea, at least not with the
current i2c-algo-bit implementation, and probably not without a big
rework of the i2c algorithm structure itself: at the moment, i2c
algorithms really assume that they have an i2c_adapter associated _and_
that this i2c_adapter has a device associated.

Maybe we could move the core of i2c-algo-bit to a library so that it is
easier to reuse. But that would be a lot more work, so we will only do
this there is a clear benefit. At this point, it seems cheaper to
pursue the first option (pre- and post-xfer hooks) if that works for
you.

-- 
Jean Delvare

Re: Using algo-bit in another i2c algo

[Alex Deucher]

On Wed, Mar 10, 2010 at 4:49 PM, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, 10 Mar 2010 14:16:18 -0500, Alex Deucher wrote:
>> I tested the second patch with a trivial patch to the radeon drm:
>>
>> --- a/drivers/gpu/drm/radeon/radeon_i2c.c
>> +++ b/drivers/gpu/drm/radeon/radeon_i2c.c
>> @@ -892,9 +892,9 @@ struct radeon_i2c_chan *radeon_i2c_create(struct
>> drm_device *dev,
>>          * make this, 2 jiffies is a lot more reliable */
>>         i2c->algo.radeon.bit_data.timeout = 2;
>>         i2c->algo.radeon.bit_data.data = i2c;
>> -       ret = i2c_bit_add_bus(&i2c->algo.radeon.bit_adapter);
>> +       ret = i2c_bit_prepare_bus(&i2c->algo.radeon.bit_adapter);
>>         if (ret) {
>> -               DRM_ERROR("Failed to register internal bit i2c %s\n", name);
>> +               DRM_ERROR("Failed to prepare internal bit i2c %s\n", name);
>>                 goto out_free;
>>         }
>>         /* set the radeon i2c adapter */
>>
>> However, just calling i2c_bit_prepare_bus does not appear to be
>> enough. as I get the following oops:
>> (...)
>
> Hmm, sorry, I have probably not been clear about the intent of
> i2c_bit_prepare_bus(). It does _not_ give you a usable i2c_adapter. The
> actual initialization happens in i2c_register_adapter(), in i2c-core.
> What it gives you is a usable i2c _algorithm_. That is, you can call
> bit_adapter.algo->master_xfer(), presumably from within your actual
> (registered) adapter's master_xfer() function.
>
> Now, I have to admit that it is pretty confusing that you call a
> "prepare" function on an i2c_adapter and you can't use it. And looking
> a bit more into it... Even calling bit_adapter.algo->master_xfer()
> assumes an initialized i2c_adapter, at least for error messages. So my
> proposal was probably not such a good idea, at least not with the
> current i2c-algo-bit implementation, and probably not without a big
> rework of the i2c algorithm structure itself: at the moment, i2c
> algorithms really assume that they have an i2c_adapter associated _and_
> that this i2c_adapter has a device associated.
>
> Maybe we could move the core of i2c-algo-bit to a library so that it is
> easier to reuse. But that would be a lot more work, so we will only do
> this there is a clear benefit. At this point, it seems cheaper to
> pursue the first option (pre- and post-xfer hooks) if that works for
> you.

Yes, the first option is working well so far.

Alex