Thanks for the late night review Andy :-)

On 12/11/18 4:20 PM, Andy Shevchenko wrote:

On Tue, Dec 11, 2018 at 03:23:05PM -0600, Pierre-Louis Bossart wrote:

...

From: Liam Girdwood liam.r.girdwood@linux.intel.com

The Sound Open Firmware driver core is a generic architecture independent layer that allows SOF to be used on many different different architectures and platforms. It abstracts DSP operations and IO methods so that the target DSP can be an internal memory mapped or external SPI or I2C based device. This abstraction also allows SOF to be run on many different VMs on the same physical HW.

SOF also requires some data in ASoC PCM runtime data for looking up SOF data during ASoC PCM operations. +/* SOF defaults if not provided by the platform in ms */ +#define TIMEOUT_DEFAULT_IPC 5 +#define TIMEOUT_DEFAULT_BOOT 100

Perhaps _MS at the end?

ok

...

+struct snd_sof_dai *snd_sof_find_dai(struct snd_sof_dev *sdev,

• 		     char *name)
  

+{

• struct snd_sof_dai *dai = NULL;
• list_for_each_entry(dai, &sdev->dai_list, list) {

• if (!dai->name)
  

• 	continue;
  

• if (strcmp(name, dai->name) == 0)
  

• 	return dai;
  

Perhaps

if (dai->name && strcmp(...))

ok

...

• }
• return NULL;

+} +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++) {

• u32 idx = (((i << 2) + i)) >> 1;
  

Looks like

u32 idx = (5 * i) >> 1;

Yes, but we'd also want an explanation of what we try to multiply by 2.5... Liam or Keyon, can you chime in?

...

• 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;
  

• }
• return pages;

+}

• if (plat_data->type == SOF_DEVICE_PCI)

• sdev->pci = container_of(plat_data->dev, struct pci_dev, dev);
  

Why not to use generic functions? dev_is_pci() to_pci_dev()

ok

...

• /* register machine driver, pass machine info as pdata */
• plat_data->pdev_mach =

• platform_device_register_data(sdev->dev, drv_name,
  

• 			      -1, mach, size);
  

PLATFORM_DEVID_AUTO (IIRC the name)?

did you mean replace -1 by PLATFORM_DEVID_NONE?

...

+static int sof_remove(struct platform_device *pdev) +{

• struct snd_sof_dev *sdev = dev_get_drvdata(&pdev->dev);
• struct snd_sof_pdata *pdata = sdev->pdata;
• if (pdata && !IS_ERR(pdata->pdev_mach))

• platform_device_unregister(pdata->pdev_mach);
  

I'm wondering if pdata could be ever NULL here.

I didn't find an error flow that yields NULL but Liam reported some issues that made no sense so might be safer with NULL

Also, as I mentioned internally the patch to accept error pointers would be in v4.21.

Indeed, but I can't rely on this just yet.

...

• snd_soc_unregister_component(&pdev->dev);
• snd_sof_fw_unload(sdev);
• snd_sof_ipc_free(sdev);
• snd_sof_free_debug(sdev);
• snd_sof_free_trace(sdev);
• snd_sof_remove(sdev);
• return 0;

+}

+void snd_sof_shutdown(struct device *dev) +{ +} +EXPORT_SYMBOL(snd_sof_shutdown);

No need to provide an empty stub. Why not to add when it would have something useful?

ok

...

+/* max BARs mmaped devices can use */ +#define SND_SOF_BARS 8

+/* time in ms for runtime suspend delay */ +#define SND_SOF_SUSPEND_DELAY 2000

_MS ?

ok

...

• struct mutex mutex; /* access mutex */
• struct list_head list; /* list in sdev pcm list */
• struct snd_pcm_hw_params params[2];
• int restore_stream[2]; /* restore hw_params for paused stream */
• u32 readback_offset; /* offset to mmaped data if used */
• struct sof_ipc_ctrl_data *control_data;
• u32 size; /* cdata size */
• enum sof_ipc_ctrl_cmd cmd;
• u32 *volume_table; /* volume table computed from tlv data*/
• struct mutex mutex; /* access mutex */
• struct list_head list; /* list in sdev control list */