Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1408090 > unrolled thread

[RFC v2 0/3] ASoC: Add support to Qualcomm msm8916-wcd codec

Started bySrinivas Kandagatla <srinivas.kandagatla@linaro.org>
First post2016-05-27 15:50 +0200
Last post2016-05-31 11:00 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [RFC v2 0/3] ASoC: Add support to Qualcomm msm8916-wcd codec Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-05-27 15:50 +0200
    [RFC v2 2/3] ASoC: Introduce SOC_SINGLE_S8_TLV() macro Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-05-27 15:50 +0200
      Applied "ASoC: Introduce SOC_SINGLE_S8_TLV() macro" to the asoc tree Mark Brown <broonie@kernel.org> - 2016-05-27 22:50 +0200
    Re: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver Kenneth Westfield <kwestfie@codeaurora.org> - 2016-05-31 01:10 +0200
      Re: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-05-31 11:00 +0200

#1408090 — [RFC v2 0/3] ASoC: Add support to Qualcomm msm8916-wcd codec

FromSrinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date2016-05-27 15:50 +0200
Subject[RFC v2 0/3] ASoC: Add support to Qualcomm msm8916-wcd codec
Message-ID<rDqjg-45S-13@gated-at.bofh.it>
This patchset aims at adding msm8916-wcd codec support.
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 series also add a new macro SOC_SINGLE_S8_TLV() which is used in
the codec driver to control gain on a 8 bit signed gain control register.

Most of this driver is ported from downstream Andriod v3.10 kernel.
Jack support and Headset button detection code is not added in to this series.

This codec support:
 -  3 Microphones: Primary Mic(Handset mic), Headset Mic and Secondary Mic.
 - 2 Digital Microphones.
 - 2 Mic Bias Circuits.
 - Earpiece
 - Headset
 - Loud Speaker.
 - Jack Detect.
 - Headset Button detection circuit.

Tested this code on v4.6 with lpass driver Headset (Mic and speakers), Secondary Mic
and Loudspeaker on DB410C. DMIC should work too.

Thanks,
srini

Changes since RFC v1 (https://lkml.org/lkml/2016/2/16/710)
- Lot of code cleanup as suggest by Mark Brown.
- Fixed bindings as suggested by Mark Rutland.
- merged all the patches to one as spliting the patches did not help review.

Srinivas Kandagatla (3):
  ASoC: msm8916: Add codec Device Tree bindings.
  ASoC: Introduce SOC_SINGLE_S8_TLV() macro
  ASoC: msm8916: Add msm8916-wcd codec driver

 .../devicetree/bindings/sound/qcom,msm8916-wcd.txt |  101 ++
 include/dt-bindings/sound/msm8916-wcd.h            |    7 +
 include/sound/soc.h                                |   11 +
 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 ++++
 8 files changed, 2725 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,msm8916-wcd.txt
 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

--
2.8.2

[toc] | [next] | [standalone]

#1408093 — [RFC v2 2/3] ASoC: Introduce SOC_SINGLE_S8_TLV() macro

FromSrinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date2016-05-27 15:50 +0200
Subject[RFC v2 2/3] ASoC: Introduce SOC_SINGLE_S8_TLV() macro
Message-ID<rDqjh-45S-37@gated-at.bofh.it>
In reply to#1408090
This patch introduces SOC_SINGLE_S8_TLV() macro for volume control
on chips which supports both negative and positive gains with sign
bit on a 8 bit register, Gain ranges from -128 to +127 with a
predefined step size.
Currently we only have support to DOUBLE_S8_TLV() which does not fit
for cases where we just have separate gain control register for each
channel.

One of the Qualcomm SOC msm8916 has such gain control register whose gain
range is from -38.4dB to +38.4dB with step size of 0.3dB.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/sound/soc.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 02b4a21..b23fc50 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -179,6 +179,17 @@
 	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_R_S_VALUE(reg_left, reg_right, xshift, \
 					    xmin, xmax, xsign_bit, xinvert) }
