On Wed, Mar 10, 2010 at 4:44 AM, Jean Delvare <khali@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > 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@xxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxx> > 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@xxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html