December 2018 - Sound-open-firmware - mailman.alsa-project.org

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 07/14] ASoC: SOF: Add DSP firmware logger support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:21 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:11PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> This patch adds support for real-time DSP logging (timestamped events >> and bespoke binary data) for firmware debug. The current solution >> relies on DMA transfers to system memory that is then accessed by >> userspace tools such as sof-logger. For Intel platforms, two types of >> DMAs are currently used (GP-DMA for Baytrail/CherryTrail and HDaudio >> DMA for SKL+) >> >> Due to historical reasons, the driver code follows the DSP firmware >> conventions and refers to 'traces', but it is currently unrelated to >> the Linux trace subsystem. Future solutions will include support for >> more advanced hardware (e.g. MIPI Sys-T), additional formats and the >> ability to enable/disable specific traces dynamically. >> + if (count > buffer_size - lpos) >> + count = buffer_size - lpos; > min() / max() ? > >> + /* make sure count is <= avail */ >> + count = avail > count ? count : avail; > ditto. > Check entire series for such. yep. > >> + dfse->dfsentry = debugfs_create_file("trace", 0444, sdev->debugfs_root, >> + dfse, &sof_dfs_trace_fops); >> + if (!dfse->dfsentry) { >> + dev_err(sdev->dev, >> + "error: cannot create debugfs entry for trace\n"); >> + kfree(dfse); >> + return -ENODEV; > Should not return an error. We must be fine w/o debugfs files. Not sure I fully agree with the 'must'. yes execution should proceed, but without debugfs we cannot, well, debug, so this is a real show-stopper > >> + } >> + >> + return 0; >> +}

2 1

[Sound-open-firmware] How do you provide the firmware binary, e.g sof-byt.ri file?
by Daniel Baluta 12 Dec '18

12 Dec '18

Hello all, I am having this problem where SOF is compiled as built in the kernel and the firmware binary is on the rootfs. When request_firmware is called in sof_probe the rootfs is not yet mounted so the call to request_firmware fails. Just curious, how do you provide the firmware binary? thanks, Daniel.

