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

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3216 bytes --]

On Thu, Nov 06, 2014 at 12:43:49PM +0000, Mark Brown wrote:
> 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).
yes that is doable by adding a parameter and setting that in specfic probe,
will add.

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

> 
> > +	/* 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?
Shouldnt be, let me verify

> 
> > +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?
Yes, anway this is only for ACPI systems.

> 
> > +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.
Yup, hopefully _DSD migration will help us here

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]