Shubhrajyoti Datta <omaplinuxkernel@xxxxxxxxx> writes: > On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman > <khilman@xxxxxxxxxxxxxxxxxxx> wrote: >> "Strashko, Grygorii" <grygorii.strashko@xxxxxx> writes: >> >>> Hi Kevin, >>> >>> yep, [1] is the same fix - thanks. >> >>> Hi Kalle, >>> >>> i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) >>> Could you check it with your use case, pls? (just to be sure that idea is right) >> >> I think the ideas is right, but [1] introduces more potential problems >> since it disables the IRQ at the INTC well before being disabled at the >> device. > I fail to see this point. Current driver supports master mode only. > So unless there is a msg queued by the core. May be no interrupts should fire. > > what I mean > > msg -> conf -> intr -> completion/error -> autosuspend. > >> With runtime PM autosuspend timeouts, that means any IRQs >> firing during the autosuspend delay will be lost, which basically >> nullifies the use of autosuspend. > > so I do not expect any interrupts between completion/error -> autosuspend. Runtime PM is designed for concurrent users (hence the usecounting.) The I2C driver may not fully support this, since there is a single bus to share, but the usage of runtime PM currently allows multiple users to do runtime PM get/put (though in this driver they will block in the wait_for_bb.) So the fundamental problem in doing the enable/disable IRQ in the xfer function, and not the runtime PM callbacks is that you're ignoring the runtime PM usecount. You're assuming that the 'get' is *always* incrementing the usecount from zero to 1, and the 'put' is *always* a transition from 1 to zero. This may be the case in current usage and tests, but does not have to be the case. Even if that never happens in practice, it can in theory, and to me leads to confusing code. It simply doesn't make sense in terms of readability to disable the IRQ at the INTC in xfer, and disable IRQs at the device level in the runtime PM callback. To put it more simply: anything that needs to be done when the I2C is about to be idled/enabled should be done in the runtime PM callbacks. Please have a look at the patch I just sent[1]. In addition to making it more readable (IMO), it elminates the need for an extra disable_irq() in probe. Kevin [1] http://marc.info/?l=linux-omap&m=135006723121147&w=2 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
