[alsa-devel] [PATCH v4 5/7] ASoC: intel - add Skylake HDA audio driver

[Mark Brown]

On Mon, May 11, 2015 at 04:24:03PM +0530, Vinod Koul wrote:

> +static void azx_free_streams(struct soc_hdac_bus *sbus)
> +{
> +	struct hdac_stream *s;
> +	struct soc_hdac_stream *stream;
> +	struct hdac_bus *bus = hdac_bus(sbus);
> +
> +	while (!list_empty(&bus->stream_list)) {
> +		s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
> +		stream = stream_to_soc_hdac_stream(s);
> +		list_del(&s->list);
> +		kfree(stream);
> +	}
> +}

Why is this (and the equivalent link function) device specific code?
They look very generic...

> +static int azx_acquire_irq(struct soc_hdac_bus *sbus, int do_disconnect)
> +{
> +	struct hda_skl *hda = to_hda_skl(sbus);
> +	struct hdac_bus *bus = hdac_bus(sbus);
> +	int ret = 0;
> +
> +	ret = request_threaded_irq(hda->pci->irq, azx_interrupt,
> +			azx_threaded_handler,
> +			hda->msi ? 0 : IRQF_SHARED,
> +			KBUILD_MODNAME, sbus);

Why not just always request the interrupt as shared - we don't seem to
care in the interrupt handling code?

> +/* PCI register access. */
> +static void pci_azx_writel(u32 value, u32 __iomem *addr)
> +{
> +	writel(value, addr);
> +}

These wrappers are setting off alarm bells - why can't we just use the
called functions directly, and given the parameters (which have just a
raw pointer and value) what else could the implementation be?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150529/59dcb05c/attachment.sig>