Re: [Sound-open-firmware] [PATCH v4 01/14] ASoC: SOF: Add Sound Open Firmware driver core

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

Thanks for the quick reviews Takashi!

On 2/14/19 3:25 AM, Takashi Iwai wrote:
> On Wed, 13 Feb 2019 23:07:21 +0100,
> Pierre-Louis Bossart wrote:
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index eb7db605955b..da3c13185263 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -1232,6 +1232,9 @@ struct snd_soc_pcm_runtime {
>>   	/* bit field */
>>   	unsigned int dev_registered:1;
>>   	unsigned int pop_wait:1;
>> +
>> +	/* private data - core does not touch */
>> +	void *private; /* FIXME: still SOF-specific, needs to less ambiguous */
> 
> Then better to name it sof_private?

Ah, this is a mistake in the FIXME comment.
initially we had a field called "sof" and Mark made the following comment:

"
 > +++ b/include/sound/soc.h
 > @@ -1133,6 +1133,7 @@ struct snd_soc_pcm_runtime {
 >  	/* runtime devices */
 >  	struct snd_pcm *pcm;
 >  	struct snd_compr *compr;
 > +	struct snd_sof_pcm *sof;
 >  	struct snd_soc_dai *codec_dai;
 >  	struct snd_soc_dai *cpu_dai;

Can we rename this somehow to be less specific to SoF or move it to be
somewhere other than the runtime structure?  I can see why you've done
this but I can also see every DSP vendor turning up and trying to add
their own field in here.
"

so we added a FIXME to track this, then later we moved the field and 
renamed it 'private' so that it can be used by others if needed but 
didn't remove the FIXME. Will fix this.

> 
> 
>> +int snd_sof_create_page_table(struct snd_sof_dev *sdev,
>> +			      struct snd_dma_buffer *dmab,
>> +			      unsigned char *page_table, size_t size)
>> +{
>> +	int i, pages;
>> +
>> +	pages = snd_sgbuf_aligned_pages(size);
>> +
>> +	dev_dbg(sdev->dev, "generating page table for %p size 0x%zx pages %d\n",
>> +		dmab->area, size, pages);
>> +
>> +	for (i = 0; i < pages; i++) {
>> +		/*
>> +		 * The number of valid address bits for each page is 20.
>> +		 * idx determines the byte position within page_table
>> +		 * where the current page's address is stored
>> +		 * in the compressed page_table.
>> +		 * This can be calculated by multiplying the page number by 2.5.
>> +		 */
>> +		u32 idx = (5 * i) >> 1;
>> +		u32 pfn = snd_sgbuf_get_addr(dmab, i * PAGE_SIZE) >> PAGE_SHIFT;
>> +		u32 *pg_table;
>> +
>> +		dev_vdbg(sdev->dev, "pfn i %i idx %d pfn %x\n", i, idx, pfn);
>> +
>> +		pg_table = (u32 *)(page_table + idx);
>> +
>> +		if (i & 1)
>> +			*pg_table |= (pfn << 4);
>> +		else
>> +			*pg_table |= pfn;
> 
> I guess this is OK for now, but better to give some notes that it's
> x86-specific.  To be more generic, we'd have to consider endianess and
> the unaligned access in the code above.

yes indeed, will check if we can make this more generic, and add notes 
in any case.