Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/dev/i2c - No need to use I2C_F_POLL here.



details:   https://anonhg.NetBSD.org/src/rev/dbdd21b45065
branches:  trunk
changeset: 1005884:dbdd21b45065
user:      thorpej <thorpej%NetBSD.org@localhost>
date:      Mon Dec 23 21:24:59 2019 +0000

description:
- No need to use I2C_F_POLL here.
- Refactor register read / write functions, splitting out i2c bus
  acquire / release everywhere.  This fixes what was almost certainly
  a deadlock in the FDT regulator section of the code (acquire bus ->
  read register, which again acquires bus -> locking against myself).

diffstat:

 sys/dev/i2c/tps65217pmic.c |  118 ++++++++++++++++++++++++++++++--------------
 1 files changed, 79 insertions(+), 39 deletions(-)

diffs (264 lines):

diff -r 716df56f9aeb -r dbdd21b45065 sys/dev/i2c/tps65217pmic.c
--- a/sys/dev/i2c/tps65217pmic.c        Mon Dec 23 20:49:09 2019 +0000
+++ b/sys/dev/i2c/tps65217pmic.c        Mon Dec 23 21:24:59 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: tps65217pmic.c,v 1.14 2019/11/03 22:55:34 jmcneill Exp $ */
+/*     $NetBSD: tps65217pmic.c,v 1.15 2019/12/23 21:24:59 thorpej Exp $ */
 
 /*-
  * Copyright (c) 2013 The NetBSD Foundation, Inc.
@@ -37,7 +37,7 @@
 #include "opt_fdt.h"
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: tps65217pmic.c,v 1.14 2019/11/03 22:55:34 jmcneill Exp $");
+__KERNEL_RCSID(0, "$NetBSD: tps65217pmic.c,v 1.15 2019/12/23 21:24:59 thorpej Exp $");
 
 #include <sys/param.h>
 #include <sys/systm.h>
@@ -146,12 +146,14 @@
        bool is_xadj;                   /* voltage is adjusted externally */
 
        uint16_t current_voltage;       /* in mV */
-
 };
 
 static int tps65217pmic_match(device_t, cfdata_t, void *);
 static void tps65217pmic_attach(device_t, device_t, void *);
 
+static int tps65217pmic_i2c_lock(struct tps65217pmic_softc *);
+static void tps65217pmic_i2c_unlock(struct tps65217pmic_softc *);
+
 static uint8_t tps65217pmic_reg_read(struct tps65217pmic_softc *, uint8_t);
 static void tps65217pmic_reg_write(struct tps65217pmic_softc *, uint8_t,
     uint8_t);
@@ -398,11 +400,19 @@
        intrmask = TPS65217PMIC_INT_USBM | TPS65217PMIC_INT_ACM |
            TPS65217PMIC_INT_PBM;
 
+       if (tps65217pmic_i2c_lock(sc) != 0) {
+               aprint_error_dev(sc->sc_dev,
+                   "failed to initialize power monitor\n");
+               return;
+       }
+
        status = tps65217pmic_reg_read(sc, TPS65217PMIC_STATUS);
        ppath = tps65217pmic_reg_read(sc, TPS65217PMIC_PPATH);
        /* acknowledge and disregard whatever interrupt was generated earlier */
        intr = tps65217pmic_reg_read(sc, TPS65217PMIC_INT);
 
+       tps65217pmic_i2c_unlock(sc);
+
        sc->sc_usbstatus = status & TPS65217PMIC_STATUS_USBPWR;
        sc->sc_acstatus = status & TPS65217PMIC_STATUS_ACPWR;
        sc->sc_usbenabled = ppath & TPS65217PMIC_PPATH_USB_EN;
@@ -430,7 +440,14 @@
 
        mutex_enter(&sc->sc_lock);
 
