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,URIBL_BLOCKED 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 3B441C282C0 for ; Fri, 25 Jan 2019 13:58:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 12314218CD for ; Fri, 25 Jan 2019 13:58:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728892AbfAYN6K (ORCPT ); Fri, 25 Jan 2019 08:58:10 -0500 Received: from mx2.suse.de ([195.135.220.15]:55780 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726095AbfAYN6J (ORCPT ); Fri, 25 Jan 2019 08:58:09 -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 802FBACA9; Fri, 25 Jan 2019 13:58:07 +0000 (UTC) Date: Fri, 25 Jan 2019 14:58:07 +0100 Message-ID: From: Takashi Iwai To: Jon Hunter Cc: Sameer Pujar , , , , , , , Subject: Re: [PATCH] ALSA: hda/tegra: enable clock during probe In-Reply-To: References: <1548351403-1875-1-git-send-email-spujar@nvidia.com> <06c00ce1-32ed-8aa9-0340-d00202a8fa62@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 Fri, 25 Jan 2019 14:26:27 +0100, Jon Hunter wrote: > > > On 25/01/2019 12:40, Takashi Iwai wrote: > > On Fri, 25 Jan 2019 12:36:00 +0100, > > Jon Hunter wrote: > >> > >> > >> On 24/01/2019 19:08, Takashi Iwai wrote: > >>> 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... > >> > >> Why? IMO it is better to have a single handler for resuming the device > >> and so if RPM is not enabled we call the handler directly. This is what > >> we have been advised to do in the past and do in other drivers. See ... > > > > The point is that we're not "resuming" anything there. It's in the > > early probe stage, and the device state is uninitialized, not really > > suspended. It'd end up with just calling the same helper > > (hda_tegra_enable_clocks()), though. > > Yes and you can make the same argument for every driver that calls > pm_runtime_get_sync() during probe to turn on clocks, handle resets, > etc, because at the end of the day the very first call to > pm_runtime_get_sync() invokes the runtime_resume callback, when we have > never been suspended. Although there are some magical pm_runtime_*() in some places, most of such pm_runtime_get_sync() is for the actual runtime PM management (to prevent the runtime suspend), while the code above is for explicitly setting up something for non-PM cases. And if pm_runtime_get_sync() is obviously superfluous, we should remove such calls. Really. > Yes at the end of the day it is the same and given that we have done > this elsewhere I think it is good to be consistent if/where we can. The code becomes less readable, and that's a good reason against it :) thanks, Takashi