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

Groups > linux.kernel > #1419658 > unrolled thread

[PATCH v3 0/2] ASoC: Add support to Qualcomm msm8916-wcd codec

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/2] ASoC: Add support to Qualcomm msm8916-wcd codec Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-06-10 20:20 +0200
    Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver Mark Brown <broonie@kernel.org> - 2016-06-14 18:10 +0200
      Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-06-15 11:20 +0200
        Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver Mark Brown <broonie@kernel.org> - 2016-06-15 11:40 +0200
          Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver Kenneth Westfield <kwestfie@codeaurora.org> - 2016-06-15 22:10 +0200
            Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2016-06-16 15:20 +0200

#1419658 — [PATCH v3 0/2] ASoC: Add support to Qualcomm msm8916-wcd codec

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.

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-rc2 with lpass driver capture support patches for
Headset (Mic and speakers), Secondary Mic and Loudspeaker on DB410C.
DMIC should work too.

Thanks,
srini

Changes since RFC v2:
 - Fixed error handling on regluator_enable suggested by Kenneth.
 - Fixed compatible strings suggested by Kenneth
 - make max/min rates inline with actual supported rates,
 	spotted by Kenneth
 - Few minor suggestions by Kenneth.

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 (2):
  ASoC: msm8916: Add codec Device Tree bindings.
  ASoC: msm8916: Add msm8916-wcd codec driver

 .../devicetree/bindings/sound/qcom,msm8916-wcd.txt |  103 ++
 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                     | 1575 ++++++++++++++++++++
 sound/soc/codecs/msm8916-wcd.h                     |  308 ++++
 7 files changed, 2709 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.3

[toc] | [next] | [standalone]

#1422049 — Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:

> +config SND_SOC_MSM8916_WCD
> +	tristate "Qualcomm MSM8916 WCD"
> +	depends on SPMI && MFD_SYSCON
> +

Normally users select MFD_SYSCON.

> @@ -208,7 +209,6 @@ snd-soc-wm9705-objs := wm9705.o
>  snd-soc-wm9712-objs := wm9712.o
>  snd-soc-wm9713-objs := wm9713.o
>  snd-soc-wm-hubs-objs := wm_hubs.o
> -
>  # Amp
>  snd-soc-max9877-objs := max9877.o
>  snd-soc-tpa6130a2-objs := tpa6130a2.o

Spurious whitespace change.

> +#include "msm8916-wcd-registers.h"
> +#include "msm8916-wcd.h"
> +#include "dt-bindings/sound/msm8916-wcd.h"

What's in here?  There weren't any constants in the bindings.

