Re: [PATCH 01/11] ASoC: SOF: Add Sound Open Firmware driver core

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jul 19, 2018 at 07:53:25PM +0100, Liam Girdwood wrote:

> 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

> +++ 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.

> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + * redistributing this file, you may do so under either license.
> + *
> + * Copyright(c) 2017 Intel Corporation. All rights reserved.

2017?

> index 000000000000..29458909182a
> --- /dev/null
> +++ b/sound/soc/sof/core.c
> @@ -0,0 +1,373 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +/*
> + * This file is provided under a dual BSD/GPLv2 license.  When using or

Please make the entire comment a C++ one, it makes it look more
intentional.

> +static inline unsigned int sof_get_pages(size_t size)
> +{
> +	return (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> +}

This feels like there should be a generic MM function for it?

> +/*
> + * Generic buffer page table creation.
> + */
> +
> +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;
> +		u32 pfn = snd_sgbuf_get_addr(dmab, i * PAGE_SIZE) >> PAGE_SHIFT;
> +		u32 *pg_table;
> +
> +		dev_dbg(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'm not 100% clear what this is intended to do - lots of magic numbers
and dependencies on the host memory configuration.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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