Message-ID: <20160530230133.GB4004@kwestfie-linux.qualcomm.com>
Date:	Mon, 30 May 2016 16:01:33 -0700
From:	Kenneth Westfield <kwestfie@...eaurora.org>
To:	Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:	Mark Brown <broonie@...nel.org>, alsa-devel@...a-project.org,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, kwestfie@...eaurora.org,
	plai@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver

On Fri, May 27, 2016 at 02:45:46PM +0100, Srinivas Kandagatla wrote:
> This patch adds support to msm8916-wcd codec.
> 
> msm8916-wcd codec is found in Qualcomm msm8916 and apq8016 processors.
> This codec IP is split in to two parts(Digital & Analog), Analog part
> is integrated in to PMIC PM8916 and the digital part is integrated into
> Application processor. Register access to the analog part is done via
> SPMI interface to PMIC, and registers on the Application processor are
> memory mapped. Data transfer between Analog and Digital Die is done via
> a internal bus called PDM.
> 
> This codec support includes:
>  - 3 Microphones: Primary Mic(Handset mic), Headset Mic and Secondary Mic.
>  - 2 Digital Microphones.
>  - 2 Mic Bias Circuits.
>  - Earpiece
>  - Headset
>  - Loud Speaker.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
> ---
>  include/dt-bindings/sound/msm8916-wcd.h  |    7 +
>  sound/soc/codecs/Kconfig                 |    4 +
>  sound/soc/codecs/Makefile                |    3 +-
>  sound/soc/codecs/msm8916-wcd-registers.h |  710 ++++++++++++++
>  sound/soc/codecs/msm8916-wcd.c           | 1582 ++++++++++++++++++++++++++++++
>  sound/soc/codecs/msm8916-wcd.h           |  308 ++++++
>  6 files changed, 2613 insertions(+), 1 deletion(-)
>  create mode 100644 include/dt-bindings/sound/msm8916-wcd.h
>  create mode 100644 sound/soc/codecs/msm8916-wcd-registers.h
>  create mode 100644 sound/soc/codecs/msm8916-wcd.c
>  create mode 100644 sound/soc/codecs/msm8916-wcd.h
 
> diff --git a/sound/soc/codecs/msm8916-wcd.c b/sound/soc/codecs/msm8916-wcd.c
> new file mode 100644
> index 0000000..fb4d0ad
> --- /dev/null
> +++ b/sound/soc/codecs/msm8916-wcd.c

> +#define MSM8916_WCD_RATES (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |\
> +			SNDRV_PCM_RATE_32000 | SNDRV_PCM_RATE_48000)

The rates listed here don't match the rates supported in the DAIs
defined later.

> +static unsigned long rx_gain_reg[] = {
> +	LPASS_CDC_RX1_VOL_CTL_B2_CTL,
> +	LPASS_CDC_RX2_VOL_CTL_B2_CTL,
> +	LPASS_CDC_RX3_VOL_CTL_B2_CTL,
> +};

Is there a reason this struct (and others that follow) don't have static
const qualifiers?

> +
> +static unsigned long tx_gain_reg[] = {
> +	LPASS_CDC_TX1_VOL_CTL_GAIN,
> +	LPASS_CDC_TX2_VOL_CTL_GAIN,
> +};

ditto.