5 8

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 14/14] ASoC: SOF: Add utils
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:06 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:18PM -0600, Pierre-Louis Bossart wrote: >> Helpers to set-up back-ends, create platform devices and common >> IO/block read/write operations >> +int sof_bes_setup(struct device *dev, struct snd_sof_dsp_ops *ops, >> + struct snd_soc_dai_link *links, int link_num, >> + struct snd_soc_card *card) >> +{ >> + char name[32]; >> + int i; >> + >> + if (!ops || !links || !card) >> + return -EINVAL; >> + >> + /* set up BE dai_links */ >> + for (i = 0; i < link_num; i++) { >> + snprintf(name, 32, "NoCodec-%d", i); > sizeof(name) ? yes > >> + links[i].name = devm_kstrdup(dev, name, GFP_KERNEL); >> + if (!links[i].name) >> + return -ENOMEM; > ...or better devm_kasprintf(). and yes > >> + >> + links[i].id = i; >> + links[i].no_pcm = 1; >> + links[i].cpu_dai_name = ops->drv[i].name; >> + links[i].platform_name = "sof-audio"; >> + links[i].codec_dai_name = "snd-soc-dummy-dai"; >> + links[i].codec_name = "snd-soc-dummy"; >> + links[i].dpcm_playback = 1; >> + links[i].dpcm_capture = 1; >> + } >> + >> + card->dai_link = links; >> + card->num_links = link_num; >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(sof_bes_setup); >> + >> +/* register sof platform device */ >> +int sof_create_platform_device(struct sof_platform_priv *priv) >> +{ >> + struct snd_sof_pdata *sof_pdata = priv->sof_pdata; >> + struct device *dev = sof_pdata->dev; >> + >> + priv->pdev_pcm = >> + platform_device_register_data(dev, "sof-audio", -1, >> + sof_pdata, sizeof(*sof_pdata)); >> + if (IS_ERR(priv->pdev_pcm)) { >> + dev_err(dev, "error: cannot register device sof-audio. Error %d\n", >> + (int)PTR_ERR(priv->pdev_pcm)); > Instead of casting perhaps use %ld? yes > >> + return PTR_ERR(priv->pdev_pcm); >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(sof_create_platform_device); >> +void sof_io_write64(struct snd_sof_dev *sdev, void __iomem *addr, u64 value) >> +{ >> +#ifdef CONFIG_64BIT >> + writeq(value, addr); >> +#else >> + memcpy_toio(addr, &value, sizeof(value)); >> +#endif > This ifdef sounds strange. > Just include io64-nonatomic-lo-hi.h and use readq() w/o limitations. ah yes. We added readq/writeq following your initial feedback, which broke ARCH=i386 and that was added as a workaround. > >> +} >> +EXPORT_SYMBOL(sof_io_write64); >> + >> +u64 sof_io_read64(struct snd_sof_dev *sdev, void __iomem *addr) >> +{ >> +#ifdef CONFIG_64BIT >> + return readq(addr); >> +#else >> + u64 val; >> + >> + memcpy_fromio(&val, addr, sizeof(val)); >> + return val; >> +#endif > Ditto. > >> +} >> +EXPORT_SYMBOL(sof_io_read64); >> + affected_mask = (1 << (8 * n)) - 1; >> + >> + /* first read the 32bit data of dest, then change affected >> + * bytes, and write back to dest. For unaffected bytes, it >> + * should not be changed >> + */ >> + __ioread32_copy(&tmp, dest + m * 4, 1); >> + tmp &= ~affected_mask; >> + >> + tmp |= *(u32 *)(src_byte + m * 4) & affected_mask; >> + __iowrite32_copy(dest + m * 4, &tmp, 1); > Ain't these equivalents to simple ioread32() / iowrite32() ? They are. We removed a loop but the this can be further simplified indeed. Thanks for finding this. >

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 09/14] ASoC: SOF: Add firmware loader support
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 4:38 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:13PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> The firmware loader exports APIs that can be called by core to load and >> process multiple different file formats. >> +static int get_ext_windows(struct snd_sof_dev *sdev, >> + struct sof_ipc_ext_data_hdr *ext_hdr) >> +{ >> + struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr; >> + >> + int ret = 0; > I don't see how it's used. Perhaps you need to check code with `make W=1`. Darn, we missed this one. I thought we were using W=1 on github but we aren't, this will be fixed. Though W=1 doesn't report this one, so need to re-inspect this. Thanks for the sighting. > >> + size_t size; >> + >> + if (w->num_windows == 0 || w->num_windows > SOF_IPC_MAX_ELEMS) >> + return -EINVAL; >> + >> + size = sizeof(*w) + sizeof(struct sof_ipc_window_elem) * w->num_windows; >> + >> + /* keep a local copy of the data */ >> + sdev->info_window = kmemdup(w, size, GFP_KERNEL); >> + if (!sdev->info_window) >> + return -ENOMEM; >> + >> + return ret; >> +} >> + dev_warn(sdev->dev, >> + "warning: block %d size zero\n", count); >> + dev_warn(sdev->dev, " type 0x%x offset 0x%x\n", >> + block->type, block->offset); > Hmm... Why do we need a kernel level duplication in words? Not sure I get this one, it's just a warning message where you don't copy a block of size zero into the target memory. > >> +int snd_sof_load_firmware(struct snd_sof_dev *sdev) >> +{ >> + dev_dbg(sdev->dev, "loading firmware\n"); > Noise. > Better to introduce a trace points and drop all these kind of messages. it's not that bad, most people understand what dmesg is, it happens once and only if you have dynamic debug. At some point we also thought about something like drm_debug where you can specify which parts you are interested in, in addition to the verbosity level. > >> + >> + if (sdev->ops->load_firmware) >> + return sdev->ops->load_firmware(sdev); >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_load_firmware);

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 08/14] ASoC: SOF: Add DSP HW abstraction operations
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 5:16 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:12PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Add operation pointers that can be called by core to control a wide >> variety of DSP targets. The DSP HW drivers will fill in these >> operations. >> +int snd_sof_pci_update_bits_unlocked(struct snd_sof_dev *sdev, u32 offset, >> + u32 mask, u32 value) >> +{ >> + bool change; >> + unsigned int old, new; >> + u32 ret = ~0; /* explicit init to remove uninitialized use warnings */ >> + >> + pci_read_config_dword(sdev->pci, offset, &ret); >> + dev_dbg(sdev->dev, "Debug PCIR: %8.8x at %8.8x\n", >> + pci_read_config_dword(sdev->pci, offset, &ret), offset); >> + >> + old = ret; >> + new = (old & (~mask)) | (value & mask); >> + >> + change = (old != new); >> + if (change) { >> + pci_write_config_dword(sdev->pci, offset, new); >> + dev_dbg(sdev->dev, "Debug PCIW: %8.8x at %8.8x\n", value, >> + offset); >> + } >> + >> + return change; > What about dropping change completely? > > if (old == new) > return false; > > ... > return true; ok for all cases > > >> +} >> +EXPORT_SYMBOL(snd_sof_pci_update_bits_unlocked); >> +int snd_sof_dsp_update_bits_unlocked(struct snd_sof_dev *sdev, u32 bar, >> + u32 offset, u32 mask, u32 value) >> +{ >> + bool change; >> + unsigned int old, new; >> + u32 ret; >> + >> + ret = snd_sof_dsp_read(sdev, bar, offset); >> + >> + old = ret; >> + new = (old & (~mask)) | (value & mask); >> + >> + change = (old != new); >> + if (change) >> + snd_sof_dsp_write(sdev, bar, offset, new); >> + >> + return change; > Ditto. > Everywhere in similar places. > >> +} >> +EXPORT_SYMBOL(snd_sof_dsp_update_bits_unlocked); >> + /* check if set state successful */ >> + for (time = 5; time > 0; time--) { >> + if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == target) >> + break; >> + msleep(20); >> + } >> + >> + if (!time) { >> + /* sleeping in 10ms steps so adjust timeout value */ >> + timeout /= 10; >> + >> + for (time = timeout; time > 0; time--) { >> + if ((snd_sof_dsp_read(sdev, bar, offset) & mask) == >> + target) > I would leave it on one line. ok > >> + break; >> + >> + usleep_range(5000, 10000); >> + } >> + } >> + >> + ret = time ? 0 : -ETIME; >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(snd_sof_dsp_register_poll); >> +static inline void snd_sof_dsp_write64(struct snd_sof_dev *sdev, u32 bar, >> + u32 offset, u64 value) >> +{ >> + if (sdev->ops->write64) >> + sdev->ops->write64(sdev, >> + sdev->bar[bar] + offset, value); > Why not one line? ok > >> +} >

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 04/14] ASoC: SOF: Add support for IPC IO between DSP and Host
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

On 12/11/18 4:57 PM, Andy Shevchenko wrote: > On Tue, Dec 11, 2018 at 03:23:08PM -0600, Pierre-Louis Bossart wrote: >> From: Liam Girdwood <liam.r.girdwood(a)linux.intel.com> >> >> Define an IPC ABI for all host <--> DSP communication. This ABI should >> be transport agnostic. i.e. it should work on MMIO and SPI/I2C style >> interfaces. >> + /* ssc1: TINTE */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_TINTE (1 << 0) >> + /* ssc1: PINTE */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PINTE (1 << 1) >> + /* ssc2: SMTATF */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_SMTATF (1 << 2) >> + /* ssc2: MMRATF */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_MMRATF (1 << 3) >> + /* ssc2: PSPSTWFDFD */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSTWFDFD (1 << 4) >> + /* ssc2: PSPSRWFDFD */ >> +#define SOF_DAI_INTEL_SSP_QUIRK_PSPSRWFDFD (1 << 5) > Style mismatch. Looks like a candidate for BIT() Yes but no. This comes from firmware definitions, so it's easier to leave them alone. Changing the kernel side of the IPC for style reasons just adds more work and chances of divergence. > >> +#define SOF_DAI_FMT_FORMAT_MASK 0x000f >> +#define SOF_DAI_FMT_CLOCK_MASK 0x00f0 >> +#define SOF_DAI_FMT_INV_MASK 0x0f00 >> +#define SOF_DAI_FMT_MASTER_MASK 0xf000 > GENMASK() ? > >> +/* flags indicating which time stamps are in sync with each other */ >> +#define SOF_TIME_HOST_SYNC (1 << 0) >> +#define SOF_TIME_DAI_SYNC (1 << 1) >> +#define SOF_TIME_WALL_SYNC (1 << 2) >> +#define SOF_TIME_STAMP_SYNC (1 << 3) >> + >> +/* flags indicating which time stamps are valid */ >> +#define SOF_TIME_HOST_VALID (1 << 8) >> +#define SOF_TIME_DAI_VALID (1 << 9) >> +#define SOF_TIME_WALL_VALID (1 << 10) >> +#define SOF_TIME_STAMP_VALID (1 << 11) >> + >> +/* flags indicating time stamps are 64bit else 3use low 32bit */ >> +#define SOF_TIME_HOST_64 (1 << 16) >> +#define SOF_TIME_DAI_64 (1 << 17) >> +#define SOF_TIME_WALL_64 (1 << 18) >> +#define SOF_TIME_STAMP_64 (1 << 19) > BIT() ? > > >> +#define SOF_IPC_PANIC_MAGIC_MASK 0x0ffff000 >> +#define SOF_IPC_PANIC_CODE_MASK 0x00000fff > GENMASK() ? same for all, when the definitions are shared with firmware it's better to keep the values as is. > >> +#define IPC_TIMEOUT_MSECS 300 > Simple _MS ? yes. > >> +/* find original TX message from DSP reply */ >> +static struct snd_sof_ipc_msg *sof_ipc_reply_find_msg(struct snd_sof_ipc *ipc, >> + u32 header) >> +{ >> + struct snd_sof_dev *sdev = ipc->sdev; >> + struct snd_sof_ipc_msg *msg; >> + >> + header = SOF_IPC_MESSAGE_ID(header); >> + >> + if (list_empty(&ipc->reply_list)) >> + goto err; > Redundant. list_for_each_*() have this check already. ok, nice catch. > >> + >> + list_for_each_entry(msg, &ipc->reply_list, list) { >> + if (SOF_IPC_MESSAGE_ID(msg->header) == header) >> + return msg; >> + } >> + >> +err: >> + dev_err(sdev->dev, "error: rx list empty but received 0x%x\n", >> + header); >> + return NULL; >> +} >> +int snd_sof_ipc_valid(struct snd_sof_dev *sdev) >> +{ >> + struct sof_ipc_fw_ready *ready = &sdev->fw_ready; >> + struct sof_ipc_fw_version *v = &ready->version; >> + >> + dev_info(sdev->dev, >> + " Firmware info: version %d:%d:%d-%s\n", v->major, v->minor, > Indentation issues. I believe they are intentional, but that's Liam's part. > >> + v->micro, v->tag); >> + dev_info(sdev->dev, >> + " Firmware: ABI %d:%d:%d Kernel ABI %d:%d:%d\n", >> + SOF_ABI_VERSION_MAJOR(v->abi_version), >> + SOF_ABI_VERSION_MINOR(v->abi_version), >> + SOF_ABI_VERSION_PATCH(v->abi_version), >> + SOF_ABI_MAJOR, SOF_ABI_MINOR, SOF_ABI_PATCH); >> + >> + if (SOF_ABI_VERSION_INCOMPATIBLE(SOF_ABI_VERSION, v->abi_version)) { >> + dev_err(sdev->dev, "error: incompatible FW ABI version\n"); >> + return -EINVAL; >> + } >> + >> + if (ready->debug.bits.build) { >> + dev_info(sdev->dev, >> + " Firmware debug build %d on %s-%s - options:\n" >> + " GDB: %s\n" >> + " lock debug: %s\n" >> + " lock vdebug: %s\n", >> + v->build, v->date, v->time, >> + ready->debug.bits.gdb ? "enabled" : "disabled", >> + ready->debug.bits.locks ? "enabled" : "disabled", >> + ready->debug.bits.locks_verbose ? >> + "enabled" : "disabled"); > I would leave it on one line (only 3 characters more, but better readability). > > ready->debug.bits.locks_verbose ? "enabled" : "disabled"); That must be a result of me asking folks to run checkpatch.pl --strict... > >> + } >> + >> + /* copy the fw_version into debugfs at first boot */ >> + memcpy(&sdev->fw_version, v, sizeof(*v)); >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(snd_sof_ipc_valid); >> + dev_dbg(sdev->dev, "pre-allocate %d IPC messages\n", >> + IPC_EMPTY_LIST_SIZE); > Noise. ok > >> + >> + /* pre-allocate message data */ >> + for (i = 0; i < IPC_EMPTY_LIST_SIZE; i++) { >> + msg->msg_data = devm_kzalloc(sdev->dev, PAGE_SIZE, GFP_KERNEL); >> + if (!msg->msg_data) >> + return NULL; >> + >> + msg->reply_data = devm_kzalloc(sdev->dev, PAGE_SIZE, >> + GFP_KERNEL); >> + if (!msg->reply_data) >> + return NULL; > Can't you just get enough (non-continuous?) pages at once via get_free_pages interface? > I didn't know that one, so, consider rather a way to look into. Dunno either, maybe something for Liam to look at? > >> + >> + init_waitqueue_head(&msg->waitq); >> + list_add(&msg->list, &ipc->empty_list); >> + msg++; >> + } >> + >> + return ipc; >> +} >> +EXPORT_SYMBOL(snd_sof_ipc_init);

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 03/14] ASoC: SOF: Add driver debug support.
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

