ec: poppy board uses 'mux_lock' without ' for Pericom
Reported by
vpalatin@chromium.org,
Jul 7 2017
|
|||
Issue description
The EC board code for Poppy declares the following Pericom PI3USB9281 chips:
struct mutex pericom_mux_lock;
struct pi3usb9281_config pi3usb9281_chips[] = {
{
.i2c_port = I2C_PORT_USB_CHARGER_0,
.mux_lock = &pericom_mux_lock,
},
{
.i2c_port = I2C_PORT_USB_CHARGER_1,
.mux_lock = &pericom_mux_lock,
},
};
BUILD_ASSERT(ARRAY_SIZE(pi3usb9281_chips) ==
CONFIG_USB_SWITCH_PI3USB9281_CHIP_COUNT);
Having the .mux_lock field set without the .mux_gpio and .mux_gpio_level fields set looks incorrect to me.
ie the pi3usb9281 will do:
if (chip->mux_lock) {
mutex_lock(chip->mux_lock);
gpio_set_level(chip->mux_gpio, chip->mux_gpio_level);
}
The gpio_set_level is probably harmless on most boards as the GPIO 0 is usually an input only interrupt one, but we still shouldn't do it in my opinion.
Not sure if the mutex here has a purpose or is just a leftover.
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform/ec/+/3ec62ac836ed0e6b7b29a81463814b5a71da2bb8 commit 3ec62ac836ed0e6b7b29a81463814b5a71da2bb8 Author: Nicolas Boichat <drinkcat@google.com> Date: Fri Jul 07 11:09:59 2017 poppy: Remove incorrect mux_lock in pi3usb9281_chips mux_lock is only used whenever the 2 PI3USB chips are sharing a common I2C bus, and is only valid if mux_gpio and mux_gpio_level are also set in the structure: if (chip->mux_lock) { mutex_lock(chip->mux_lock); gpio_set_level(chip->mux_gpio, chip->mux_gpio_level); } Let's remove mux_lock on poppy. BRANCH=none BUG= chromium:740010 TEST=Flash poppy, BC1.2 charging works on both ports. Change-Id: I76490c379efdfb3f37c590129429c163a03ae664 Reviewed-on: https://chromium-review.googlesource.com/563147 Commit-Ready: Nicolas Boichat <drinkcat@chromium.org> Tested-by: Nicolas Boichat <drinkcat@chromium.org> Reviewed-by: Vincent Palatin <vpalatin@chromium.org> [modify] https://crrev.com/3ec62ac836ed0e6b7b29a81463814b5a71da2bb8/board/poppy/board.c
,
Jul 7 2017
,
Jan 22 2018
|
|||
►
Sign in to add a comment |
|||
Comment 1 by drinkcat@chromium.org
, Jul 7 2017Nice catch, thanks. I guess this was copied from oak: struct pi3usb9281_config pi3usb9281_chips[] = { { .i2c_port = I2C_PORT_PERICOM, .mux_gpio = GPIO_USB_C_BC12_SEL, .mux_gpio_level = 0, .mux_lock = &pericom_mux_lock, }, { .i2c_port = I2C_PORT_PERICOM, .mux_gpio = GPIO_USB_C_BC12_SEL, .mux_gpio_level = 1, .mux_lock = &pericom_mux_lock, }, }; USB_C_BC12_SEL is used to multiplex I2C SCL between the 2 charger chips. Clearly not needed on poppy.