On Tue, Nov 17, 2020 at 11:39:48AM -0600, Pierre-Louis Bossart wrote:
> 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?

It's not really about cleanliness, it's about being able to mark the ops
struct as const and therefore make it read only which the security
people like.

> 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.

The check is fine.