| Message ID | 1461594824-7215-4-git-send-email-jglauber@cavium.com |
|---|---|
| State | Superseded |
| Headers | show |
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 >
On 04/25/2016 03:16 PM, 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? I think you may be reading too much into the intent of the code. I don't know that anyone has ever run "i2cdetect -q" on these buses, I know that I haven't. Since I2C is inherently unprobable, we specify the presence of all devices from the device tree passed by system firmware. Runtime scanning for devices is not necessary. That said, it is not a big deal one way or the other with respect to this patch. If someone verifies that "i2cdetect -q" does something sensible, then there is probably no harm in dropping this patch. > > >> --- >> 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 >> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thanks, Jan > > > --- > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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.
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; }
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(-)