[alsa-devel] [PATCH v3 03/14] ASoC: SOF: Add driver debug support.

[Pierre-Louis Bossart]

[combined feedback from Ranjani, Liam and me]


>> +	/* copy from DSP MMIO */
>> +	pm_runtime_get_noresume(sdev->dev);
> Why are we doing a _get_noresume() here?  It won't actually do anything
> to the device...
the reason we currently do a get_noresume() is so that we can still read 
the debugfs entries in case of DSP panic, where a regular resume would 
not succeed.

That said this part will be reworked based on feedback from Rafael whose 
suggestion was to get rid of the pm calls while reading debugfs. We can 
make sure that all the values are flushed out of the dsp before 
suspending which will make the need to wake it up redundant.

>
>> +
>> +	memcpy_fromio(buf,  dfse->buf + pos, size);
> Extra space?
yep. we run checkpatch.pl --strict all the time and missed this somehow.
>
>> +	/*
>> +	 * TODO: revisit to check if we need mark_last_busy, or if we
>> +	 * should change to use xxx_put_sync[_suspend]().
>> +	 */
>> +	ret = pm_runtime_put_sync_autosuspend(sdev->dev);
>> +	if (ret < 0)
>> +		dev_warn(sdev->dev, "warn: debugFS failed to autosuspend %zd\n",
>> +			 ret);
> It rather depends what you're doing...  I'm definitely confused as to
> why you need a _sync operation - if you're doing autosuspend stuff
> presumably you don't care so much if the device gets powered down
> immediately, and I can't in general see why that'd be important.
yes, as stated above we'll rework all this
>
>> +	dfse->dfsentry = debugfs_create_file(name, 0444, sdev->debugfs_root,
>> +					     dfse, &sof_dfs_fops);
>> +	if (!dfse->dfsentry) {
>> +		dev_err(sdev->dev, "error: cannot create debugfs entry.\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(snd_sof_debugfs_io_create_item);
> debugfs uses EXPORT_SYMBOL_GPL().

We don't have a burning desire to keep this as EXPORT_SYMBOL since 
debugfs is really Linux specific, so will move all these statements as 
EXPORT_SYMBOL_GPL.

Thanks Mark for your time, much appreciated. We've already dealt with 
most of the feedback from Takashi/Andy/Daniel so should have a new 
version of these patches in 1-2 weeks.