From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 400BEC77B7D for ; Thu, 18 May 2023 10:07:10 +0000 (UTC) Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id D12BD204; Thu, 18 May 2023 12:06:17 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz D12BD204 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1684404428; bh=n95vOdpJNyqFOR+qf2g5v9wQGqALmpMyo2jVk3XPOTQ=; h=Date:From:To:CC:Subject:References:In-Reply-To:List-Id: List-Archive:List-Help:List-Owner:List-Post:List-Subscribe: List-Unsubscribe:From; b=NmbSDVaKeg7ShzXlL/ZQY5DvkgODyqFh/TiBq3bvB6RnBCxI/zPLAxhU9RsfY/ouz GrqTVJXtJDM+emH2Pt6hWt1C+pGUeAcJwF3obO62ZkYnGbbr8Os2Hg6eIKIbuKnYG9 nMCk5bN89S5Q8QCe/4tZQClxED9jGHKO6VbmaXpg= Received: by alsa1.perex.cz (Postfix, from userid 50401) id 9EA0DF8053D; Thu, 18 May 2023 12:05:49 +0200 (CEST) Received: from mailman-core.alsa-project.org (mailman-core.alsa-project.org [10.254.200.10]) by alsa1.perex.cz (Postfix) with ESMTP id 41D54F8053D; Thu, 18 May 2023 12:05:49 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 61549F80272; Thu, 18 May 2023 12:05:44 +0200 (CEST) Received: from mx0b-001ae601.pphosted.com (mx0b-001ae601.pphosted.com [67.231.152.168]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id EF332F8016D for ; Thu, 18 May 2023 12:05:37 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz EF332F8016D Authentication-Results: alsa1.perex.cz; dkim=pass (2048-bit key, unprotected) header.d=cirrus.com header.i=@cirrus.com header.a=rsa-sha256 header.s=PODMain02222019 header.b=rFZUlfip Received: from pps.filterd (m0077474.ppops.net [127.0.0.1]) by mx0b-001ae601.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 34I8e9jF027959; Thu, 18 May 2023 05:05:35 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cirrus.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=PODMain02222019; bh=H6FDbp1npII27yTlAhF24XW68RkUa7dX7o8yv6tiCqc=; b=rFZUlfipRPwbaM3Gb1MOlnJexT2Hp6JgRgs9R54rn4p1uYbs9vn1WXeFkYbvfL3VZy0l prsdomk3BUHSQPlj2W8bdiSL2vTSAwhZf0AFbViZ9hxUNo2vf5Y3zTULdqTCKbhQl1+u 7OOXSoP7XzyTNrDJ/klgMsOgp+xZCNJ8dGqsoOSAGZumvz87bkJU21KFsZ5dsdwJSkch Py/UsEZ9mL46fomxwXkrB3fnJDuO3FR5KVf+piQ94g2AUPpfhAn89kn3BdwdFaUwogm9 B3bbUR3l8jIiU2lXH0EshPQzdlJm9FFi/FFFWE298ael1p03cydopKWtmaL0pP+m4kJm Pw== Received: from ediex02.ad.cirrus.com ([84.19.233.68]) by mx0b-001ae601.pphosted.com (PPS) with ESMTPS id 3qngq1g37w-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 18 May 2023 05:05:35 -0500 Received: from ediex02.ad.cirrus.com (198.61.84.81) by ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.26; Thu, 18 May 2023 05:05:34 -0500 Received: from ediswmail.ad.cirrus.com (198.61.86.93) by anon-ediex02.ad.cirrus.com (198.61.84.81) with Microsoft SMTP Server id 15.2.1118.26 via Frontend Transport; Thu, 18 May 2023 05:05:34 -0500 Received: from ediswmail.ad.cirrus.com (ediswmail.ad.cirrus.com [198.61.86.93]) by ediswmail.ad.cirrus.com (Postfix) with ESMTP id E56B711CA; Thu, 18 May 2023 10:05:33 +0000 (UTC) Date: Thu, 18 May 2023 10:05:33 +0000 From: Charles Keepax To: Pierre-Louis Bossart CC: , , , , , , , , , , , , , , , , , Subject: Re: [PATCH 06/10] mfd: cs42l43: Add support for cs42l43 core driver Message-ID: <20230518100533.GY68926@ediswmail.ad.cirrus.com> References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com> <20230512122838.243002-7-ckeepax@opensource.cirrus.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Proofpoint-GUID: ZkIENuEU-gCo6RFU-Z0OcYxUupdNyKMS X-Proofpoint-ORIG-GUID: ZkIENuEU-gCo6RFU-Z0OcYxUupdNyKMS X-Proofpoint-Spam-Reason: safe Message-ID-Hash: BSGJ7N4M4R55O5GVK22Q4SJGF63HZU2I X-Message-ID-Hash: BSGJ7N4M4R55O5GVK22Q4SJGF63HZU2I X-MailFrom: prvs=0502bbb445=ckeepax@opensource.cirrus.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-alsa-devel.alsa-project.org-0; header-match-alsa-devel.alsa-project.org-1; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.8 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" Archived-At: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Fri, May 12, 2023 at 09:52:21AM -0500, Pierre-Louis Bossart wrote: > > +static int cs42l43_sdw_interrupt(struct sdw_slave *sdw, > > + struct sdw_slave_intr_status *status) > > +{ > > + /* > > + * There is only a single bit in GEN_INT_STAT_1 and it doesn't clear if > > + * IRQs are still pending so doing a read/write here after handling the > > + * IRQ is fine. > > + */ > > + sdw_read_no_pm(sdw, CS42L43_GEN_INT_STAT_1); > > + sdw_write_no_pm(sdw, CS42L43_GEN_INT_STAT_1, 1); > > + > > + return 0; > > +} > > not really getting the comment and code above. Where is the IRQ handled? > In the 'other non-SoundWire part"? Yeah in the actual IRQ handler, I will update the comment to make this more clear. > > + ret = regmap_register_patch(cs42l43->regmap, cs42l43_reva_patch, > > + ARRAY_SIZE(cs42l43_reva_patch)); > > + if (ret) { > > + dev_err(cs42l43->dev, "Failed to apply register patch: %d\n", ret); > > + goto err; > > + } > > + > > + pm_runtime_mark_last_busy(cs42l43->dev); > > + pm_runtime_put_autosuspend(cs42l43->dev); > > any reason why the two pm_runtime routines are not placed last, just > before the return? Yeah that probably makes more sense I will update. > > + ret = cs42l43_power_up(cs42l43); > > + if (ret) > > + return ret; > > + > > + pm_runtime_set_autosuspend_delay(cs42l43->dev, 250); > > + pm_runtime_use_autosuspend(cs42l43->dev); > > + pm_runtime_set_active(cs42l43->dev); > > + pm_runtime_get_noresume(cs42l43->dev); > > you probably want a comment to explain that the get_noresume() is > intentional to prevent the device from suspending before the workqueue > is handled. Yeah will add one. > > + /* > > + * Don't care about being resumed here, but we do want force_resume to > > + * always trigger an actual resume, so that register state for the > > + * MCU/GPIOs is returned as soon as possible after system resume > > + */ > > + pm_runtime_get_noresume(dev); > > + > > + ret = pm_runtime_force_suspend(dev); > > + if (ret) { > > + dev_err(cs42l43->dev, "Failed to force suspend: %d\n", ret); > > + return ret; > > + } > > + > > + pm_runtime_put_noidle(dev); > > Is the get_noresume/put_noidle useful here? What does it do? The hope was the comment would explain this :-) Yeah it is a slightly surprising sequence. It is about ensuring force_resume runs a runtime resume, which it will only do if the reference count is resumed when we suspend. I will add a little more to the comment to hopefully clarify why we are doing this. > And it seems wrong anyways, if pm_runtime_force_suspend() fails then the > usage-count is not decreased. Yeah that is bug, thanks for the spot I will fix that up. > > +static int __maybe_unused cs42l43_runtime_resume(struct device *dev) > > +{ > > + struct cs42l43 *cs42l43 = dev_get_drvdata(dev); > > + unsigned int reset_canary; > > + int ret; > > + > > + dev_dbg(cs42l43->dev, "Runtime resume\n"); > > + > > + ret = cs42l43_wait_for_attach(cs42l43); > > is there a specific reason why the existing initialization_complete is > not used? Not massively, the driver does a fair amount of detaching and attaching during probe (has to soft reset a few times and they cause the device to fall off the bus). It was just slightly easier to keep track of all of them keeping the code internal to the driver and once we were doing it anyway it was less code to use the same mechanism on resume. Thanks, Charles