+       if (tps65217pmic_i2c_lock(sc) != 0) {
+               device_printf(sc->sc_dev,
+                   "WARNING: unable to perform power monitor task.\n");
+               return;
+       }
        status = tps65217pmic_reg_read(sc, TPS65217PMIC_STATUS);
+       tps65217pmic_i2c_unlock(sc);
+
        usbstatus = status & TPS65217PMIC_STATUS_USBPWR;
        acstatus = status & TPS65217PMIC_STATUS_ACPWR;
 
@@ -510,11 +527,19 @@
                return;
        }
 
+       if (tps65217pmic_i2c_lock(sc) != 0) {
+               device_printf(sc->sc_dev,
+                   "WARNING: unable to configure LED\n");
+               return;
+       }
+
        tps65217pmic_reg_write(sc, TPS65217PMIC_WLEDCTRL1, val);
        tps65217pmic_reg_write(sc, TPS65217PMIC_WLEDCTRL2,
            (brightness - 1) & TPS65217PMIC_WLEDCTRL2_DUTY);
        val |= TPS65217PMIC_WLEDCTRL1_ISINK_EN;
        tps65217pmic_reg_write(sc, TPS65217PMIC_WLEDCTRL1, val);
+
+       tps65217pmic_i2c_unlock(sc);
 }
 
 static void
@@ -523,10 +548,18 @@
        int i;
        struct tps_reg_param *c_reg;
 
+       if (tps65217pmic_i2c_lock(sc) != 0) {
+               device_printf(sc->sc_dev,
+                   "WARNING: unable to refresh regulators\n");
+               return;
+       }
+
        for (i = 0; i < NTPS_REG; i++) {
                c_reg = &tps_regulators[i];
                tps65217pmic_regulator_read_config(sc, c_reg);
        }
+
+       tps65217pmic_i2c_unlock(sc);
 }
 
 /* Get version and revision of the chip. */
@@ -535,8 +568,16 @@
 {
        uint8_t chipid;
 
+       if (tps65217pmic_i2c_lock(sc) != 0) {
+               device_printf(sc->sc_dev,
+                   "WARNING: unable to get chip ID\n");
+               return;
+       }
+
        chipid = tps65217pmic_reg_read(sc, TPS65217PMIC_CHIPID);
 
+       tps65217pmic_i2c_unlock(sc);
+
        sc->sc_version = chipid & TPS65217PMIC_CHIPID_VER_MASK;
        sc->sc_revision = chipid & TPS65217PMIC_CHIPID_REV_MASK;
 }
@@ -694,32 +735,45 @@
        aprint_normal("\n");
 }
 
+static int
+tps65217pmic_i2c_lock(struct tps65217pmic_softc *sc)
+{
+       int error;
+
+       error = iic_acquire_bus(sc->sc_tag, 0);
+       if (error) {
+               device_printf(sc->sc_dev,
+                   "unable to acquire i2c bus, error %d\n", error);
+       }
+       return error;
+}
+
+static void
+tps65217pmic_i2c_unlock(struct tps65217pmic_softc *sc)
+{
+       iic_release_bus(sc->sc_tag, 0);
+}
+
 static uint8_t
 tps65217pmic_reg_read(struct tps65217pmic_softc *sc, uint8_t reg)
 {
        uint8_t wbuf[2];
        uint8_t rv;
 
-       if (iic_acquire_bus(sc->sc_tag, I2C_F_POLL) != 0) {
-               aprint_error_dev(sc->sc_dev, "cannot acquire bus for read\n");
-               return 0;
-       }
-
        wbuf[0] = reg;
 
        if (iic_exec(sc->sc_tag, I2C_OP_READ_WITH_STOP, sc->sc_addr, wbuf,
-           1, &rv, 1, I2C_F_POLL)) {
+           1, &rv, 1, 0)) {
                aprint_error_dev(sc->sc_dev, "cannot execute operation\n");
-               iic_release_bus(sc->sc_tag, I2C_F_POLL);
+               iic_release_bus(sc->sc_tag, 0);
                return 0;
        }