+#define SOC_SINGLE_S8_TLV(xname, xreg, xmin, xmax, tlv_array) \
+{	.iface  = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
+		  SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.tlv.p  = (tlv_array), \
+	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw,\
+	.put = snd_soc_put_volsw, \
+	.private_value = (unsigned long)&(struct soc_mixer_control) \
+	{.reg = xreg, .rreg = xreg,  \
+	 .min = xmin, .max = xmax, .platform_max = xmax, \
+	.sign_bit = 7,} }
 #define SOC_DOUBLE_S8_TLV(xname, xreg, xmin, xmax, tlv_array) \
 {	.iface  = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
-- 
2.8.2

[toc] | [prev] | [next] | [standalone]

#1408306 — Applied "ASoC: Introduce SOC_SINGLE_S8_TLV() macro" to the asoc tree

FromMark Brown <broonie@kernel.org>
Date2016-05-27 22:50 +0200
SubjectApplied "ASoC: Introduce SOC_SINGLE_S8_TLV() macro" to the asoc tree
Message-ID<rDwRI-8aV-11@gated-at.bofh.it>
In reply to#1408093
The patch

   ASoC: Introduce SOC_SINGLE_S8_TLV() macro

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 627f3c2fc0c8fd52ebe7c0f28744d98d3955a1d6 Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Fri, 27 May 2016 14:45:45 +0100
Subject: [PATCH] ASoC: Introduce SOC_SINGLE_S8_TLV() macro

This patch introduces SOC_SINGLE_S8_TLV() macro for volume control
on chips which supports both negative and positive gains with sign
bit on a 8 bit register, Gain ranges from -128 to +127 with a
predefined step size.
Currently we only have support to DOUBLE_S8_TLV() which does not fit
for cases where we just have separate gain control register for each
channel.

One of the Qualcomm SOC msm8916 has such gain control register whose gain
range is from -38.4dB to +38.4dB with step size of 0.3dB.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/soc.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index fd7b58a58d6f..6144882cc96a 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -179,6 +179,17 @@
 	.get = snd_soc_get_volsw, .put = snd_soc_put_volsw, \
 	.private_value = SOC_DOUBLE_R_S_VALUE(reg_left, reg_right, xshift, \
 					    xmin, xmax, xsign_bit, xinvert) }
+#define SOC_SINGLE_S8_TLV(xname, xreg, xmin, xmax, tlv_array) \
+{	.iface  = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
+		  SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.tlv.p  = (tlv_array), \
+	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw,\
+	.put = snd_soc_put_volsw, \
+	.private_value = (unsigned long)&(struct soc_mixer_control) \
+	{.reg = xreg, .rreg = xreg,  \
+	 .min = xmin, .max = xmax, .platform_max = xmax, \
+	.sign_bit = 7,} }
 #define SOC_DOUBLE_S8_TLV(xname, xreg, xmin, xmax, tlv_array) \
 {	.iface  = SNDRV_CTL_ELEM_IFACE_MIXER, .name = (xname), \
 	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1409747 — Re: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver

FromKenneth Westfield <kwestfie@codeaurora.org>
Date2016-05-31 01:10 +0200
SubjectRe: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver
Message-ID<rEEtP-2wt-1@gated-at.bofh.it>
In reply to#1408090
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@linaro.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

[toc] | [prev] | [next] | [standalone]

#1410008 — Re: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver

FromSrinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date2016-05-31 11:00 +0200
SubjectRe: [RFC v2 3/3] ASoC: msm8916: Add msm8916-wcd codec driver
Message-ID<rENGO-pS-17@gated-at.bofh.it>
In reply to#1409747
Thanks Kenneth for Review,

On 31/05/16 00:01, Kenneth Westfield wrote:
> 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@linaro.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.
>

Thanks, I will fix that in next version.
>> +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?
>
No reason, I can make them static const i guess.
>> +
>> +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.

Ok,
>
>> +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.
>
Ok, I will do that .

>> +	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.
>
Will give it a try and see how it looks.

>> +{
>> +	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.
Good point, will do that.
>
>> +		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?
Yes, we should I guess. I will fix it too in next version.
>
>> +	}
>> +
>> +	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...
Yep, Will fix this too.
>
>> +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?
Thanks, I will have a relook at the error handling in the probe.

>
--srini

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web