Re: [PATCH 06/14] ASoC: Intel: byt/cht: set pm ops dynamically - Pierre-Louis Bossart

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Mark Brown <broonie@kernel.org>
Cc: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>,
	alsa-devel@alsa-project.org, tiwai@suse.de,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Hans de Goede <hdegoede@redhat.com>,
	Rander Wang <rander.wang@linux.intel.com>
Subject: Re: [PATCH 06/14] ASoC: Intel: byt/cht: set pm ops dynamically
Date: Tue, 17 Nov 2020 11:39:48 -0600	[thread overview]
Message-ID: <da03823b-baa0-91b6-c2b0-85733192af68@linux.intel.com> (raw)
In-Reply-To: <20201117171842.GF5142@sirena.org.uk>

On 11/17/20 11:18 AM, Mark Brown wrote:
> On Thu, Nov 12, 2020 at 04:38:17PM -0600, Pierre-Louis Bossart wrote:
> 
>> +	/* set pm ops */
>> +	if (sof_parent)
>> +		pdev->dev.driver->pm = &snd_soc_pm_ops;
> 
> This might need revisiting in future since we should be able to have the
> driver PM ops be static const and hence unwritable but that's more of a
> robustness thing for the time being given that only a limited set of
> systems have this hardware and we know that there can't be multiple
> devices.

FWIW it's been done in other places, e.g.

drivers/net/wireless/ti/wlcore/main.c:  wl->dev->driver->pm = 
&wlcore_pm_ops;
drivers/net/wireless/ti/wlcore/main.c:  wl->dev->driver->pm = NULL;

The alternative would be to add an .ops whose callbacks conditionally 
call snd_soc_suspend/resume/poweroff. Not much cleaner, is it?

The check on the 'sof_parent' was not present in initial versions, I had 
an additional 'machine parameter' set by the SOF driver. But early 
reviewers suggested a check on the parent name was enough. It achieves 
the same thing in the end, make sure that we don't change anything for 
power management when the Atom/SST driver is used.