[alsa-devel] [PATCH v3 4/6] ASoC: Intel: mrfld- add ACPI module

[Mark Brown]

On Tue, Nov 04, 2014 at 04:25:18PM +0530, Vinod Koul wrote:

> @@ -323,7 +324,11 @@ EXPORT_SYMBOL_GPL(sst_context_init);
>  void sst_context_cleanup(struct intel_sst_drv *ctx)
>  {
>  	pm_runtime_get_noresume(ctx->dev);
> +#if IS_ENABLED(CONFIG_ACPI)
> +	pm_runtime_disable(ctx->dev);
> +#else
>  	pm_runtime_forbid(ctx->dev);
> +#endif
>  	sst_unregister(ctx->dev);
>  	sst_set_fw_state_locked(ctx, SST_SHUTDOWN);
>  	flush_scheduled_work();

No, we should be able to build a kernel which runs with or without ACPI
- make this a runtime check if it's required (a comment might be in
order too since this is a weird thing to do).

> @@ -371,8 +376,19 @@ void sst_configure_runtime_pm(struct intel_sst_drv *ctx)
>  {
>  	pm_runtime_set_autosuspend_delay(ctx->dev, SST_SUSPEND_DELAY);
>  	pm_runtime_use_autosuspend(ctx->dev);
> +#if IS_ENABLED(CONFIG_ACPI)
> +	/*
> +	 * For acpi devices, the actual physical device state is
> +	 * initially active. So change the state to active before
> +	 * enabling the pm
> +	 */
> +	pm_runtime_set_active(ctx->dev);
> +	pm_runtime_enable(ctx->dev);
> +	sst_save_shim64(ctx, ctx->shim, ctx->shim_regs64);
> +#else
>  	pm_runtime_allow(ctx->dev);
>  	pm_runtime_put_noidle(ctx->dev);
> +#endif

The general idiom for runtime PM is that we should default to bringing
the device to power up anyway, that way the kernel continues to work if
runtime PM has been disabled.

> +	/* save the shim registers because PMC doesn't save state */
> +	if (ctx->dev_id == SST_BYT_ACPI_ID)
> +		sst_save_shim64(ctx, ctx->shim, ctx->shim_regs64);

Is there any harm in doing this unconditionally?

> +static acpi_status sst_acpi_mach_match(acpi_handle handle, u32 level,
> +				       void *context, void **ret)
> +{
> +	*(bool *)context = true;
> +	return AE_OK;
> +}
> +
> +static struct sst_machines *sst_acpi_find_machine(
> +	struct sst_machines *machines)
> +{
> +	struct sst_machines *mach;
> +	bool found = false;
> +
> +	for (mach = machines; mach->codec_id; mach++)
> +		if (ACPI_SUCCESS(acpi_get_devices(mach->codec_id,
> +						  sst_acpi_mach_match,
> +						  &found, NULL)) && found)
> +			return mach;

acpi_get_devices() seems painful.

> +#else
> +int sst_acpi_probe(struct platform_device *pdev)
> +{
> +	return -EINVAL;
> +}
> +
> +int sst_acpi_remove(struct platform_device *pdev)
> +{
> +	return -EINVAL;
> +}
> +#endif

Why not just not build this file if !CONFIG_ACPI?

> +static struct sst_machines sst_acpi_bytcr[] = {
> +	{"10EC5640", "T100", "bytt100_rt5640", NULL, "fw_sst_0f28.bin",
> +						&byt_rvp_platform_data },
> +	{},
> +};

Yay.  Much win.  Not that there's much that can be done at this point.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20141106/39251a54/attachment.sig>