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

Groups > linux.kernel > #1386530 > unrolled thread

[PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support

Started byJan Glauber <jglauber@cavium.com>
First post2016-04-25 16:40 +0200
Last post2016-04-26 23:10 +0200
Articles 8 — 3 participants

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

  [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Jan Glauber <jglauber@cavium.com> - 2016-04-25 16:40 +0200
    Re: [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Wolfram Sang <wsa@the-dreams.de> - 2016-04-26 00:20 +0200
      Re: [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Jan Glauber <jan.glauber@caviumnetworks.com> - 2016-04-26 08:50 +0200
        Re: [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Wolfram Sang <wsa@the-dreams.de> - 2016-04-26 09:40 +0200
          Re: [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Jan Glauber <jan.glauber@caviumnetworks.com> - 2016-04-26 14:40 +0200
            Re: [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support Wolfram Sang <wsa@the-dreams.de> - 2016-04-26 15:00 +0200
              [PATCH] i2c: octeon: Remove zero-length message support Jan Glauber <jglauber@cavium.com> - 2016-04-26 16:50 +0200
                Re: [PATCH] i2c: octeon: Remove zero-length message support Wolfram Sang <wsa@the-dreams.de> - 2016-04-26 23:10 +0200

#1386530 — [PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support

FromJan Glauber <jglauber@cavium.com>
Date2016-04-25 16:40 +0200
Subject[PATCH v7 03/15] i2c: octeon: Remove I2C_FUNC_SMBUS_QUICK support
Message-ID<rrPQ6-7se-23@gated-at.bofh.it>
SMBUS QUICK never worked for the read case, because EINVAL was returned
for a zero length message. The hardware does not support SMBUS QUICK
messages so disable the support and remove the zero length check.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 0f536a1..ad563cf 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -459,9 +459,6 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 	int i, result, length = *rlength;
 	bool final_read = false;
 
-	if (length < 1)
-		return -EINVAL;
-
 	octeon_i2c_data_write(i2c, (target << 1) | 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -597,7 +594,7 @@ static struct i2c_bus_recovery_info octeon_i2c_recovery_info = {
 
 static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
 	       I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_SMBUS_BLOCK_PROC_CALL;
 }
 
-- 
1.9.1

[toc] | [next] | [standalone]

#1386953

FromWolfram Sang <wsa@the-dreams.de>
Date2016-04-26 00:20 +0200
Message-ID<rrX1g-4VT-21@gated-at.bofh.it>
In reply to#1386530

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

On Mon, Apr 25, 2016 at 04:33:32PM +0200, Jan Glauber wrote:
> SMBUS QUICK never worked for the read case, because EINVAL was returned
> for a zero length message. The hardware does not support SMBUS QUICK
> messages so disable the support and remove the zero length check.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

I see better now and I think we need to drop this patch. It looks like
the driver supports only SMBUS_QUICK_WRITE which can be used for
scanning devices. The driver probably works with 'i2cdetect -q'. This
will regress if I apply the patch.

It seems it can't do SMBUS_QUICK_READ thus the extra check in the code.

I2C_FUNC_SMBUS_QUICK should have been split up in READ and WRITE to
support this scenario.

Are my assumptions correct?


> ---
>  drivers/i2c/busses/i2c-octeon.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
> index 0f536a1..ad563cf 100644
> --- a/drivers/i2c/busses/i2c-octeon.c
> +++ b/drivers/i2c/busses/i2c-octeon.c
> @@ -459,9 +459,6 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
>  	int i, result, length = *rlength;
>  	bool final_read = false;
>  
> -	if (length < 1)
> -		return -EINVAL;
> -
>  	octeon_i2c_data_write(i2c, (target << 1) | 1);
>  	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
>  
> @@ -597,7 +594,7 @@ static struct i2c_bus_recovery_info octeon_i2c_recovery_info = {
>  
>  static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
> +	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
>  	       I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_SMBUS_BLOCK_PROC_CALL;
>  }
>  
> -- 
> 1.9.1
>

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

#1387171

FromJan Glauber <jan.glauber@caviumnetworks.com>
Date2016-04-26 08:50 +0200
Message-ID<rs4YN-2UO-1@gated-at.bofh.it>
In reply to#1386953
On Tue, Apr 26, 2016 at 07:58:45AM +0200, Jan Glauber wrote:
> On Tue, Apr 26, 2016 at 12:16:22AM +0200, Wolfram Sang wrote:
> > On Mon, Apr 25, 2016 at 04:33:32PM +0200, Jan Glauber wrote:
> > > SMBUS QUICK never worked for the read case, because EINVAL was returned
> > > for a zero length message. The hardware does not support SMBUS QUICK
> > > messages so disable the support and remove the zero length check.
> > > 
> > > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > 
> > I see better now and I think we need to drop this patch. It looks like
> > the driver supports only SMBUS_QUICK_WRITE which can be used for
> > scanning devices. The driver probably works with 'i2cdetect -q'. This
> > will regress if I apply the patch.
> > 
> > It seems it can't do SMBUS_QUICK_READ thus the extra check in the code.
> > 
> > I2C_FUNC_SMBUS_QUICK should have been split up in READ and WRITE to
> > support this scenario.
> > 
> > Are my assumptions correct?
> 
> Yes, I thought briefly about splitting SMBUS_QUICK into read-write
> variants too. To me the question is if this feature is still used on modern
> devices or if this is more a relict of the past. I don't know enough
> about SMBUS to answer that.
> 
> Also, the ThunderX documentation does not mention that it is supported.
> The read case would be forbidden by the documentation (that was probably
> the reason for adding the read EINVAL). The write case is kind of a grey
> area, it might or might not work.
> 
> I'll see what i2cdetect does.

Checking on ThunderX:

[root@localhost linux-aarch64]# i2cdetect -q 0
WARNING! This program can confuse your I2C bus, cause data loss and worse!
I will probe file /dev/i2c-0 using quick write commands.
I will probe address range 0x03-0x77.
Continue? [Y/n] 
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 
10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f 
20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f 
30: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f 
40: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f 
50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f 
60: 60 61 62 63 64 65 66 67 UU 69 6a 6b 6c 6d 6e 6f 
70: 70 71 72 73 74 75 76 77 

Address 68 is the only device (rtc clock, module loaded).

Do all these other numbers make sense (although there are no
devices)?

--Jan

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

#1387190

FromWolfram Sang <wsa@the-dreams.de>
Date2016-04-26 09:40 +0200
Message-ID<rs5Ld-3z9-43@gated-at.bofh.it>
In reply to#1387171

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

> > Yes, I thought briefly about splitting SMBUS_QUICK into read-write
> > variants too. To me the question is if this feature is still used on modern
> > devices or if this is more a relict of the past. I don't know enough
> > about SMBUS to answer that.

Well, note that there are zero-length messages in I2C allowed as well.
Not only in SMBUS. I mainly use the term SMBUS_QUICK because it covers
both cases.


> Checking on ThunderX:
...
> Do all these other numbers make sense (although there are no
> devices)?

It makes sense in a way that it shows SMBUS_QUICK_WRITE is broken :) It
doesn't react to ACK/NACK properly. So, what needs to be done:

1) remove SMBUS_QUICK as you did in this patch 2) move the length check
so it doesn't only check read messages but also write messages. That is
to prevent handling custom setup I2C messages with a length of 0 (which
is legal). I'd suggest to return -EOPNOTSUPP in this case.

Thanks,

   Wolfram

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

#1387428

FromJan Glauber <jan.glauber@caviumnetworks.com>
Date2016-04-26 14:40 +0200
Message-ID<rsarx-7Tr-27@gated-at.bofh.it>
In reply to#1387190
On Tue, Apr 26, 2016 at 09:36:20AM +0200, Wolfram Sang wrote:
> 
> > > Yes, I thought briefly about splitting SMBUS_QUICK into read-write
> > > variants too. To me the question is if this feature is still used on modern
> > > devices or if this is more a relict of the past. I don't know enough
> > > about SMBUS to answer that.
> 
> Well, note that there are zero-length messages in I2C allowed as well.
> Not only in SMBUS. I mainly use the term SMBUS_QUICK because it covers
> both cases.
> 
> 
> > Checking on ThunderX:
> ...
> > Do all these other numbers make sense (although there are no
> > devices)?
> 
> It makes sense in a way that it shows SMBUS_QUICK_WRITE is broken :) It
> doesn't react to ACK/NACK properly. So, what needs to be done:
> 
> 1) remove SMBUS_QUICK as you did in this patch 2) move the length check
> so it doesn't only check read messages but also write messages. That is
> to prevent handling custom setup I2C messages with a length of 0 (which
> is legal). I'd suggest to return -EOPNOTSUPP in this case.

OK, I'll do that. Should I rebase the remaining patches or would you
like to review them first ? :)

Thanks,
Jan

> Thanks,
> 
>    Wolfram
>

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

#1387432

FromWolfram Sang <wsa@the-dreams.de>
Date2016-04-26 15:00 +0200
Message-ID<rsaKS-81H-1@gated-at.bofh.it>
In reply to#1387428

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

> OK, I'll do that. Should I rebase the remaining patches or would you
> like to review them first ? :)

