From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-106111.protonmail.ch (mail-106111.protonmail.ch [79.135.106.111]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6B90C3A7F51 for ; Wed, 14 Oct 2026 06:26:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=79.135.106.111 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791959175; cv=none; b=FvAUbsC7/vB4Lfo+qa3t6nas2MOxZnN8iX+DW5cc7Y923HPr7cD+/sTzMyLbq+zl3h1PfXEg4dEvaxz+DjYkepb0HEPkcAos9osJQuTL8Rdhn/O3xbhP5YDNYwhBvIvA3t33EGw6Tln0LmOhB+gtWahSQ389RcxDs1lso9+wlnE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1791959175; c=relaxed/simple; bh=AOdH/NtuZ7x8y7DtXRJfW6mTAkgGYDqaCZ1dUmXuEjw=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=T3vMFUolpLiYjKyzcfS+h6HXrPhIDw8GublSrYAqCmQyIYZMnpJFimwdyxTY5eRgXyHfrOLgFxGGbiJHJvsaxpQ2LoBvURJO028H9bncktrUOznYR5MNt18SeVS7J18XWSRJ04QoU9RzkRrP3EXY4/ictaFFJY1FyiN2Gy5ZQVk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=geanix.com; spf=pass smtp.mailfrom=geanix.com; dkim=pass (2048-bit key) header.d=geanix.com header.i=@geanix.com header.b=RCnt3iiX; arc=none smtp.client-ip=79.135.106.111 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=geanix.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=geanix.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=geanix.com header.i=@geanix.com header.b="RCnt3iiX" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=geanix.com; s=protonmail; t=1791959165; x=1792218365; bh=jD2XxFEplXV76tZsHTrcQNbZhqqu40Gf29di8WhQIGA=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID:From:To: Cc:Date:Subject:Reply-To:Feedback-ID:Message-ID:BIMI-Selector; b=RCnt3iiXjVOA/QmNhoDMEnaYhWbTGZYUqsSlknJWb2BUFtA7GIajfbKewS21/JT9z +45PB/J9yEfcskhTLK0k+bZNm62rQ0L6yZr+QjpmuDtWCFC9Z6XUzlGlWVH9vppvL0 etJenDqSrVpFaytJqe3BPjETUz5x8PVHHL26wqntgdNgdGbP1hR9huav8YqVAzuHmi 7kcEIle+sVeT+abWU3T1COJ6FPsmOFbgYpYBTutIYHnO5vqMTu1Ci2HpdedLP77W0m 0p8SeikFj2U9Tl4mHZEUOZZ127CsNUob75Q4yE3QGj1NVC+D02He18Bs7AqcrUqPd5 4+RIpweQ5NwHw== X-Pm-Submission-Id: 4j4Ln666YWz2ScPW From: Esben Haabendal To: "Jonathan Cameron" Cc: "Andy Shevchenko" , "Lars-Peter Clausen" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Martin Kepplinger" , "Sean Nyekjaer" , "David Lechner" , Nuno =?utf-8?Q?S=C3=A1?= , "Andy Shevchenko" , "Martin Kepplinger" , "Christoph Muellner" , , , Subject: Re: [PATCH v10 10/10] iio: accel: mma8452: Support interrupt sharing In-Reply-To: <20261011134146.3ae020a1@jic23-hlaptop> References: <20260928-mma8452-open-drain-v10-0-b906fb408386@geanix.com> <20260928-mma8452-open-drain-v10-10-b906fb408386@geanix.com> <87mrt1vfz6.fsf@geanix.com> <20261011134146.3ae020a1@jic23-hlaptop> Date: Wed, 14 Oct 2026 08:26:02 +0200 Message-ID: <877bjkn6h1.fsf@geanix.com> User-Agent: Gnus/5.13 (Gnus v5.13) Precedence: bulk X-Mailing-List: devicetree@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain "Jonathan Cameron" writes: > On Mon, 28 Sep 2026 12:15:57 +0200 > Esben Haabendal wrote: > >> "Andy Shevchenko" writes: >> >> > On Mon, Sep 28, 2026 at 10:26:26AM +0200, Esben Haabendal wrote: >> >> Adding support for sharing interrupt line with other device requires the >> >> interrupt handler to handle runtime PM suspension properly, ignoring the >> >> irq if the device is suspended (maybe even off). And while at it, we use >> >> the PM reference to ensure we do not get suspended while processing an irq. >> >> >> >> In order to prevent the chip from raising irq while suspended (that is when >> >> using fixed regulator, where suspend just means setting the device in >> >> STANDBY mode), we disable all interrupt sources by clearing CTRL_REG4, and >> >> then restores the value again when resuming. >> >> >> >> The scoped_guard in mma8452_runtime_suspend() is changed to a plain >> >> mutex_lock() instead, both to prevent mixing guards and goto, but also to >> >> ensure that we stay in STANDBY mode while writing to CTRL_REG4 and all the >> >> way up to disabling the device as much as possible. >> >> >> >> With that in place, it is safe to add the IRQF_SHARED flag. >> >> >> >> Keep in mind that the device by default is using push-pull for the irq pin, >> >> which might require additional hardware design to allow interrupt sharing. >> > >> > ... >> > >> >> + pm_status = pm_runtime_get_if_active(dev); >> >> + if (pm_status == 0) >> >> + /* device is powered down */ >> >> + return IRQ_NONE; >> > >> >> + if (IS_ENABLED(CONFIG_PM) && pm_status < 0) >> >> + /* runtime PM was disabled, possibly suspending */ >> >> + return IRQ_HANDLED; >> > >> > This is very interesting part. I bet this will be the first driver using this. >> > A big question "why?" >> >> Because as I understood it, I needed to add this for the patch to be >> accepted [1]. It is definitely possible that I have misunderstood that. >> >> The technical reason I believe is that during system suspend, >> pm_runtime_force_suspend() disabled runtime PM, and if a shared >> interrupt durings this, pm_runtime_get_if_active returns -EINVAL, and we >> could therefore end up doing I2C reads on unpowered hardware. >> >> Per that reasoning, we probably should be doing this in other drivers as >> well. > > I suspect that the combination of a shared interrupt and runtime pm > is moderately rare and even when it is there are drivers doing their > own tracking to avoid this sort of corner case. > > There are a lot of these nasty corners in runtime pm :( > > This seems like a reasonable solution to me, but given > all the discussion on this version was around this patch I'm going > to pick up 1-9 to but ask you to send a new version of this one to > make sure we get maximum eyes on it. Please include the PM maintainer > as well in the +CC in case we have this all wrong! Okay. Anything that I should change before sending a new version? And should I reset to version 1, or should it be v11? /Esben > >> >> >> + /* >> >> + * pm_status is now 1 or -EINVAL (with CONFIG_PM not enabled). If >> >> + * pm_status==1, runtime PM is enabled and device is RPM_ACTIVE. If >> >> + * pm_status==-EINVAL, runtime PM is build-time disabled (i.e. CONFIG_PM >> >> + * not enabled), and we can/must assume device is active. >> >> + */ >> >> + >> >> src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC); >> >> if (src < 0) >> >> - return IRQ_NONE; >> >> + goto out_runtime_put; >> >> >> >> if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY))) >> >> - return IRQ_NONE; >> >> + goto out_runtime_put; >> >> >> >> if (src & MMA8452_INT_DRDY) { >> >> iio_trigger_poll_nested(indio_dev->trig); >> >> @@ -1120,6 +1139,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p) >> >> ret = IRQ_HANDLED; >> >> } >> >> >> >> +out_runtime_put: >> >> + if (pm_status > 0) >> >> + pm_runtime_put_autosuspend(dev); >> >> + >> >> return ret; >> >> } >> > >> > ... >> > >> >> + pm_runtime_enable(dev); >> >> + pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS); >> >> + pm_runtime_use_autosuspend(dev); >> > >> > I don't see the respective _dont_use_autosuspend() call anywhere. >> >> Ah. I was not aware it was attached to a usage counter as well. I will >> add a pm_runtime_dont_use_autosuspend() to the runtime_suspend: error handling. >> >> > ... >> > >> > I am wondering how many of the above possible scenarios you were able >> > to test. >> >> Not enough. I haven't done any additional instrumentation to cause all >> the various error conditions that is handled here in _probe(). >> >> > ... >> > >> > P.S. I bet you can now make a presentation "PM runtime in Linux and >> > why it is so hard." >> >> LOL. I am getting there, yes. I have definitely learned a lot about how >> runtime PM works. Especially since I was mostly starting from scratch on >> that. >> >> But i fear it would easily end up being quite a chaotic presentation :) > > :) > > I've vaguely be wondering if we need a bunch of self test + examples > to poke all the more crazy corners of runtime pm. That way we can > at least be sure that if we do finally understand it we aren't going > to get any regressions. Not a bad idea :) /Esben