[PATCH] i2c-imx: fix error handling
- Return -ETIMEDOUT on bus busy error
- Fix timeout test "time_after(jiffies, orig_jiffies + HZ / 1000)" :
By default, HZ=100 on arm. This means that this test has no chances to
work and may result in a dead loop. Set timeout to 500ms.
- Don't try to send a new message if we failed to transmit
previous one. This was preventing to recover from error on my system
Signed-off-by: Arnaud Patard
---
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 32375bd..ced8a38 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -145,10 +145,10 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
"<%s> I2C Interrupted\n", __func__);
return -EINTR;
}
- if (time_after(jiffies, orig_jiffies + HZ / 1000)) {
+ if (time_after(jiffies, orig_jiffies + msecs_to_jiffies(500))) {
dev_dbg(&i2c_imx->adapter.dev,
"<%s> I2C bus is busy\n", __func__);
- return -EIO;
+ return -ETIMEDOUT;
}
schedule();
}
@@ -443,6 +443,8 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
result = i2c_imx_read(i2c_imx, &msgs[i]);
else
result = i2c_imx_write(i2c_imx, &msgs[i]);
+ if (result)
+ goto fail0;
}
fail0:
Re: [PATCH]I2C device - release cleanup
On Tue, 23 Mar 2010 13:02:41 +, jhautb...@gmail.com wrote: > Hi Jean, > > < snip > > > Did you test your patch? I am very skeptical that calling > > > single_release() out of the blue is the right thing to do. My instinct > > > tells me that single_release() is only meant for callers of > > > single_open(). > > Well, using this call works fine with my hardware. > I would say, as before :). > Looking at the source code of single_release, this is very similar to what > is done today. > > But yes, it would also be interesting to use single_open in the open() > syscall. > I think this would be nicer to use only single_*() functions. > > Maybe is it interesting to submit a patch that does a cleanup for all the > i2c-dev file ? And not only the release function ? Yes, that would be better. If you do that, please make sure to run your patch through scripts/checkpatch.pl before sending it. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: i2c probing
Hi Matthieu, On Sun, 21 Mar 2010 15:22:00 +0100, matthieu castet wrote: > Do you know why there is 2 methods of probing i2c [1] and [2], with different > quirks for eeprom. Historical reasons, mainly. i2c_detect() is an old thing, it is used by device drivers which want to automatically probe for devices they support (and provide a reliable detection function to this purpose.) Mainly hardware monitoring drivers do this. i2c_new_probed_device() is more recent, it is meant for bus drivers which want to probe for device presence at a selected address. There is no detection function involved in this case. i2c_detect() is using the device presence detection method that has been used since 1999 or so. For i2c_new_probed_device(), we learned from our past mistakes and opted for a safer approach. But in practice, both should work equally fine. > Why can't they be merged together ? I don't really remember, and in all honesty, I did not realize the legacy probing method was still present until I read your post. I guess that I didn't dare changing the legacy probing code originally. Now that we have good results with the new one, I agree that it would be nice to merge both (that is, get rid of the old one and use the new one everywhere.) Would you be interested in working on this? You can register yourself on the wiki [1] if you want, and add an action item. Or just send a patch right away if you think it is simple enough. Note that I have been working lately on letting callers of i2c_new_probed_device() pass a custom probe function. These changes will certainly collide with the ones you are suggesting. So if you want to work on this, please let me know so that we can coordinate our efforts. [1] https://i2c.wiki.kernel.org/index.php/Main_Page Thanks, -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]I2C device - release cleanup
Hi Jean-Michel,
On Tue, 23 Mar 2010 11:01:28 +0100, Jean-Michel Hautbois wrote:
> Hi there,
>
> Here is a little patch which aims to cleanup the release function in
> i2c-dev.c.
> This is only a call to single_release, instead of kfree and several things.
>
> Signed-off-by: Jean-Michel Hautbois
> ---
> drivers/i2c/i2c-dev.c
>
> --- linux-2.6.34-rc2/drivers/i2c/i2c-dev.c.orig 2010-03-23
> 10:19:26.0 +0100
> +++ linux-2.6.34-rc2/drivers/i2c/i2c-dev.c2010-03-23 10:55:54.0
> +0100
> @@ -36,6 +36,7 @@
> #include
> #include
> #include
> +#include
>
> static struct i2c_driver i2cdev_driver;
>
> @@ -477,12 +478,10 @@ static int i2cdev_open(struct inode *ino
> static int i2cdev_release(struct inode *inode, struct file *file)
> {
> struct i2c_client *client = file->private_data;
> -
> +
This is adding trailing white-space. Obviously you did not review your
own patch before sending it. And you did not run it through
scripts/checkpatch.pl either.
> i2c_put_adapter(client->adapter);
> - kfree(client);
> - file->private_data = NULL;
> -
> - return 0;
> +
> + return single_release(inode, file);
> }
>
> static const struct file_operations i2cdev_fops = {
Did you test your patch? I am very skeptical that calling
single_release() out of the blue is the right thing to do. My instinct
tells me that single_release() is only meant for callers of
single_open().
--
Jean Delvare
http://khali.linux-fr.org/wishlist.html
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: omap: fix OOPS in omap_i2c_unidle() during probe
Commit d84d3ea317ce0db89ce0903b4037f800c5d4c477 added register shift to allow also 16-bit register access. However, omap_i2c_unidle() is called before these are set which causes the following OOPS: Unhandled fault: alignment exception (0x801) at 0xfa070009 Internal error: : 801 [#1] last sysfs file: Modules linked in: CPU: 0Not tainted (2.6.34-rc2-00052-gae6be51 #3) PC is at omap_i2c_unidle+0x44/0x138 LR is at trace_hardirqs_on_caller+0x158/0x18c pc : []lr : []psr: 2013 sp : cfc2bf10 ip : 0009 fp : r10: r9 : r8 : c0378560 r7 : c0378b88 r6 : c0378558 r5 : cfcadc00 r4 : cfcadc00 r3 : 0009 r2 : fa07 r1 : r0 : Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 10c5387f Table: 80004019 DAC: 0017 Process swapper (pid: 1, stack limit = 0xcfc2a2e8) Stack: (0xcfc2bf10 to 0xcfc2c000) bf00: c0372cf8 c027225c c0a69678 bf20: cfc3e508 c0500898 c0378560 c0378560 c0500898 cfcac8c0 c04fc280 c017d4f4 bf40: c0378560 c017c63c c0378560 c0378594 c0500898 cfcac8c0 c04fc280 c017c754 bf60: c017c6f4 c0500898 c017beac cfc16a5c cfc3fd94 c0023448 c0500898 bf80: c0500898 c017b7d4 c032dc7f 0093 cfc28d40 c0023448 c0500898 bfa0: c017ca48 c0023448 c001d274 bfc0: c002b344 0031 0192 c0023448 bfe0: c0008578 c002c304 ffdf [] (omap_i2c_unidle+0x44/0x138) from [] (omap_i2c_probe+0x1a4/0x398) [] (omap_i2c_probe+0x1a4/0x398) from [] (platform_drv_probe+0x18/0x1c) [] (platform_drv_probe+0x18/0x1c) from [] (driver_probe_device+0xc0/0x178) [] (driver_probe_device+0xc0/0x178) from [] (__driver_attach+0x60/0x84) [] (__driver_attach+0x60/0x84) from [] (bus_for_each_dev+0x44/0x74) [] (bus_for_each_dev+0x44/0x74) from [] (bus_add_driver+0x9c/0x218) [] (bus_add_driver+0x9c/0x218) from [] (driver_register+0xa8/0x130) [] (driver_register+0xa8/0x130) from [] (do_one_initcall+0x5c/0x1b8) [] (do_one_initcall+0x5c/0x1b8) from [] (kernel_init+0x90/0x144) [] (kernel_init+0x90/0x144) from [] (kernel_thread_exit+0x0/0x8) Code: e5942004 e3a0c009 e1a0331c e3a01000 (e18210b3) ---[ end trace 1b75b31a2719ed1c ]--- This patch moves register shift setting before any register accesses are done. Signed-off-by: Mika Westerberg Cc: Cory Maccarrone --- drivers/i2c/busses/i2c-omap.c | 10 +- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index c7c2375..0d5a54a 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -902,6 +902,11 @@ omap_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, dev); + if (cpu_is_omap7xx()) + dev->reg_shift = 1; + else + dev->reg_shift = 2; + if ((r = omap_i2c_get_clocks(dev)) != 0) goto err_iounmap; @@ -925,11 +930,6 @@ omap_i2c_probe(struct platform_device *pdev) dev->b_hw = 1; /* Enable hardware fixes */ } - if (cpu_is_omap7xx()) - dev->reg_shift = 1; - else - dev->reg_shift = 2; - /* reset ASAP, clearing any IRQs */ omap_i2c_init(dev); -- 1.5.6.5 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]I2C device - release cleanup
Hi there,
Here is a little patch which aims to cleanup the release function in i2c-dev.c.
This is only a call to single_release, instead of kfree and several things.
Signed-off-by: Jean-Michel Hautbois
---
drivers/i2c/i2c-dev.c
--- linux-2.6.34-rc2/drivers/i2c/i2c-dev.c.orig 2010-03-23
10:19:26.0 +0100
+++ linux-2.6.34-rc2/drivers/i2c/i2c-dev.c 2010-03-23 10:55:54.0
+0100
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
static struct i2c_driver i2cdev_driver;
@@ -477,12 +478,10 @@ static int i2cdev_open(struct inode *ino
static int i2cdev_release(struct inode *inode, struct file *file)
{
struct i2c_client *client = file->private_data;
-
+
i2c_put_adapter(client->adapter);
- kfree(client);
- file->private_data = NULL;
-
- return 0;
+
+ return single_release(inode, file);
}
static const struct file_operations i2cdev_fops = {
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
