From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe Date: Thu, 24 Jan 2019 20:08:23 +0100 Message-ID: References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <1548351403-1875-1-git-send-email-spujar@nvidia.com> Sender: linux-kernel-owner@vger.kernel.org To: Sameer Pujar Cc: pierre-louis.bossart@linux.intel.com, perex@perex.cz, alsa-devel@alsa-project.org, thierry.reding@gmail.com, jonathanh@nvidia.com, rlokhande@nvidia.com, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org List-Id: linux-tegra@vger.kernel.org On Thu, 24 Jan 2019 18:36:43 +0100, Sameer Pujar wrote: > > If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks > will not be ON. This could cause issue during probe, where hda init > setup is done. This patch checks whether runtime PM is enabled or not. > If disabled, clocks are enabled in probe() and disabled in remove() > > This patch does following minor changes as cleanup, > * return code check for pm_runtime_get_sync() to take care of failure > and exit gracefully. > * In remove path runtime PM is disabled before calling snd_card_free(). > * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check. > * runtime PM callbacks moved out of CONFIG_PM check > > Signed-off-by: Sameer Pujar > Reviewed-by: Ravindra Lokhande > Reviewed-by: Jon Hunter (snip) > @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev) > if (!azx_has_pm_runtime(chip)) > pm_runtime_forbid(hda->dev); > > + /* explicit resume if runtime PM is disabled */ > + if (!pm_runtime_enabled(hda->dev)) { > + err = hda_tegra_runtime_resume(hda->dev); > + if (err) > + goto out_free; > + } > + > schedule_work(&hda->probe_work); Calling runtime_resume here is really confusing... > @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work) > struct platform_device *pdev = to_platform_device(hda->dev); > int err; > > - pm_runtime_get_sync(hda->dev); > + err = pm_runtime_get_sync(hda->dev); > + if (err < 0) { > + dev_err(hda->dev, > + "failed in pm_runtime_get_syc with err = %d\n", > + err); > + return; > + } This pm_runtime_get_sync() is needed just because you enabled runtime PM before probe_work. Why not deferring the runtime PM enablement after probing done? That is what we need is the hda_tegra_enable_clocks() call at probe unconditionally before enabling runtime PM. > err = hda_tegra_first_init(chip, pdev); > if (err < 0) > goto out_free; > @@ -599,12 +611,13 @@ static void hda_tegra_probe_work(struct work_struct *work) > > static int hda_tegra_remove(struct platform_device *pdev) > { > - int ret; > - > - ret = snd_card_free(dev_get_drvdata(&pdev->dev)); > pm_runtime_disable(&pdev->dev); > + if (!pm_runtime_status_suspended(&pdev->dev)) { > + hda_tegra_runtime_suspend(&pdev->dev); > + pm_runtime_set_suspended(&pdev->dev); > + } > - return ret; > + return snd_card_free(dev_get_drvdata(&pdev->dev)); Forcing the suspend *before* snd_card_free() doesn't sound right. It's the point before the disconnect and release procedure of all the rest. That is, the other hardware components are still active at this point. thanks, Takashi 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 X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 47B43C282C3 for ; Thu, 24 Jan 2019 19:08:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1E699218A2 for ; Thu, 24 Jan 2019 19:08:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728937AbfAXTIZ (ORCPT ); Thu, 24 Jan 2019 14:08:25 -0500 Received: from mx2.suse.de ([195.135.220.15]:34396 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725909AbfAXTIY (ORCPT ); Thu, 24 Jan 2019 14:08:24 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 6FABCAE52; Thu, 24 Jan 2019 19:08:23 +0000 (UTC) Date: Thu, 24 Jan 2019 20:08:23 +0100 Message-ID: From: Takashi Iwai To: "Sameer Pujar" Cc: , , , , , , , Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe In-Reply-To: <1548351403-1875-1-git-send-email-spujar@nvidia.com> References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 24 Jan 2019 18:36:43 +0100, Sameer Pujar wrote: > > If CONFIG_PM is disabled or runtime PM calls are forbidden, the clocks > will not be ON. This could cause issue during probe, where hda init > setup is done. This patch checks whether runtime PM is enabled or not. > If disabled, clocks are enabled in probe() and disabled in remove() > > This patch does following minor changes as cleanup, > * return code check for pm_runtime_get_sync() to take care of failure > and exit gracefully. > * In remove path runtime PM is disabled before calling snd_card_free(). > * hda_tegra_disable_clocks() is moved out of CONFIG_PM_SLEEP check. > * runtime PM callbacks moved out of CONFIG_PM check > > Signed-off-by: Sameer Pujar > Reviewed-by: Ravindra Lokhande > Reviewed-by: Jon Hunter (snip) > @@ -555,6 +553,13 @@ static int hda_tegra_probe(struct platform_device *pdev) > if (!azx_has_pm_runtime(chip)) > pm_runtime_forbid(hda->dev); > > + /* explicit resume if runtime PM is disabled */ > + if (!pm_runtime_enabled(hda->dev)) { > + err = hda_tegra_runtime_resume(hda->dev); > + if (err) > + goto out_free; > + } > + > schedule_work(&hda->probe_work); Calling runtime_resume here is really confusing... > @@ -571,7 +576,14 @@ static void hda_tegra_probe_work(struct work_struct *work) > struct platform_device *pdev = to_platform_device(hda->dev); > int err; > > - pm_runtime_get_sync(hda->dev); > + err = pm_runtime_get_sync(hda->dev); > + if (err < 0) { > + dev_err(hda->dev, > + "failed in pm_runtime_get_syc with err = %d\n", > + err); > + return; > + } This pm_runtime_get_sync() is needed just because you enabled runtime PM before probe_work. Why not deferring the runtime PM enablement after probing done? That is what we need is the hda_tegra_enable_clocks() call at probe unconditionally before enabling runtime PM. > err = hda_tegra_first_init(chip, pdev); > if (err < 0) > goto out_free; > @@ -599,12 +611,13 @@ static void hda_tegra_probe_work(struct work_struct *work) > > static int hda_tegra_remove(struct platform_device *pdev) > { > - int ret; > - > - ret = snd_card_free(dev_get_drvdata(&pdev->dev)); > pm_runtime_disable(&pdev->dev); > + if (!pm_runtime_status_suspended(&pdev->dev)) { > + hda_tegra_runtime_suspend(&pdev->dev); > + pm_runtime_set_suspended(&pdev->dev); > + } > - return ret; > + return snd_card_free(dev_get_drvdata(&pdev->dev)); Forcing the suspend *before* snd_card_free() doesn't sound right. It's the point before the disconnect and release procedure of all the rest. That is, the other hardware components are still active at this point. thanks, Takashi