Fire corei2c driver bugs when using i2cdetect (random results)

BeagleV
1

Hi Beagle friends,
A couple issues I ran into with the driver. If you want to use i2cdetect, you will need these fixes.

This is for the Microchip CoreI2C drivers /dev/i2c-0 and /dev/i2c-1.

diff --git a/drivers/i2c/busses/i2c-microchip-corei2c.c b/drivers/i2c/busses/i2c-microchip-corei2c.c

@@ -463,8 +460,8 @@ static int mchp_corei2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned
 
        switch (size) {
        case I2C_SMBUS_QUICK:
-               msgs[CORE_I2C_SMBUS_MSG_WR].buf = NULL;
-               return 0;
+               msgs[CORE_I2C_SMBUS_MSG_WR].len = 0; /* Zero-length for quick */
+               break;
        case I2C_SMBUS_BYTE:
                if (read_write == I2C_SMBUS_WRITE)
                        msgs[CORE_I2C_SMBUS_MSG_WR].buf = &command;

The driver incorrectly assumes no bus transaction is needed. However, the data being aquired is the ack bit from sending a slave address on the bus. The above patch makes sure that actually happens.

The next part of the fix is in the same function:

@@ -505,7 +502,10 @@ static int mchp_corei2c_smbus_xfer(struct i2c_adapter *adap, u16 addr, unsigned
                return -EOPNOTSUPP;
        }
 
-       mchp_corei2c_xfer(&idev->adapter, msgs, num_msgs);
+       int ret = mchp_corei2c_xfer(&idev->adapter, msgs, num_msgs);
+        if (ret < 0)
+                return ret;  // Propagate errors like -6 (I2C NACK)
        if (read_write == I2C_SMBUS_WRITE || size <= I2C_SMBUS_BYTE_DATA)
                return 0;

The function return code was being thrown away! Not good. The next issue is, once you make these fixes, your git tree status becomes dirty. The “dirty” tag gets added on to the version string making it too long. So I made this mod:

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index 5818465ab..6cc54c2d0 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -141,12 +141,12 @@ scm_version()
        # git-diff-index does not refresh the index, so it may give misleading
        # results.
        # See git-update-index(1), git-diff-index(1), and git-status(1).
-       if {
-               git --no-optional-locks status -uno --porcelain 2>/dev/null ||
-               git diff-index --name-only HEAD
-       } | read dummy; then
-               printf '%s' -dirty
-       fi
+       # if {
+       #       git --no-optional-locks status -uno --porcelain 2>/dev/null ||
+       #       git diff-index --name-only HEAD
+       # } | read dummy; then
+       #       printf '%s' -dirty
+       # fi
 }

Just commented that part out. /shrug

Hope you find it useful.

Thanks,
J

2

I can’t quite make out what I’m looking at. It’s a driver of some kind sure,
but other than that, I’m not sure how helpful this is…

What kernel, or even better, what repository and what branch/tag are you referring to?

Microchip just found and fixed some stuff related to i2cdetect in 6.6.75;
are you duplicating their efforts?
If not, perhaps mailing a patch to their maintainer would be better?
(not sure they read this forum)

3

Sorry I guess I should not assumed that people familiar with the beagle would know how to find the i2c driver. Duplicating there effort? I pulled the latest driver the Microchip repo and they had not fixed it yet, so I fixed it myself. No good deed goes unpunished especially in open source land.

drivers/i2c/busses/i2c-microchip-corei2c.c

-J

4

I don’t know why you felt compelled to take my comment that hard…
Remember, I am commenting on what you write, not on you as a person.

In any case, your edited version is soo much better, but I still stand by
my position from earlier; it’s very unlikely that your work is ever picked up by MCHP here,
so I really think you should figure out who to mail your patch to.

That way we can all benefit from your efforts.
I can tell you from personal experience that they’re very nice people
and they do listen to and appreciate feedback from the community.

5

I mailed the information to the guy who wrote it. He didn’t respond.

-J

6 
voodoo@hestia:~/linux-src$ ./scripts/get_maintainer.pl drivers/i2c/busses/i2c-microchip-corei2c.c 
Conor Dooley <conor.dooley@microchip.com> (maintainer:RISC-V MICROCHIP FPGA SUPPORT)
Daire McNamara <daire.mcnamara@microchip.com> (maintainer:RISC-V MICROCHIP FPGA SUPPORT)
Andi Shyti <andi.shyti@kernel.org> (maintainer:I2C SUBSYSTEM HOST DRIVERS)
linux-riscv@lists.infradead.org (open list:RISC-V MICROCHIP FPGA SUPPORT)
linux-i2c@vger.kernel.org (open list:I2C SUBSYSTEM HOST DRIVERS)
linux-kernel@vger.kernel.org (open list)
RISC-V MICROCHIP FPGA SUPPORT status: Supported

Conor pretty much maintaines the whole fpga stack for microchip, please ping those 3 and include linux-i2c@vger.kernel.org

Regards,

2 Likes