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.