Re: [PATCH v5 2/4] ASoC: Intel - add Skylake HDA audio driver

[Mark Brown <broonie@kernel.org>]

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

On Thu, Jun 11, 2015 at 10:03:56PM +0530, Vinod Koul wrote:

> +/* initialize SD streams, use seprate streeam tag for PB and CP */
> +static int skl_init_stream(struct hdac_ext_bus *ebus, int start_idx,
> +				int num_stream, int dir)
> +{
> +	int stream_tag = 0;
> +	int i, tag, idx = start_idx;
> +
> +	for (i = 0; i < num_stream; i++) {
> +		struct hdac_ext_stream *stream =
> +				kzalloc(sizeof(*stream), GFP_KERNEL);
> +		if (!stream)
> +			return -ENOMEM;
> +		tag = ++stream_tag;
> +		snd_hdac_ext_stream_init(ebus, stream, idx, dir, tag);
> +		idx++;
> +	}
> +
> +	return 0;
> +}
> +
> +static void skl_free_streams(struct hdac_ext_bus *ebus)
> +{
> +	struct hdac_stream *s;
> +	struct hdac_ext_stream *stream;
> +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> +
> +	while (!list_empty(&bus->stream_list)) {
> +		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
> +		stream = stream_to_hdac_ext_stream(s);
> +		list_del(&s->list);
> +		kfree(stream);
> +	}
> +}

Still not sure why these are Sky Lake specific?

> +static void skl_free_hda_links(struct hdac_ext_bus *ebus)
> +{
> +	struct hdac_ext_link *l;
> +
> +	while (!list_empty(&ebus->hlink_list)) {
> +		l = list_first_entry(&ebus->hlink_list, struct hdac_ext_link, list);
> +		list_del(&l->list);
> +		kfree(l);
> +	}
> +}

Or this?

> +static int skl_suspend(struct device *dev)
> +{
> +	struct pci_dev *pci = to_pci_dev(dev);
> +	struct hdac_ext_bus *ebus = pci_get_drvdata(pci);
> +	struct hdac_bus *bus = ebus_to_hbus(ebus);
> +
> +	snd_hdac_bus_stop_chip(bus);
> +	snd_hdac_bus_enter_link_reset(bus);
> +	if (bus->irq >= 0) {
> +		free_irq(bus->irq, bus);


Why are we freeing the interrupt over suspend?  That is very unusual
behaviour.

> +static int skl_dmic_device_register(struct skl *skl)
> +{
> +	struct hdac_bus *bus = ebus_to_hbus(&skl->ebus);
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("dmic-codec", -1);
> +	if (!pdev) {
> +		dev_err(bus->dev, "failed to allocate dmic device\n");
> +		return -1;
> +	}
> +
> +	ret = platform_device_add(pdev);
> +	if (ret) {
> +		dev_err(bus->dev, "failed to add hda codec device\n");
> +		platform_device_put(pdev);
> +		return -1;
> +	}

Don't squash the error codes you get, return them - and if you must
return a fixed error code I'm pretty sure you don't mean -EPERM.

> +	skl->dmic_dev = pdev;

There can only ever be one DMIC?

> +
> +static const struct hdac_io_ops skl_io_ops = {
> +	.reg_writel = skl_pci_writel,
> +	.reg_readl = skl_pci_readl,
> +	.reg_writew = skl_pci_writew,
> +	.reg_readw = skl_pci_readw,
> +	.reg_writeb = skl_pci_writeb,
> +	.reg_readb = skl_pci_readb,
> +	.dma_alloc_pages = skl_dma_alloc_pages,
> +	.dma_free_pages = skl_dma_free_pages,
> +};

Still not thrilled at open coding these wrappers.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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