From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (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 2F65F582BA8; Fri, 16 Oct 2026 17:32:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1792171954; cv=none; b=NWpplWBcRgy0rtBLsYVOaQk21p0VJrlZGhXkOk4rR4zAXJ4jvW5kladVwBHTyuV+uXH8c6RSoknQ1gblz6dm1OWWQ22pi0SnCoPf2krI3KCSlyeAKNqA4NMJ+fSY36FF4FUvvw8So+LDJgrCg/Qt222XXJ+9QTtwt0vOTsaEovs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1792171954; c=relaxed/simple; bh=Ns63LGgGXeTfZgjYanh9XF0PGadK6PdLUy8UZhCvrwQ=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=T3YJgel7ou2RuW+W91YWBwVnez7sHH9PCUzvPLxpQLdqfI8imfqAMgkXDFfML5ER0vrFNIpoXgPLmwBMun9VBRHvuLJGiWjKXjS0Vof66Ki7W8C1+CPwLsQztWNLk5yFsiKq9TV3mW9IRli3f24/SImTDw8RtoXDqM4ZVJrzn6I= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=fOEv1L23; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="fOEv1L23" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 15A641F000C6; Fri, 16 Oct 2026 17:32:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1792171953; bh=C0yoXdc6vSqaARtYU9OV8+3n8jcp1mX3G3yPqSziOCU=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=fOEv1L23cObP1nf6UVZYMaZEQN8x0bHHZBM4LBk9tnnwcJAxDX1Wh4+QCur7942el hPWmlYApNH3+NhHpoQYgckRrrkYtuLj++pbt8Mjtrbfd8vDh1S3JziGSqs6lHISddp 5hoxZJFUxqpO8PBBCfES3cB3DfeYUCEqeR1wbQ0veMETsh/ikNZVBvyXT7tpja6XQt A1ExkOnI7tJbFrCbt4SYP6bvY4BhoovxkJBwTbO2ON6qwVC59bJ1zv+MwNZ7bmDI8c 0uwsBMfCfvMO0TDJ8OUQmHRWptmuvEIgQzc/PlH+QNDmotRpPFUouh5hMFfiTFcY4d eIhNmmkwtvMww== Date: Fri, 16 Oct 2026 18:32:27 +0100 From: Jonathan Cameron To: Esben Haabendal Cc: "Andy Shevchenko" , "Lars-Peter Clausen" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Martin Kepplinger" , "Sean Nyekjaer" , "David Lechner" , Nuno =?UTF-8?B?U8Oh?= , "Andy Shevchenko" , "Martin Kepplinger" , "Christoph Muellner" , , , Subject: Re: [PATCH v10 10/10] iio: accel: mma8452: Support interrupt sharing Message-ID: <20261016183227.7914d3c5@jic23-hlaptop> In-Reply-To: <877bjkn6h1.fsf@geanix.com> 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> <877bjkn6h1.fsf@geanix.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Wed, 14 Oct 2026 08:26:02 +0200 Esben Haabendal wrote: > "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? v11. I lost track on whether anything makes sense to change so do what you think is best on that ;) Jonathan > > /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