>> + if (count > size - pos) >> + count = size - pos; > max() / min() ok > >> + /* intermediate buffer size must be u32 multiple */ >> + size = (count + 3) & ~3; > Hmm, don't we have something like round_up()? yes, round_up (count, 4) would work. > >> + 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; >> + } > We shouldn't rely on debugfs files. > Thus, error checking needs to be removed. > > Something like > > void () > { > struct dentry *d; > > d = debugfs_...(); > if (!d) > dev_err(); > } You lost me on this one. The main purpose of this return -ENODEV is to avoid adding more entries if one item failed. It's not different from what our estimeed colleagues did in skl-debug.c > > >> + 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; >> + } > Ditto. > >> + sdev->debugfs_root = debugfs_create_dir("sof", NULL); >> + if (IS_ERR_OR_NULL(sdev->debugfs_root)) { >> + dev_err(sdev->dev, "error: failed to create debugfs directory\n"); >> + return -EINVAL; >> + } >> + >> + /* create debugFS files for platform specific MMIO/DSP memories */ >> + for (i = 0; i < ops->debug_map_count; i++) { >> + map = &ops->debug_map[i]; >> + >> + err = snd_sof_debugfs_io_create_item(sdev, sdev->bar[map->bar] + >> + map->offset, map->size, >> + map->name); >> + if (err < 0) >> + dev_err(sdev->dev, "error: cannot create debugfs for %s\n", >> + map->name); > Ditto (error message dups above). > >> + } >> + >> + return err; or maybe that's the issue, we shouldn't return an error on the init proper, but I see nothing wrong in stopping when things go south. >> +} >> +EXPORT_SYMBOL(snd_sof_dbg_init);

1 0

Re: [Sound-open-firmware] [PATCH v3 01/14] ASoC: SOF: Add Sound Open Firmware driver core
by Pierre-Louis Bossart 12 Dec '18

12 Dec '18

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(a)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 */

1 0

Re: [Sound-open-firmware] [alsa-devel] [PATCH v3 02/14] ASoC: SOF: Add Sound Open Firmware KControl support
by Pierre-Louis Bossart 11 Dec '18

11 Dec '18

>> + max_size = (be->max < max_size) ? be->max : max_size; > min() / max() ? Good catch, this can be simplified. Not sure I like max_size = min (be->max, max_size), it's confusing. Maybe if (be->max < max_size) max_size = be->max;

1 0