On Wed, Nov 19, 2014 at 07:52:44PM +0100, Kenneth Westfield wrote:
> From: Kenneth Westfield <kwestfie@xxxxxxxxxxxxxx>
>
> Add the CPU DAI driver for the QCOM LPASS SOC.
>
> Change-Id: I64ac4407dd32bb9a3066d4b7427292002eaf5d14
> Signed-off-by: Kenneth Westfield <kwestfie@xxxxxxxxxxxxxx>
> Signed-off-by: Banajit Goswami <bgoswami@xxxxxxxxxxxxxx>
> ---
[...]
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <sound/soc.h>
> +#include <sound/pcm_params.h>
> +#include "lpass-lpaif.h"
> +#include "lpass-pcm-mi2s.h"
This header and the associated structures are not added until 5/9:
"ASoC: ipq806x: Add I2S PCM platform driver"...
> +
> +#define DRV_NAME "lpass-cpu-dai"
> +#define DRV_VERSION "1.0"
> +
> +#define LPASS_INVALID (-1)
> +
> +struct mi2s_hw_params {
> + uint8_t channels;
> + uint32_t freq;
> + uint8_t bit_width;
> +};
This struct, the static global instance of it below ('mi2s_params'), and
the additional use of it in lpass_cpu_mi2s_hw_params() ('curr_params')
are only ever written, never read.
> +
> +static struct clk *lpaif_mi2s_bit_clk;
> +static struct clk *lpaif_mi2s_osr_clk;
It would seem more logical to me to put these in allocated private driver
data for the DAI, managed from (struct snd_soc_dai_driver).probe/remove.
> +static struct mi2s_hw_params mi2s_params;
[...]
> +static int lpass_cpu_mi2s_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{
[...]
> + 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;
> +
> + ret = clk_set_rate(lpaif_mi2s_bit_clk, rate * bit_act * channels);
> + if (ret) {
> + dev_err(dai->dev, "%s: error in setting mi2s bit clk\n",
> + __func__);
> + return ret;
clk_disable_unprepare(lpaif_mi2s_osr_clk)?
> + }
> + ret = clk_prepare_enable(lpaif_mi2s_bit_clk);
> + if (ret) {
> + dev_err(dai->dev, "%s: error in enabling mi2s bit clk\n",
> + __func__);
> + return ret;
clk_disable_unprepare(lpaif_mi2s_bit_clk)?
clk_disable_unprepare(lpaif_mi2s_osr_clk)?
> + }
> + prtd->lpaif_clk.is_bit_clk_enabled = 1;
> +
> + return 0;
> +}
> +
> +static int lpass_cpu_mi2s_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + return 0;
> +}
Isn't this ((struct snd_soc_dai_ops).prepare) optional?
> +
> +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);
> + }
> +
> + lpaif_mi2s_bit_clk = clk_get(dai->dev, "mi2s_bit_clk");
> + if (IS_ERR(lpaif_mi2s_bit_clk)) {
> + dev_err(dai->dev, "%s: Error in getting mi2s_bit_clk\n",
> + __func__);
> + return PTR_ERR(lpaif_mi2s_bit_clk);
clk_put(lpaif_mi2s_osr_clk)?
> + }
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> + lpaif_cfg_i2s_playback(0, 0, LPAIF_MI2S);
> + } else {
> + dev_err(dai->dev, "%s: Invalid stream direction\n", __func__);
> + return -EINVAL;
clk_put(lpaif_mi2s_bit_clk)?
clk_put(lpaif_mi2s_osr_clk)?
> + }
> +
> + return 0;
> +}
> +
> +static void lpass_cpu_mi2s_shutdown(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct lpass_runtime_data_t *prtd = runtime->private_data;
> +
> + if (prtd->lpaif_clk.is_osr_clk_enabled)
> + clk_disable_unprepare(lpaif_mi2s_osr_clk);
This behavior is a bit odd. If you clk_prepare_enable() the clocks in
.hw_params, shouldn't you clk_disable_unprepare() in .hw_free? Then you
wouldn't need these booleans, or the associated lpaif_clk struct.
> + clk_put(lpaif_mi2s_osr_clk);
> +
> + if (prtd->lpaif_clk.is_bit_clk_enabled)
> + clk_disable_unprepare(lpaif_mi2s_bit_clk);
> + clk_put(lpaif_mi2s_bit_clk);
> +}
-Courtney
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
