On Wed, Nov 19, 2014 at 10:52:44AM -0800, Kenneth Westfield wrote:
> + if (channels == 8) {
> + if (bit_width == 24 &&
> + ((samp_freq != 176400) &&
> + (samp_freq != 192000)))
> + return 2;
> + return 1;
Coding style - there's more brackets than are needed coupled with some
strange indentation (eg, the second samp_freq line being indented to the
outside bracket when it's still within that bracket). Use of switch
statements would probably help, at least on channels.
> +static int lpass_cpu_mi2s_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
> + uint32_t ret = 0;
> + uint32_t bit_act;
> + uint16_t bit_div;
> + uint32_t bit_width = params_format(params);
> + uint32_t channels = params_channels(params);
> + uint32_t rate = params_rate(params);
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct lpass_runtime_data_t *prtd = runtime->private_data;
> + struct mi2s_hw_params curr_params;
> +
> + bit_act = snd_pcm_format_width(bit_width);
> + if (bit_act == LPASS_INVALID) {
snd_pcm_format_width() returns an error code on error, LPASS_INVALID is
not an error code. Check the return value for error codes...
> + dev_err(dai->dev, "%s: Invalid bit width given\n", __func__);
> + return -EINVAL;
...and just return them.
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + /* disable SPKR to make sure it will start in sane state */
> + lpaif_cfg_i2s_playback(0, 0, LPAIF_MI2S);
Shouldn't we be doing this on probe and/or resume if it's required?
> +
> + /*
> + * Set channel info, it will take effect only if SPKR is
> + * enabled
> + */
> + ret = lpaif_cfg_mi2s_playback_hwparams_channels(channels,
> + LPAIF_MI2S, bit_act);
> + } else {
> + dev_err(dai->dev, "%s: Invalid stream direction\n", __func__);
> + return -EINVAL;
> + }
If the device only supports playback no need to have any conditional
code here, the core will prevent capture being started.
> + ret = clk_set_rate(lpaif_mi2s_osr_clk,
> + (rate * bit_act * channels * bit_div));
> + if (ret) {
> + dev_err(dai->dev, "%s: error in setting mi2s osr clk\n",
> + __func__);
> + return ret;
> + }
> + ret = clk_prepare_enable(lpaif_mi2s_osr_clk);
> + if (ret) {
> + dev_err(dai->dev, "%s: error in enabling mi2s osr clk\n",
> + __func__);
> + return ret;
> + }
> + prtd->lpaif_clk.is_osr_clk_enabled = 1;
Coding style, more blank lines between blocks here. Also not clear why
we're tracking if the clock is enabled when we do it unconditonally.
> +static int lpass_cpu_mi2s_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + return 0;
> +}
Remove empty functions.
> +static int lpass_cpu_mi2s_startup(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> +
> + lpaif_mi2s_osr_clk = clk_get(dai->dev, "mi2s_osr_clk");
> + if (IS_ERR(lpaif_mi2s_osr_clk)) {
> + dev_err(dai->dev, "%s: Error in getting mi2s_osr_clk\n",
> + __func__);
> + return PTR_ERR(lpaif_mi2s_osr_clk);
> + }
No, request resources in probe(). That way deferred probe works and we
don't get mysterious errors at runtime if things go wrong.
Attachment:
signature.asc
Description: Digital signature