-       iic_release_bus(sc->sc_tag, I2C_F_POLL);
 
        return rv;
 }
 
 static void
-tps65217pmic_reg_write_unlocked(struct tps65217pmic_softc *sc,
+tps65217pmic_reg_write(struct tps65217pmic_softc *sc,
     uint8_t reg, uint8_t data)
 {
        uint8_t wbuf[2];
@@ -728,40 +782,26 @@
        wbuf[1] = data;
 
        if (iic_exec(sc->sc_tag, I2C_OP_WRITE_WITH_STOP, sc->sc_addr, NULL, 0,
-           wbuf, 2, I2C_F_POLL)) {
+           wbuf, 2, 0)) {
                aprint_error_dev(sc->sc_dev, "cannot execute I2C write\n");
        }
 }
 
-static void __unused
-tps65217pmic_reg_write(struct tps65217pmic_softc *sc, uint8_t reg, uint8_t data)
-{
-
-       if (iic_acquire_bus(sc->sc_tag, I2C_F_POLL) != 0) {
-               aprint_error_dev(sc->sc_dev, "cannot acquire bus for write\n");
-               return;
-       }
-
-       tps65217pmic_reg_write_unlocked(sc, reg, data);
-
-       iic_release_bus(sc->sc_tag, I2C_F_POLL);
-}
-
 static void
 tps65217pmic_reg_write_l2(struct tps65217pmic_softc *sc,
     uint8_t reg, uint8_t data)
 {
        uint8_t regpw = reg ^ TPS65217PMIC_PASSWORD_XOR;
-       if (iic_acquire_bus(sc->sc_tag, I2C_F_POLL) != 0) {
-               aprint_error_dev(sc->sc_dev, "cannot acquire bus for write\n");
+
+       if (tps65217pmic_i2c_lock(sc))
                return;
-       }
 
-       tps65217pmic_reg_write_unlocked(sc, TPS65217PMIC_PASSWORD, regpw);
-       tps65217pmic_reg_write_unlocked(sc, reg, data);
-       tps65217pmic_reg_write_unlocked(sc, TPS65217PMIC_PASSWORD, regpw);
-       tps65217pmic_reg_write_unlocked(sc, reg, data);
-       iic_release_bus(sc->sc_tag, I2C_F_POLL);
+       tps65217pmic_reg_write(sc, TPS65217PMIC_PASSWORD, regpw);
+       tps65217pmic_reg_write(sc, reg, data);
+       tps65217pmic_reg_write(sc, TPS65217PMIC_PASSWORD, regpw);
+       tps65217pmic_reg_write(sc, reg, data);
+
+       tps65217pmic_i2c_unlock(sc);
 }
 
 static void
@@ -946,7 +986,7 @@
        uint8_t val;
        int error;
 
-       error = iic_acquire_bus(pmic_sc->sc_tag, I2C_F_POLL);
+       error = tps65217pmic_i2c_lock(pmic_sc);
        if (error != 0)
                return error;
 
@@ -959,7 +999,7 @@
 
        regulator->is_enabled = enable;
 
-       iic_release_bus(pmic_sc->sc_tag, I2C_F_POLL);
+       tps65217pmic_i2c_unlock(pmic_sc);
 
        return 0;
 }
@@ -972,13 +1012,13 @@
        struct tps_reg_param *regulator = sc->sc_param;
        int error;
 
-       error = iic_acquire_bus(pmic_sc->sc_tag, I2C_F_POLL);
+       error = tps65217pmic_i2c_lock(pmic_sc);
        if (error != 0)
                return error;
 
        error = tps65217pmic_set_volt(pmic_sc->sc_dev, regulator->name, min_uvol / 1000);
 
-       iic_release_bus(pmic_sc->sc_tag, I2C_F_POLL);
+       tps65217pmic_i2c_unlock(pmic_sc);
 
        return error;
 }



Home | Main Index | Thread Index | Old Index