> On May 30, 2018, at 4:42 PM, Brad Spencer <b...@anduin.eldar.org> wrote:
>
> Jason Thorpe <thor...@me.com> writes:
>
>>> On May 30, 2018, at 11:54 AM, Brad Spencer <b...@anduin.eldar.org> 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
