[PATCH][I2C] Marvell mv64xxx i2c driver

[Mark A. Greer]

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