From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-43171.protonmail.ch (mail-43171.protonmail.ch [185.70.43.171]) (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 27857496D5E; Mon, 28 Sep 2026 10:17:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.70.43.171 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790590675; cv=none; b=FHgyVXcVOJ/rkP0HKKSTykrK2NRfEpW0mtNGBzjBgex0RX4GmJ/5zwfZyUvVZQcgnDPshQK3uzfw3PkiVXECnR18keOGQzbiPKPPdTPRimvvFo2/DppqDHjGz2t+wAY5orvx9r0MhmPU1MZYBTWFcNLO0+k/DZrQDpbHWrEQhBU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790590675; c=relaxed/simple; bh=rIg9380cRezZenEJuRoL6hRrOBHgZBhb2pmfGNTIoMA=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=TsH0V+7D9ZNwuaIiR5eLMUrd2ocdm7Z1JA7TRx3FUHJu/Tpvef5aoKuQ8OEm8lBGhHjO7jGhFWLUUknooCOAoQXi9o6QTmq7vLBuwx/LM0P2DAUlI5sQ1PSJW2JPak6jjUgxQ8BAp+v0Wd8/RN1qd/CaaNjxj6cLnGV8c1Ve7lQ= 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=oOX98vLJ; arc=none smtp.client-ip=185.70.43.171 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="oOX98vLJ" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=geanix.com; s=protonmail; t=1790590670; x=1790849870; bh=3zXWZcKRjA7HrYtEDUkyB4mBtf+UGGoBdFfoxKpnM3w=; 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=oOX98vLJq3mKjvgWAkNw/BQgYohrhn4JkwDei1z/4bGfp3sQIivRciJgNw2R0tP7c DMy1du5bhTdVYuCHZzTNhP1oGcJ33r6tzYMQb71H4/M8O8NgT7gbcuWzR5D7fy6Leg IT/xIPAdDBbVuaknl1eEUG6kJnFFGZbmElB4huOF7iZoJCKT2FH9OVmgSGOsrWVYc2 c3uDhCiGfQtAXUqN6QGBoUnKR4DUEiTdZYd6WlUwn7QdQsPbF3oRWJDzpo03Eba7mP NUMU6Z8hZLphSDDg90qJpv/SsfDZHq7Zj+tRZ7xeDSQUG2yWn5qymwahtRXju60+Lt QkNgGwrErYu3w== X-Pm-Submission-Id: 4htcgw0s44z1DDKy From: Esben Haabendal To: "Andy Shevchenko" Cc: "Jonathan Cameron" , "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: <87mrt1vfz6.fsf@geanix.com> References: <20260928-mma8452-open-drain-v10-0-b906fb408386@geanix.com> <20260928-mma8452-open-drain-v10-10-b906fb408386@geanix.com> <_qpupQDTVixVhDhmNMaIip5Umv0_ddRwhfIMlE9N6FZ7_FpG4rtj-V4vKQysOm8sYjNlBSU2SqKLvEneoiJK7Q==@protonmail.internalid> <87mrt1vfz6.fsf@geanix.com> Date: Mon, 28 Sep 2026 12:17:47 +0200 Message-ID: <87h5j9vfw4.fsf@geanix.com> User-Agent: Gnus/5.13 (Gnus v5.13) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain "Esben Haabendal" writes: > "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. Sorry, forgot to add the reference. [1] https://lore.kernel.org/all/20260921015115.4c7d79f3@jic23-hlaptop/ > > 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. > >>> + /* >>> + * 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 :) > > /Esben