Just resend this one, please. I probably pick up patches 8+9 for the
next cycle, but with the i2c-mux locking rework planned for 4.7, too, I
don't think I have the bandwidth to apply the rest for 4.7 as well.

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

#1387561 — [PATCH] i2c: octeon: Remove zero-length message support

FromJan Glauber <jglauber@cavium.com>
Date2016-04-26 16:50 +0200
Subject[PATCH] i2c: octeon: Remove zero-length message support
Message-ID<rsctl-Xx-33@gated-at.bofh.it>
In reply to#1387432
Zero-length message support (SMBUS QUICK or i2c) never worked with
the Octeon hardware. Disable SMBUS QUICK support and bail out in
case of a zero-length i2c request.

After this change 'i2c-detect -q' will return an error on Octeon but
the previously reported results were wrong anyway.

Signed-off-by: Jan Glauber <jglauber@cavium.com>
---
 drivers/i2c/busses/i2c-octeon.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-octeon.c b/drivers/i2c/busses/i2c-octeon.c
index 9f11e19..e8a65f5 100644
--- a/drivers/i2c/busses/i2c-octeon.c
+++ b/drivers/i2c/busses/i2c-octeon.c
@@ -837,9 +837,6 @@ static int octeon_i2c_read(struct octeon_i2c *i2c, int target,
 	int i, result, length = *rlength;
 	bool final_read = false;
 
-	if (length < 1)
-		return -EINVAL;
-
 	octeon_i2c_data_write(i2c, (target << 1) | 1);
 	octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB);
 
