* Andreas Kemnade <andreas@xxxxxxxxxxxx> [190222 20:08]:
> Some more research:
> what failed is this:
> static int
> omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> {
> struct omap_i2c_dev *omap = i2c_get_adapdata(adap);
> int i;
> int r;
>
> r = pm_runtime_get_sync(omap->dev);
> if (r < 0) {
> we get -EACCESS there.
OK
> It is indeed the case that
> handle_twl4030_pih is called before i2c runtime resume.
> Since we have threaded irq here, we can do some sleep in case of
> problems. So this dirty hack helps:
>
> diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c
> index b16c16f194fd..6c729c4ec9b0 100644
> --- a/drivers/mfd/twl4030-irq.c
> +++ b/drivers/mfd/twl4030-irq.c
> @@ -34,6 +34,7 @@
> #include <linux/of.h>
> #include <linux/irqdomain.h>
> #include <linux/mfd/twl.h>
> +#include <linux/delay.h>
>
> #include "twl-core.h"
>
> @@ -298,7 +299,11 @@ static irqreturn_t handle_twl4030_pih(int irq, void *devid)
> REG_PIH_ISR_P1);
> if (ret) {
> pr_warn("twl4030: I2C error %d reading PIH ISR\n", ret);
> - return IRQ_NONE;
> + msleep(10);
> + ret = twl_i2c_read_u8(TWL_MODULE_PIH, &pih_isr,
> + REG_PIH_ISR_P1);
> + if (ret)
> + return IRQ_NONE;
> }
>
> while (pih_isr) {
How about just check for -EACCESS and return IRQ_NONE without
warning here? And only warn for other errors.
Then in Linux next, we now have new i2c_mark_adapter_suspended(),
so later on if we have something like is_i2c_adapter_suspended()
that could be added. But that's not going to be available for
v5.0 kernels, so for the fix I'd go with just -EACCES + IRQ_NONE.
> So if for some reason handle_twl4030_pih is called late enough, we will
> not have those problems.
> So maybe we just need to add a suspend/resume callback pair in
> twl-core.c and disable/enable the pih there? So when the pih is run,
> runtime_pm is ready, so you are allowed to do pm_runtime_get()
>
> To wake up from suspend, it is sufficient to have
>
> twl4030_pins: pinmux_twl4030_pins {
> pinctrl-single,pins = <
> OMAP3_CORE1_IOPAD(0x21e0, PIN_INPUT_PULLUP | PIN_OFF_WAKEUPENABLE | MUX_MODE0) /* sys_nirq.sys_nirq */
> >;
> };
Well PIN_OFF_WAKEUPENABLE is nowadays managed with Linux generic
wakeirqs with dev_pm_set_dedicated_wake_irq(). But in this case
you should not even need to set a pad wake up as i2c1 is in
always-on domain.
Because of the RTC on twl, it's best to not add new dependencies
pin muxing and generic wakeirqs that might be needed if we mask
the twl interrupt for suspend.
Regards,
Tony
