Re: i2c and indirect vs. direct config

[Jason Thorpe <thorpej%me.com@localhost>]

> On May 30, 2018, at 4:42 PM, Brad Spencer <brad%anduin.eldar.org@localhost> wrote:
> 
> Jason Thorpe <thorpej%me.com@localhost> writes:
> 
>>> On May 30, 2018, at 11:54 AM, Brad Spencer <brad%anduin.eldar.org@localhost> wrote:
> 
> A zero length write would probably also work and should be just as safe,
> although I am not sure that every i2c controller supports that sort of
> thing.  The RPI had a KASSERT() that wasn't needed that would have
> paniced upon trying such a thing.  It was removed when the si70xx driver
> was added to the tree because that driver needed to be able to do that a
> zero length write, then wait before doing a read.  If clock stretching
> support existed in the i2c infrastructure, this would not have been
> required.

So, a “quick read” didn’t work… I had to actually read a byte back in order to get it to work correctly.

With just “quick read”:

[    1.000000] bsciic0 at fdt1: Broadcom Serial Controller
[    1.000000] iic0 at bsciic0: I2C bus
[    1.000000] dsrtc0 at iic0 addr 0x68: DS3231 Real-time Clock
[    1.000000] bsciic1 at fdt1: Broadcom Serial Controller
[    1.000000] iic1 at bsciic1: I2C bus
[    1.000000] dsrtc1 at iic1 addr 0x68: DS3231 Real-time Clock
[    1.000000] bsciic2 at fdt1: Broadcom Serial Controller
[    1.000000] iic2 at bsciic2: I2C bus
[    1.000000] dsrtc2 at iic2 addr 0x68: DS3231 Real-time Clock
[    1.000000] tsllux0 at iic2 addr 0x39: TSL256x Light-to-Digital converter

Note that the stupid clock still ghosts, but the light sensor does not, because the light sensor driver contains an additional “read byte” check (that’s actually a bit more dangerous potentially than a receive byte… but the intent is to read the Id register after verifying that’s it’s one of the legal addresses).  Both have wild carded parent locators.

With a “receive byte” instead:

[    1.000000] bsciic0 at fdt1: Broadcom Serial Controller
[    1.000000] iic0 at bsciic0: I2C bus
[    1.000000] bsciic1 at fdt1: Broadcom Serial Controller
[    1.000000] iic1 at bsciic1: I2C bus
[    1.000000] bsciic2 at fdt1: Broadcom Serial Controller
[    1.000000] iic2 at bsciic2: I2C bus
[    1.000000] dsrtc0 at iic2 addr 0x68: DS3231 Real-time Clock
[    1.000000] tsllux0 at iic2 addr 0x39: TSL256x Light-to-Digital converter

…i.e. the right thing happened.

NOW.  i2c_scan defaults to doing a “quick write”, HOWEVER, that code has the comment about the “quick write” method corrupting the EEPROM on Thinkpads.  But it also has a comment about the “receive byte” method possibly causing problems with write-only devices?

So, I’m a little perplexed about what to do… perhaps the “receive byte” method is the best for now?

? pca9685.c
? pca9685reg.h
Index: i2c.c
===================================================================
RCS file: /cvsroot/src/sys/dev/i2c/i2c.c,v
retrieving revision 1.58
diff -u -p -r1.58 i2c.c
--- i2c.c	15 May 2018 02:02:18 -0000	1.58
+++ i2c.c	31 May 2018 03:26:23 -0000
@@ -127,22 +127,51 @@ iic_print(void *aux, const char *pnp)
 	return UNCONF;
 }
 
+static bool
+iic_is_special_address(i2c_addr_t addr)
+{
+
+	/*
+	 * See: https://www.i2c-bus.org/addressing/
+	 */
+
+	/* General Call (read) / Start Byte (write) */
+	if (addr == 0x00)
+		return (true);
+
+	/* CBUS Addresses */
+	if (addr == 0x01)
+		return (true);
+
+	/* Reserved for Different Bus Formats */
+	if (addr == 0x02)
+		return (true);
+
+	/* Reserved for future purposes */
+	if (addr == 0x03)
+		return (true);
+
+	/* High Speed Master Code */
+	if ((addr & 0x7c) == 0x04)
+		return (true);
+
+	/* 10-bit Slave Addressing prefix */
+	if ((addr & 0x7c) == 0x78)
+		return (true);
+	
+	/* Reserved for future purposes */
+	if ((addr & 0x7c) == 0x7c)
+		return (true);
+	
+	return (false);
+}
+
 static int
 iic_search(device_t parent, cfdata_t cf, const int *ldesc, void *aux)
 {
 	struct iic_softc *sc = device_private(parent);
 	struct i2c_attach_args ia;
 
-	/*
-	 * I2C doesn't have any regular probing capability.  If we
-	 * encounter a cfdata with a wild-carded address or a wild-
-	 * carded parent spec, we skip them because they can only
-	 * be used for direct-coniguration.
-	 */
-	if (cf->cf_loc[IICCF_ADDR] == IICCF_ADDR_DEFAULT ||
-	    cf->cf_pspec->cfp_unit == DVUNIT_ANY)
-		return 0;
-
 	ia.ia_tag = sc->sc_tag;
 	ia.ia_size = cf->cf_loc[IICCF_SIZE];
 	ia.ia_type = sc->sc_type;
@@ -153,10 +182,43 @@ iic_search(device_t parent, cfdata_t cf,
 	ia.ia_prop = NULL;
 
 	for (ia.ia_addr = 0; ia.ia_addr <= I2C_MAX_ADDR; ia.ia_addr++) {
+		uint8_t dummy;
+		int error;
+
+		/*
+		 * Skip I2C addresses that are reserved for
+		 * special purposes.
+		 */
+		if (iic_is_special_address(ia.ia_addr))
+			continue;
+
 		if (sc->sc_devices[ia.ia_addr] != NULL)
 			continue;
 
-		if (cf->cf_loc[IICCF_ADDR] != ia.ia_addr)
+		if (cf->cf_loc[IICCF_ADDR] != IICCF_ADDR_DEFAULT &&
+		    cf->cf_loc[IICCF_ADDR] != ia.ia_addr)
+			continue;
+
+		/*
+		 * Check for the presence of a device by trying
+		 * to solicit an ACK in response to the address.
+		 * In a perfect world, a "quick read" would do the
+		 * trick, but it seems insufficient.  A "receive
+		 * byte" seems more reliable, but potentially has
+		 * problems with write-only devices?  A "quick
+		 * write" might be the perfect solution if not
+		 * for a known problem corrupting an EEPROM on
+		 * some Thinkpad models, so we'll compromise with
+		 * "receive byte" for now.
+		 */
+		if ((error = iic_acquire_bus(ia.ia_tag, I2C_F_POLL)) != 0) {
+			/* XXX log error?  bail out? */
+			continue;
+		}
+		error = iic_smbus_receive_byte(ia.ia_tag, ia.ia_addr, &dummy,
+					       I2C_F_POLL);
+		(void) iic_release_bus(ia.ia_tag, I2C_F_POLL);
+		if (error)
 			continue;
 
 		if (config_match(parent, cf, &ia) > 0)


-- thorpej