> +static int msm8916_wcd_codec_enable_dec(struct snd_soc_dapm_widget *w,
> +					struct snd_kcontrol *kcontrol,
> +					int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +	unsigned int decimator;
> +	char *dec_name = NULL;
> +	char *widget_name = NULL;
> +	char *temp;
> +	int ret = 0;
> +	u16 dec_reset_reg, tx_vol_ctl_reg, tx_mux_ctl_reg;
> +	u8 dec_hpf_cut_of_freq;
> +	char *dec_num;
> +
> +	widget_name = kstrndup(w->name, 15, GFP_KERNEL);
> +	if (!widget_name)
> +		return -ENOMEM;
> +	temp = widget_name;
> +
> +	dec_name = strsep(&widget_name, " ");
> +	widget_name = temp;
> +	if (!dec_name) {
> +		dev_err(codec->dev, "Invalid decimator = %s\n", w->name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	dec_num = strpbrk(dec_name, "12");
> +	if (dec_num == NULL) {
> +		dev_err(codec->dev, "Invalid Decimator\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	ret = kstrtouint(dec_num, 10, &decimator);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Invalid decimator = %s\n", dec_name);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	dec_reset_reg = LPASS_CDC_CLK_TX_RESET_B1_CTL;
> +	tx_vol_ctl_reg = LPASS_CDC_TX1_VOL_CTL_CFG + 32 * (decimator - 1);
> +	tx_mux_ctl_reg = LPASS_CDC_TX1_MUX_CTL + 32 * (decimator - 1);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		/* Enableable TX digital mute */

typo.

> +static int msm8916_wcd_codec_parse_dt(struct platform_device *pdev,
> +				      struct msm8916_wcd_chip *chip)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +	struct regulator_bulk_data regs[2];
> +	const char *ext1_cap = "qcom,micbias1-ext-cap";
> +	const char *ext2_cap = "qcom,micbias2-ext-cap";

IMO, Since these strings are used just once below, simply use the string
literals.

> +	u32 res[2];
> +
> +	ret = of_property_read_u32_array(np, "reg", res, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (of_property_read_bool(pdev->dev.of_node, ext1_cap))
> +		chip->micbias1_cap_mode = MICB_1_EN_EXT_BYP_CAP;
> +	else
> +		chip->micbias1_cap_mode = MICB_1_EN_NO_EXT_BYP_CAP;
> +
> +	if (of_property_read_bool(pdev->dev.of_node, ext2_cap))
> +		chip->micbias2_cap_mode = MICB_1_EN_EXT_BYP_CAP;
> +	else
> +		chip->micbias2_cap_mode = MICB_1_EN_NO_EXT_BYP_CAP;
> +
> +	chip->analog_offset = res[0];
> +	chip->digital_map = syscon_regmap_lookup_by_phandle(np,
> +							    "qcom,lpass-codec-core");
> +	if (IS_ERR(chip->digital_map))
> +		return PTR_ERR(chip->digital_map);
> +
> +	chip->mclk = devm_clk_get(dev, "mclk");
> +	if (IS_ERR(chip->mclk)) {
> +		dev_err(dev, "failed to get mclk\n");
> +		return PTR_ERR(chip->mclk);
> +	}
> +
> +	regs[0].supply = "vddio";
> +	regs[1].supply = "vdd-tx-rx";
> +
> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(regs), regs);
> +	if (ret) {
> +		dev_err(dev, "Failed to get regulator supplies %d\n", ret);
> +		return ret;
> +	}
> +	chip->vddio = regs[0].consumer;
> +	chip->vdd_tx_rx = regs[1].consumer;
> +
> +	return 0;
> +}

> +static int msm8916_wcd_device_up(struct snd_soc_codec *codec)

Since this is a small code block that is only called once,
just embed it into the codec_probe function.

> +{
> +	struct msm8916_wcd_chip *msm8916_wcd = snd_soc_codec_get_drvdata(codec);
> +	u32 reg;
> +
> +	snd_soc_write(codec, CDC_D_PERPH_RESET_CTL4, 0x01);
> +	snd_soc_write(codec, CDC_A_PERPH_RESET_CTL4, 0x01);
> +
> +	for (reg = 0; reg < ARRAY_SIZE(msm8916_wcd_reg_init_val); reg++)
> +		snd_soc_update_bits(codec,
> +				    msm8916_wcd_reg_init_val[reg].reg,
> +				    msm8916_wcd_reg_init_val[reg].mask,
> +				    msm8916_wcd_reg_init_val[reg].val);
> +
> +	if (TOMBAK_IS_1_0(msm8916_wcd->pmic_rev)) {
> +		for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults); reg++)
> +			snd_soc_write(codec, wcd_reg_defaults[reg].reg,
> +				      wcd_reg_defaults[reg].val);
> +	} else {
> +		for (reg = 0; reg < ARRAY_SIZE(wcd_reg_defaults_2_0); reg++)
> +			snd_soc_write(codec, wcd_reg_defaults_2_0[reg].reg,
> +				      wcd_reg_defaults_2_0[reg].val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int msm8916_wcd_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
> +	int err;
> +
> +	err = regulator_enable(chip->vddio);
> +	if (err < 0) {
> +		dev_err(codec->dev, "failed to enable VDDIO regulator\n");

Print the error value here, and any other places where it is missing too.

> +		return err;
> +	}
> +
> +	err = regulator_enable(chip->vdd_tx_rx);
> +	if (err < 0) {
> +		dev_err(codec->dev, "failed to enable VDD_TX_RX regulator\n");
> +		return err;

Does the vddio regulator need to be disabled on error?  or it this
handled elsewhere?

> +	}
> +
> +	snd_soc_codec_set_drvdata(codec, chip);
> +	chip->pmic_rev = snd_soc_read(codec, CDC_D_REVISION1);
> +	chip->codec_version = snd_soc_read(codec, CDC_D_PERPH_SUBTYPE);
> +	dev_info(codec->dev, "PMIC REV: %d\t CODEC Version: %d\n",
> +		 chip->pmic_rev, chip->codec_version);
> +
> +	msm8916_wcd_device_up(codec);
> +	/* Set initial cap mode */
> +	msm8916_wcd_configure_cap(codec, false, false);
> +
> +	return 0;
> +}
> +
> +static int msm8916_wcd_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	u8 tx_fs_rate;
> +
> +	switch (params_rate(params)) {
> +	case 8000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_8_KHZ;
> +		break;
> +	case 16000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_16_KHZ;
> +		break;
> +	case 32000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_32_KHZ;
> +		break;
> +	case 48000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_48_KHZ;
> +		break;
> +	case 96000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_96_KHZ;
> +		break;
> +	case 192000:
> +		tx_fs_rate = TX_I2S_CTL_TX_I2S_FS_RATE_F_192_KHZ;
> +		break;

Supported rates need to match (mentioned above).

> +static const struct snd_soc_dapm_route audio_map[] = {

Please use consistent name prefixes for variables
(msm8916_wcd_audio_map).

> +static struct snd_soc_dai_driver msm8916_wcd_codec_dai[] = {
> +	[0] = {
> +	       .name = "msm8916_wcd_i2s_rx1",
> +	       .id = MSM8916_WCD_PLAYBACK_DAI,
> +	       .playback = {
> +			    .stream_name = "AIF1 Playback",
> +			    .rates = MSM8916_WCD_RATES,
> +			    .formats = MSM8916_WCD_FORMATS,
> +			    .rate_max = 192000,

Supported rates...

> +static int msm8916_wcd_probe(struct platform_device *pdev)
> +{
> +	struct msm8916_wcd_chip *chip;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +	if (!chip)
> +		return -ENOMEM;
> +
> +	chip->analog_map = dev_get_regmap(dev->parent, NULL);
> +	if (!chip->analog_map)
> +		return -ENXIO;

Does the analog_map need to be deallocated on later errors in this
function?  or is dev_get_regmap a devm_ type function?

-- 
Kenneth Westfield
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, 
a Linux Foundation Collaborative Project

Powered by blists - more mailing lists