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.