Hi, Alexey.
--
Alexey Dobriyan wrote:
>On Wednesday 02 February 2005 19:26, Mark A. Greer wrote:
>
>
>
>>How's this (a complete replacement for previous patch)?
>>
>>
>
>
>
>>--- a/drivers/i2c/busses/Kconfig
>>+++ b/drivers/i2c/busses/Kconfig
>>
>>
>
>
>
>>+ If you say yes to this option, support will be included for the
>>+ built-in I2C interface on the Marvell 64xxx line of host bridges
>>
>>
>
>Dot at the end. Should have noticed it earlier...
>
Gahh, I noticed that too a while back. I thought I'd fixed it... Done.
>
>
>>+ if (drv_data->state == MV64XXX_I2C_STATE_IDLE) {
>>+ drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
>>+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
>>
>>
>
>drv_data->state is already MV64XXX_I2C_STATE_IDLE. Gcc will probably optimize
>this line away, but...
>
Done away with.
>
>
>>+ }
>>+ else {
>>
>>
>
>It should be "} else {" according to CodingStyle.
>
Done.
>
>
>>+static int
>>+mv64xxx_i2c_intr(int irq, void *dev_id, struct pt_regs *regs)
>>
>>
>
>
>
>>+ while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
>>+ MV64XXX_I2C_REG_CONTROL_IFLG) {
>>+ status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
>>+ mv64xxx_i2c_fsm(drv_data, status);
>>
>>
>
>It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to
>MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel()
>something. Is it correct to _not_ check ->rc here?
>
I think so. It still needs to go into do_action even when rc != 0 (in
which case it'll do a STOP condition).
> If it isn't then would it be
>better to make all functions that set it return -E___ instead and drop
>struct mv64xxx_i2c_data.rc altogether?
>
I may not be understanding what you mean but I think I need something
like mv64xxx_i2c_data.rc or a plain old global to hang onto the return
code. I need that so when the wait_event_interruptible_timeout()
returns, that thread can find out what happened while it was blocked.
This is what I mean:
- calling thread enters i2c_xfer
- eventually the initial i2c action is executed and the thread blocks in
wait_event_interruptible_timeout()
- other processes run
- several interrupts happen, last one causing an error which is stored
in mv64xxx_i2c_data.rc
- do_action issues a stop on the i2c bus
- thread is unblocked and now has to dig out the rc from mv64xxx_i2c_data.rc
Or am I not understanding what you mean?
>+
>+ if (!time_left <= 0) {
>
>
>
>Confusing. You meant "if (time_left)" or "if (time_left > 0)"?
>
No, I'm blind. I meant "if (time_left <=0)". Should be fixed now.
Good catch.
>
>
>>+static struct i2c_algorithm mv64xxx_i2c_algo = {
>>+ .name = MV64XXX_I2C_CTLR_NAME "algorithm",
>>
>>
>
>MV64XXX_I2C_CTLR_NAME doesn't end with space. " algorithm" here.
>
Space added.
>
>
>>+ dev_err(dev, "mv64xxx: Can't register intr handler "
>>+ "irq: %d\\n", drv_data->irq);
>>
>>
>
>You snipped s# \\n # \n # suggestion in my previous email. ;-)
>
Ah, got it this time. :)
This patch is a replacement patch that should address your concerns
except maybe the mv64xxx_i2c_data.rc one.
Signed-off-by: Mark A. Greer <mgreer at mvista.com>
--
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: i2c_6.patch
Url: http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050203/27b92af8/attachment.pl
