Re: [PATCH 3/9] ASoC: ipq806x: add native LPAIF driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3991 bytes --]

On Wed, Nov 19, 2014 at 10:52:43AM -0800, Kenneth Westfield wrote:

> +#define DRV_NAME	"lpass-lpaif"
> +#define DRV_VERSION	"1.0"

Don't add versions like this, the kernel is already more than adequately
versioned and nobody is ever going to bother to update it.

> +static int lpaif_pcm_int_enable(uint8_t dma_ch)
> +{
> +	uint32_t intr_val;
> +	uint32_t status_val;
> +	unsigned long flags;
> +
> +	if (dma_ch >= LPAIF_MAX_CHANNELS) {
> +		pr_err("%s: invalid DMA channel given: %hhu\n",
> +				__func__, dma_ch);

dev_err().

> +void lpaif_cfg_i2s_playback(uint8_t enable, uint32_t mode, uint32_t off)
> +{

The kernel types for fixed size unsigned integers are u8, u32 and so on.

> +		if ((bit_width == 16) && (channels == 2)) {
> +			cfg |= LPAIF_DMACTL_WPSCNT_MONO;
> +		} else if (((bit_width == 16) && (channels == 4)) ||

switch statements please, it's both more legible and more extensible.

> +void lpaif_register_dma_irq_handler(int dma_ch,
> +		irqreturn_t (*callback)(int intrsrc, void *private_data),
> +		void *private_data)
> +{
> +	lpaif_dai[dma_ch]->callback = callback;
> +	lpaif_dai[dma_ch]->private_data = private_data;
> +}
> +
> +void lpaif_unregister_dma_irq_handler(int dma_ch)
> +{
> +	lpaif_dai[dma_ch]->callback = NULL;
> +	lpaif_dai[dma_ch]->private_data = NULL;
> +}

What is this doing?  Linux already has a perfectly good interface for
requesting and releasing interrupts...

> +static int lpaif_dai_find_dma_channel(uint32_t intrsrc)
> +{
> +	uint32_t dma_channel = 0;
> +
> +	while (dma_channel < LPAIF_MAX_CHANNELS) {
> +		if (intrsrc & LPAIF_PER_CH(dma_channel))
> +			return dma_channel;
> +
> +		dma_channel++;
> +	}

A comment explaining why we can't just map directly might be helpful
here.

> +
> +	return -1;

Real error codes please.

> +static irqreturn_t lpaif_dai_irq_handler(int irq, void *data)
> +{
> +	unsigned long flag;
> +	uint32_t intrsrc;
> +	uint32_t dma_ch;
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	spin_lock_irqsave(&lpaif_lock, flag);
> +	intrsrc = readl(lpaif_dai_info.base + LPAIF_IRQ_STAT(0));
> +	writel(intrsrc, lpaif_dai_info.base + LPAIF_IRQ_CLEAR(0));
> +	spin_unlock_irqrestore(&lpaif_lock, flag);

This appears to be unconditionally acknowleding all interrupts, even
those we don't understand.  This is bad practice - we should at least be
logging an error and ideally returning IRQ_NONE if we don't understand
the interrupt and letting the core error handling work for us.  For
example it looks like this will happily and silently acknowledge both
error and underrun interrupts from the hardware.

It's also not at all obvious why we're taking this spinlock in the
interrupt handler, that's *extremely* unusual.  What is being protected
- the code needs to make that clear?

> +	while (intrsrc) {
> +		dma_ch = lpaif_dai_find_dma_channel(intrsrc);
> +		if (dma_ch != -1) {
> +			if (lpaif_dai[dma_ch]->callback) {
> +
> +				ret = lpaif_dai[dma_ch]->callback(intrsrc,
> +					lpaif_dai[dma_ch]->private_data);
> +			}
> +			intrsrc &= ~LPAIF_PER_CH(dma_ch);
> +		} else {
> +			pr_err("%s: error getting channel\n", __func__);
> +			break;
> +		}
> +	}
> +	return ret;
> +}

This all looks like a very simple demux of a single register - don't we
have a generic irqchip that can handle it?  It looks like that's what
you're trying to implement here, each DMA channel could be requesting
the three interrupts it has separately rather than open coding an
interrupt controller here.  

I'm not actually immediately seeing one right now but it'd be simple to
add (or regmap-irq could do it if we used a regmap, though it assumes
threading and isn't a great fit).

> +static struct platform_driver lpass_lpaif_driver = {
> +	.probe = lpaif_dai_probe,
> +	.remove = lpaif_dai_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.owner = THIS_MODULE,

No need for .owner on platform devices any more.

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