> +struct msm8916_wcd_chip {
> +	struct regmap *analog_map;
> +	struct regmap *digital_map;
> +	unsigned int analog_offset;
> +	u16 pmic_rev;
> +	u16 codec_version;

Why is this one device and not two devices?  The description indicated
that this was two separate bits of silicon.

> +static int msm8916_wcd_write(struct snd_soc_codec *codec, unsigned int reg,
> +			     unsigned int val)
> +{
> +	int ret = -EINVAL;
> +	struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
> +	u8 *cache = codec->reg_cache;
> +
> +	if (!msm8916_wcd_reg_readonly[reg])
> +		cache[reg] = val;

Why is the driver open coding a cache?  Don't do that!

> +	case SND_SOC_DAPM_POST_PMU:
> +		if (w->shift == 5)
> +			snd_soc_update_bits(codec, LPASS_CDC_RX1_B6_CTL,
> +					    RXn_B6_CTL_MUTE_MASK, 0);
> +		else if (w->shift == 4)
> +			snd_soc_update_bits(codec, LPASS_CDC_RX2_B6_CTL,
> +					    RXn_B6_CTL_MUTE_MASK, 0);

Switch statement.

> +	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;
> +	}

I'm not terribly clear what this is doing, it probably needs some
comments explaining what's going on at the very least.

> +	/*RX stuff */
> +	SND_SOC_DAPM_AIF_IN("I2S RX1", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("I2S RX2", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("I2S RX3", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),

Use DAPM routes to connect the widgets in, don't name the DAI in the
widget.

> +		mclk_rate = clk_get_rate(msm8916_wcd->mclk);
> +
> +		if (mclk_rate == 12288000)
> +			snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
> +					    TOP_CTL_DIG_MCLK_FREQ_MASK,
> +					    TOP_CTL_DIG_MCLK_FREQ_F_12_288MHZ);
> +
> +		else if (mclk_rate == 9600000)
> +			snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
> +					    TOP_CTL_DIG_MCLK_FREQ_MASK,
> +					    TOP_CTL_DIG_MCLK_FREQ_F_9_6MHZ);

Switch statement, and this should also handle unexpected rates.

> +static int msm8916_wcd_codec_probe(struct snd_soc_codec *codec)
> +{
> +	struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
> +	int err, reg;
> +
> +	err = regulator_enable(chip->vddio);
> +	if (err < 0) {
> +		dev_err(codec->dev,
> +			"failed to enable VDDIO regulator (%d)\n", err);
> +		return err;
> +	}
> +
> +	err = regulator_enable(chip->vdd_tx_rx);
> +	if (err < 0) {
> +		dev_err(codec->dev,
> +			"failed to enable VDD_TX_RX regulator (%d)\n", err);
> +		regulator_disable(chip->vddio);
> +		return err;
> +	}

Why is this not using regulator_bulk_enable()?  I'd also expect to see
most if not all of this initial setup stuff in the main device probe.

> +	if (TOMBAK_IS_1_0(chip->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);
> +	}

Please reset the chip properly.

> +	ret = clk_prepare_enable(chip->mclk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable mclk %d\n", ret);
> +		return ret;
> +	}

Runtime PM?

> +static const struct of_device_id msm8916_wcd_match_table[] = {
> +	{.compatible = "qcom,msm8916-pmic-wcd-codec"},
> +	{}
> +};

We were peering inside the parent for the register map, why does this
appear in the device tree as a separate device?  Both the patch
description and that code suggest that it doesn't really have a separate
existence independent of the broader IP.

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

#1422813 — Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver

Thanks for review comments,

On 14/06/16 16:59, Mark Brown wrote:
> On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:
>
>> +config SND_SOC_MSM8916_WCD
>> +	tristate "Qualcomm MSM8916 WCD"
>> +	depends on SPMI && MFD_SYSCON
>> +
>
> Normally users select MFD_SYSCON.
>
This driver is child of spmi bus so, we need SPMI dependency here along 
with SYSCON.

>> @@ -208,7 +209,6 @@ snd-soc-wm9705-objs := wm9705.o
>>   snd-soc-wm9712-objs := wm9712.o
>>   snd-soc-wm9713-objs := wm9713.o
>>   snd-soc-wm-hubs-objs := wm_hubs.o
>> -
>>   # Amp
>>   snd-soc-max9877-objs := max9877.o
>>   snd-soc-tpa6130a2-objs := tpa6130a2.o
>
> Spurious whitespace change.
Yep will fix it.
>
>> +#include "msm8916-wcd-registers.h"
>> +#include "msm8916-wcd.h"
>> +#include "dt-bindings/sound/msm8916-wcd.h"
>
> What's in here?  There weren't any constants in the bindings.
>
Yes, there are DAI id's which are used in device trees.

>> +struct msm8916_wcd_chip {
>> +	struct regmap *analog_map;
>> +	struct regmap *digital_map;
>> +	unsigned int analog_offset;
>> +	u16 pmic_rev;
>> +	u16 codec_version;
>
> Why is this one device and not two devices?  The description indicated
> that this was two separate bits of silicon.

In theory there are 3 devices,
one is the pmic-spmi driver, which provides regmap access to analog part 
of codec registers.
second is syscon driver which provides regmap access to digital parts of 
codec to codec driver.
third is the codec driver which uses both the above.

Codec registers range is just split into two, range 0x0- 0x200 sits in 
pmic address space and range 0x201 - 0x4ff in the SOC address space,

Are there any other better ways to model this kinda driver?

>
>> +static int msm8916_wcd_write(struct snd_soc_codec *codec, unsigned int reg,
>> +			     unsigned int val)
>> +{
>> +	int ret = -EINVAL;
>> +	struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
>> +	u8 *cache = codec->reg_cache;
>> +
>> +	if (!msm8916_wcd_reg_readonly[reg])
>> +		cache[reg] = val;
>
> Why is the driver open coding a cache?  Don't do that!
>
Yep Will remove it. I guess this is already done in the core..
>> +	case SND_SOC_DAPM_POST_PMU:
>> +		if (w->shift == 5)
>> +			snd_soc_update_bits(codec, LPASS_CDC_RX1_B6_CTL,
>> +					    RXn_B6_CTL_MUTE_MASK, 0);
>> +		else if (w->shift == 4)
>> +			snd_soc_update_bits(codec, LPASS_CDC_RX2_B6_CTL,
>> +					    RXn_B6_CTL_MUTE_MASK, 0);
>
> Switch statement.
>
>> +	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;
>> +	}
>
> I'm not terribly clear what this is doing, it probably needs some
> comments explaining what's going on at the very least.
I will make sure that I comment it properly in next version.

>
>> +	/*RX stuff */
>> +	SND_SOC_DAPM_AIF_IN("I2S RX1", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
>> +	SND_SOC_DAPM_AIF_IN("I2S RX2", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
>> +	SND_SOC_DAPM_AIF_IN("I2S RX3", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
>
> Use DAPM routes to connect the widgets in, don't name the DAI in the
> widget.
Yep, I will relook at this.
>
>> +		mclk_rate = clk_get_rate(msm8916_wcd->mclk);
>> +
>> +		if (mclk_rate == 12288000)
>> +			snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
>> +					    TOP_CTL_DIG_MCLK_FREQ_MASK,
>> +					    TOP_CTL_DIG_MCLK_FREQ_F_12_288MHZ);
>> +
>> +		else if (mclk_rate == 9600000)
>> +			snd_soc_update_bits(codec, LPASS_CDC_TOP_CTL,
>> +					    TOP_CTL_DIG_MCLK_FREQ_MASK,
>> +					    TOP_CTL_DIG_MCLK_FREQ_F_9_6MHZ);
>
> Switch statement, and this should also handle unexpected rates.
Yep, sounds good, Will fix it in next version.

>
>> +static int msm8916_wcd_codec_probe(struct snd_soc_codec *codec)
>> +{
>> +	struct msm8916_wcd_chip *chip = dev_get_drvdata(codec->dev);
>> +	int err, reg;
>> +
>> +	err = regulator_enable(chip->vddio);
>> +	if (err < 0) {
>> +		dev_err(codec->dev,
>> +			"failed to enable VDDIO regulator (%d)\n", err);
>> +		return err;
>> +	}
>> +
>> +	err = regulator_enable(chip->vdd_tx_rx);
>> +	if (err < 0) {
>> +		dev_err(codec->dev,
>> +			"failed to enable VDD_TX_RX regulator (%d)\n", err);
>> +		regulator_disable(chip->vddio);
>> +		return err;
>> +	}
>
> Why is this not using regulator_bulk_enable()?  I'd also expect to see
> most if not all of this initial setup stuff in the main device probe.
Yep, we can move to using bulk* apis.
>
>> +	if (TOMBAK_IS_1_0(chip->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);
>> +	}
>
> Please reset the chip properly.
Yep. I will re-order the
>
>> +	ret = clk_prepare_enable(chip->mclk);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to enable mclk %d\n", ret);
>> +		return ret;
>> +	}
>
> Runtime PM?

I will re-look at runtime pm stuff before I send the next version.
>
>> +static const struct of_device_id msm8916_wcd_match_table[] = {
>> +	{.compatible = "qcom,msm8916-pmic-wcd-codec"},
>> +	{}
>> +};
>
> We were peering inside the parent for the register map, why does this

I think that's the only way/interface to access PMIC spmi registers I guess.
> appear in the device tree as a separate device?  Both the patch

This node is child of spmi bus, like the other spmi devices.

> description and that code suggest that it doesn't really have a separate
> existence independent of the broader IP.
>
Yes, the code is written in a way that there is no separate existence 
hiding the register map split in the read/write wrappers.

thanks,
srini

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

#1422828 — Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jun 15, 2016 at 10:16:27AM +0100, Srinivas Kandagatla wrote:
> On 14/06/16 16:59, Mark Brown wrote:
> > On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:

> > > +config SND_SOC_MSM8916_WCD
> > > +	tristate "Qualcomm MSM8916 WCD"
> > > +	depends on SPMI && MFD_SYSCON

> > Normally users select MFD_SYSCON.

> This driver is child of spmi bus so, we need SPMI dependency here along with
> SYSCON.

That does not seem relevant to the problem with depending on MFD_SYSCON.

> > > +#include "msm8916-wcd-registers.h"
> > > +#include "msm8916-wcd.h"
> > > +#include "dt-bindings/sound/msm8916-wcd.h"

> > What's in here?  There weren't any constants in the bindings.

> Yes, there are DAI id's which are used in device trees.

That doesn't make them present in the binding document...

> > Why is this one device and not two devices?  The description indicated
> > that this was two separate bits of silicon.

> In theory there are 3 devices,
> one is the pmic-spmi driver, which provides regmap access to analog part of
> codec registers.
> second is syscon driver which provides regmap access to digital parts of
> codec to codec driver.
> third is the codec driver which uses both the above.

> Codec registers range is just split into two, range 0x0- 0x200 sits in pmic
> address space and range 0x201 - 0x4ff in the SOC address space,

> Are there any other better ways to model this kinda driver?

Why not just have separate devices for each of the register maps?

> > > +static const struct of_device_id msm8916_wcd_match_table[] = {
> > > +	{.compatible = "qcom,msm8916-pmic-wcd-codec"},
> > > +	{}
> > > +};

> > We were peering inside the parent for the register map, why does this

> I think that's the only way/interface to access PMIC spmi registers I guess.

Don't guess, understand what the code is doing.

> > appear in the device tree as a separate device?  Both the patch

> This node is child of spmi bus, like the other spmi devices.

If this is a SPMI device it needs to register a SPMI device not a
platform device.

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

#1423365 — Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver

On Wed, Jun 15, 2016 at 10:31:36AM +0100, Mark Brown wrote:
> On Wed, Jun 15, 2016 at 10:16:27AM +0100, Srinivas Kandagatla wrote:
> > On 14/06/16 16:59, Mark Brown wrote:
> > > On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:

> > In theory there are 3 devices,
> > one is the pmic-spmi driver, which provides regmap access to analog part of
> > codec registers.
> > second is syscon driver which provides regmap access to digital parts of
> > codec to codec driver.
> > third is the codec driver which uses both the above.
> 
> > Codec registers range is just split into two, range 0x0- 0x200 sits in pmic
> > address space and range 0x201 - 0x4ff in the SOC address space,
> 
> > Are there any other better ways to model this kinda driver?
> 
> Why not just have separate devices for each of the register maps?

Srinivas,

Mark has a good point.  Also, by having distinct devices and drivers;
you should make use of ASoC's supporting wrappers for regmap accesses.

-- 
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]

#1424042 — Re: [PATCH v3 2/2] ASoC: msm8916: Add msm8916-wcd codec driver

On 15/06/16 21:07, Kenneth Westfield wrote:
> On Wed, Jun 15, 2016 at 10:31:36AM +0100, Mark Brown wrote:
>> On Wed, Jun 15, 2016 at 10:16:27AM +0100, Srinivas Kandagatla wrote:
>>> On 14/06/16 16:59, Mark Brown wrote:
>>>> On Fri, Jun 10, 2016 at 07:18:45PM +0100, Srinivas Kandagatla wrote:
>
>>> In theory there are 3 devices,
>>> one is the pmic-spmi driver, which provides regmap access to analog part of
>>> codec registers.
>>> second is syscon driver which provides regmap access to digital parts of
>>> codec to codec driver.
>>> third is the codec driver which uses both the above.
>>
>>> Codec registers range is just split into two, range 0x0- 0x200 sits in pmic
>>> address space and range 0x201 - 0x4ff in the SOC address space,
>>
>>> Are there any other better ways to model this kinda driver?
>>
>> Why not just have separate devices for each of the register maps?
>
> Srinivas,
>
> Mark has a good point.  Also, by having distinct devices and drivers;
> you should make use of ASoC's supporting wrappers for regmap accesses.

Yep, Let to try that and see how it looks.

thanks,
srini
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web