@@ -925,6 +922,12 @@ static int octeon_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	for (i = 0; ret == 0 && i < num; i++) {
 		struct i2c_msg *pmsg = &msgs[i];
 
+		/* zero-length messages are not supported */
+		if (!pmsg->len) {
+			ret = -EOPNOTSUPP;
+			break;
+		}
+
 		ret = octeon_i2c_start(i2c);
 		if (ret)
 			return ret;
@@ -998,7 +1001,7 @@ static struct i2c_bus_recovery_info octeon_i2c_recovery_info = {
 
 static u32 octeon_i2c_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) |
 	       I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_SMBUS_BLOCK_PROC_CALL;
 }
 
-- 
1.9.1

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

#1388100 — Re: [PATCH] i2c: octeon: Remove zero-length message support

FromWolfram Sang <wsa@the-dreams.de>
Date2016-04-26 23:10 +0200
SubjectRe: [PATCH] i2c: octeon: Remove zero-length message support
Message-ID<rsip4-6eQ-21@gated-at.bofh.it>
In reply to#1387561

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

On Tue, Apr 26, 2016 at 04:26:52PM +0200, Jan Glauber wrote:
> Zero-length message support (SMBUS QUICK or i2c) never worked with
> the Octeon hardware. Disable SMBUS QUICK support and bail out in
> case of a zero-length i2c request.
> 
> After this change 'i2c-detect -q' will return an error on Octeon but
> the previously reported results were wrong anyway.
> 
> Signed-off-by: Jan Glauber <jglauber@cavium.com>

Applied to for-next, thanks!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web