Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383762 > unrolled thread

Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework Wolfram Sang <wsa@the-dreams.de> - 2016-04-20 23:40 +0200
    Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework Wolfram Sang <wsa@the-dreams.de> - 2016-04-21 16:00 +0200
      Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework Jan Glauber <jan.glauber@caviumnetworks.com> - 2016-04-21 20:00 +0200
        Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework Wolfram Sang <wsa@the-dreams.de> - 2016-04-21 23:40 +0200

#1383762 — Re: [PATCH v6 07/19] i2c: octeon: Use i2c recovery framework

[Multipart message — attachments visible in raw view] — view raw

On Mon, Apr 11, 2016 at 05:28:38PM +0200, Jan Glauber wrote:
> Switch to the i2c bus recovery framework using generic SCL recovery.
> If this fails try to reset the hardware. The recovery is triggered
> during START on timeout of the interrupt or failure to reach
> the START / repeated-START condition.
> 
> The START function is moved to xfer and while at it:
> - removed xfer debug message (i2c core already provides debugging)
> - removed length is zero check
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>
> ---
>  drivers/i2c/busses/i2c-octeon.c | 178 +++++++++++++++++++++++++---------------
>  1 file changed, 111 insertions(+), 67 deletions(-)

Interesting, it got larger...

> 
> +/**
> + * octeon_i2c_write_int - read the TWSI_INT register

read_int

> +	int ret, retries = 2;

I don't think 'retries' makes sense here. On failure, you return
-EAGAIN, so the core will retry 'adapter->retries' times anyhow.

> -	if (length < 1)
> -		return -EINVAL;

So, the adapter support 0-length messages now? Or why was it there? I
have the feeling this is a seperate patch.

> +static void octeon_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct octeon_i2c *i2c = i2c_get_adapdata(adap);
> +
> +	/*
> +	 * The stop resets the state machine, does not _transmit_ STOP unless
> +	 * engine was active.
> +	 */
> +	octeon_i2c_stop(i2c);
> +
> +	octeon_i2c_write_int(i2c, 0);

Maybe a comment why the delay?

> +	udelay(5);
> +}

[toc] | [next] | [standalone]

#1384256

[Multipart message — attachments visible in raw view] — view raw

> I assumed this check was bogus and there are no valid 0-length
> messages...

They are valid (check SMBUS_QUICK), but not every controller can handle
them correctly. Your driver has SMBUS_QUICK enabled, so this is a
contradiction to the check above where it rejects it.

So, it looks like it needs to be tested again (and documented this
time). If the HW can't do it, the FUNC bit for QUICK needs to be masked
out. If it can do SMBUS_QUICK, the check can probably go away.

[toc] | [prev] | [next] | [standalone]

#1384463

On Thu, Apr 21, 2016 at 03:54:50PM +0200, Wolfram Sang wrote:
> 
> > I assumed this check was bogus and there are no valid 0-length
> > messages...
> 
> They are valid (check SMBUS_QUICK), but not every controller can handle
> them correctly. Your driver has SMBUS_QUICK enabled, so this is a
> contradiction to the check above where it rejects it.

Oops, this mismatch dates back to the inital driver code. From the
documentation I would say SMBUS_QUICK is not supported, although nothing
terrible happens in the write case.

> So, it looks like it needs to be tested again (and documented this
> time). If the HW can't do it, the FUNC bit for QUICK needs to be masked
> out. If it can do SMBUS_QUICK, the check can probably go away.
> 

I would like to disable SMBUS_QUICK. It never worked for the read case.
Could we break something by disabling the quick-write case or is
the quick-write emulated by a larger write if the feature bit is not
set?

[toc] | [prev] | [next] | [standalone]

#1384588

[Multipart message — attachments visible in raw view] — view raw

> I would like to disable SMBUS_QUICK. It never worked for the read case.

Fine with me.

> Could we break something by disabling the quick-write case or is
> the quick-write emulated by a larger write if the feature bit is not
> set?

No emulation. It is simply not supported then. It is actually quite
dangerous to simulate it with regular write.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web