Re: [Patch v2 05/11] ASoC: ipq806x: Add LPASS CPU DAI driver

[Mark Brown <broonie@kernel.org>]

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

On Mon, Dec 08, 2014 at 02:01:07PM -0800, Kenneth Westfield wrote:

Please stop CCing Rob Herring's Calxeda address, it bounces.

> +	default:
> +		pr_err("%s: invalid bitwidth given: %u\n", __func__, bitwidth);
> +		return -EINVAL;
> +	}

Repeating again: dev_err().

> +	ret = lpass_lpaif_mi2s_channels(prtd, channels, bit_act);

> +	ret = lpass_lpaif_mi2s_bitwidth(prtd, bitwidth);

Just inline these helper functions, they're basically just abstracting a
single switch statement each which adds little if anything.

> +static int lpass_cpu_mi2s_daiops_hw_free(struct snd_pcm_substream *substream,
> +		struct snd_soc_dai *dai)
> +{
> +	struct lpass_cpu_mi2s_data *prtd = snd_soc_dai_get_drvdata(dai);
> +
> +	if (prtd->mi2s_clocks_enabled) {
> +		clk_disable_unprepare(prtd->mi2s_osr_clk);
> +		clk_disable_unprepare(prtd->mi2s_bit_clk);
> +	}

This seems problematic, why is the clock being disabled here rather than
in a place matching that where it was enabled so we don't need to do
this checking.  I suspect you should be using a DAPM widget to manage
the clocks.

> +	prtd->irq_acquired = 0;

What is this supposed to do?  It looks write only.

> +#ifndef _LPASS_CPU_MI2S_H
> +#define _LPASS_CPU_MI2S_H
> +
> +enum pinctrl_pin_state {
> +	STATE_DISABLED = 0,
> +	STATE_ENABLED = 1
> +};
> +static const char *const pin_states[] = {"Disabled", "Enabled"};

This apppears to be the same pinctrl stuff you had in the Maxim CODEC
driver.  Similar issues with reproducing core pinctrl functionality
apply here too, and the fact that the code has been cut'n'pasted between
different drivers isn't a good sign.

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