mbox series

[v2,0/2] Add mediatek codec mt6359 driver

Message ID 1597028754-7732-1-git-send-email-jiaxin.yu@mediatek.com (mailing list archive)
Headers show
Series Add mediatek codec mt6359 driver | expand

Message

Jiaxin Yu Aug. 10, 2020, 3:05 a.m. UTC
Add mediatek codec (MT6359) driver

MT6359 support playback and capture feature.

On downlink path, it includes three DACs for handset, headset,
and lineout path. On unlink path, it includeds three ADCs for
main mic, second mic, 3rd mic, and headset mic.

By scenario, select *_MUX widget to create damp path.
And by select mic_type_mux to choose DMIC/AMIC/....

For example, select these MUX widget to create headset path
(1) DAC In Mux --> "Normal Path"
(2) HPL Mux --> "Audio Playback"
(3) HPR Mux --> "Audio Playback"

v2 changes:
	1. Move playback_gpio/capture_gpio to the machine driver.
	2. Fix mic_type(ACC/DMIC/DCC/DCC_*) at init stage.
	3. Move devm_regulor_get to mt6359_platform_driver_probe.
	4. Add relulator_disable in remove function.
	5. Use stereo controls to the volume control.
	6. Use SOC_ENUM_SINGLE_DECL instead of SOC_VALUE_ENUM_SINGLE_DECL.
	7. Cleanup unused code.

v1 changes:
	1.lkml link:
		https://lkml.org/lkml/2020/3/5/1257

Jiaxin Yu (2):
  ASoC: mediatek: mt6359: add codec driver
  dt-bindings: mediatek: mt6359: add codec document

 .../devicetree/bindings/sound/mt6359.yaml          |   68 +
 sound/soc/codecs/Kconfig                           |    8 +
 sound/soc/codecs/Makefile                          |    2 +
 sound/soc/codecs/mt6359.c                          | 2966 ++++++++++++++++++++
 sound/soc/codecs/mt6359.h                          | 2653 +++++++++++++++++
 5 files changed, 5697 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/mt6359.yaml
 create mode 100644 sound/soc/codecs/mt6359.c
 create mode 100644 sound/soc/codecs/mt6359.h

Comments

Tzung-Bi Shih Aug. 10, 2020, 8:13 a.m. UTC | #1
On Mon, Aug 10, 2020 at 11:11 AM Jiaxin Yu <jiaxin.yu@mediatek.com> wrote:
>
> Add the mt6359 codec driver.
>
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>

Reviewed-by: Tzung-Bi Shih <tzungbi@google.com>

This patch also reviewed few rounds on https://crrev.com/c/2299951
Mark Brown Aug. 10, 2020, 6:59 p.m. UTC | #2
On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote:

> +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt)
> +{
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);

> +void mt6359_reset_playback_gpio(struct snd_soc_component *cmpnt)
> +{
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);

> +void mt6359_set_capture_gpio(struct snd_soc_component *cmpnt)
> +{

> +void mt6359_reset_capture_gpio(struct snd_soc_component *cmpnt)
> +{

What are these, should they not be managed through gpiolib and/or
pinctrl?

> +/* use only when doing mtkaif calibraiton at the boot time */
> +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable)
> +{
> +	regmap_update_bits(priv->regmap, MT6359_DCXO_CW12,
> +			   0x1 << RG_XO_AUDIO_EN_M_SFT,
> +			   (enable ? 1 : 0) << RG_XO_AUDIO_EN_M_SFT);
> +	return 0;

Either don't have a return value or use the result of
regmap_update_bits().  There's similar issues with some other functions
in here.

> +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> +{

> +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);

Why is this exported?

> +static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up)
> +{
> +	int i = 0, stage = 0;
> +
> +	/* Reduce HP aux feedback loop gain step by step */
> +	for (i = 0; i <= 0xf; i++) {
> +		stage = up ? i : 0xf - i;

Please write normal conditional statements, it helps legibility.

> +static int mt6359_put_volsw(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +			snd_soc_kcontrol_component(kcontrol);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(component);
> +	struct soc_mixer_control *mc =
> +			(struct soc_mixer_control *)kcontrol->private_value;
> +	unsigned int reg;
> +	int index = ucontrol->value.integer.value[0];
> +	int ret;
> +
> +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> +	if (ret < 0)
> +		return ret;

So we make the volume change actually take effect...

> +	switch (mc->reg) {
> +	case MT6359_ZCD_CON2:
> +		regmap_read(priv->regmap, MT6359_ZCD_CON2, &reg);
> +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] =
> +			(reg >> RG_AUDHPLGAIN_SFT) & RG_AUDHPLGAIN_MASK;
> +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] =
> +			(reg >> RG_AUDHPRGAIN_SFT) & RG_AUDHPRGAIN_MASK;
> +		break;

...then read the value that was set and store it elsewhere.  What's
going on here?

> +/*HP MUX */
> +static const char * const hp_in_mux_map[] = {
> +	"Open",
> +	"LoudSPK Playback",
> +	"Audio Playback",
> +	"Test Mode",
> +	"HP Impedance",
> +	"undefined1",
> +	"undefined2",
> +	"undefined3",
> +};

Why expose undefined (and presumably out of spec) values to userspace?

> +static int mt_clksq_event(struct snd_soc_dapm_widget *w,
> +			  struct snd_kcontrol *kcontrol,
> +			  int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> +
> +	dev_dbg(priv->dev, "%s(), event = 0x%x\n", __func__, event);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		/* audio clk source from internal dcxo */
> +		regmap_update_bits(priv->regmap, MT6359_AUDENC_ANA_CON23,
> +				   RG_CLKSQ_IN_SEL_TEST_MASK_SFT,
> +				   0x0);

This also appeared to be controlled in _set_clkseq() - are we sure that
things couldn't get confused about the state?

> +	/* HP damp circuit enable */
> +	/*Enable HPRN/HPLN output 4K to VCM */

Spaces around the /* */

> +static int mt_hp_event(struct snd_soc_dapm_widget *w,
> +		       struct snd_kcontrol *kcontrol,
> +		       int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> +	unsigned int mux = dapm_kcontrol_get_value(w->kcontrols[0]);
> +	int device = DEVICE_HP;
> +
> +	dev_dbg(priv->dev, "%s(), event 0x%x, dev_counter[DEV_HP] %d, mux %u\n",
> +		__func__, event, priv->dev_counter[device], mux);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		priv->dev_counter[device]++;
> +		if (priv->dev_counter[device] > 1)
> +			break;	/* already enabled, do nothing */
> +		else if (priv->dev_counter[device] <= 0)

Why are we doing additional refcounting on top of what DAPM is doing?
This seems like there should be at least one widget representing the
shared bits of the audio path.

> +#define MT6359_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> +			SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |\
> +			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> +			SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |\
> +			SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |\
> +			SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE)

The driver doesn't appear to configure anything except the sample rate -
how are all these formats supported?

> +	/* hp gain ctl default choose ZCD */
> +	priv->hp_gain_ctl = HP_GAIN_CTL_ZCD;
> +	hp_gain_ctl_select(priv, priv->hp_gain_ctl);

Why not use the hardware default?

> +	mt6359_codec_init_reg(cmpnt);
> +
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] = 8;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1] = 3;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2] = 3;
> +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP3] = 3;

Same here.

> +	ret = regulator_enable(priv->avdd_reg);
> +	if (ret) {
> +		dev_err(priv->dev, "%s(), failed to enable regulator!\n",
> +			__func__);
> +		return ret;
> +	}

Perhaps make this a DAPM widget?

> +	priv->avdd_reg = devm_regulator_get(&pdev->dev, "vaud18");
> +	if (IS_ERR(priv->avdd_reg)) {
> +		dev_err(&pdev->dev, "%s(), have no vaud18 supply", __func__);
> +		return PTR_ERR(priv->avdd_reg);
> +	}

It's better to print error codes to help people debugging problems.

> +static const struct of_device_id mt6359_of_match[] = {
> +	{.compatible = "mediatek,mt6359-sound",},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mt6359_of_match);

We don't need a compatible here, we know that this device is here since
it's part of the parent device and isn't something that might appear in
another device.  This is reflecting the Linux driver model, not the
hardware.
Jiaxin Yu Aug. 12, 2020, 7:29 a.m. UTC | #3
On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote:
> On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote:
> 
> > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt)
> > +{
> > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> 
> > +void mt6359_reset_playback_gpio(struct snd_soc_component *cmpnt)
> > +{
> > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> 
> > +void mt6359_set_capture_gpio(struct snd_soc_component *cmpnt)
> > +{
> 
> > +void mt6359_reset_capture_gpio(struct snd_soc_component *cmpnt)
> > +{
> 
> What are these, should they not be managed through gpiolib and/or
> pinctrl?

These are the functions that control the mux of input or output
signal. I refer to the other codec drivers, most of them are manipulate
the regs in their codec drivers also. Maybe it's easier to control?

> > +/* use only when doing mtkaif calibraiton at the boot time */
> > +static int mt6359_set_dcxo(struct mt6359_priv *priv, bool enable)
> > +{
> > +	regmap_update_bits(priv->regmap, MT6359_DCXO_CW12,
> > +			   0x1 << RG_XO_AUDIO_EN_M_SFT,
> > +			   (enable ? 1 : 0) << RG_XO_AUDIO_EN_M_SFT);
> > +	return 0;
> 
> Either don't have a return value or use the result of
> regmap_update_bits().  There's similar issues with some other functions
> in here.
> 

Yes, I will refine them.

> > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> > +{
> 
> > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);
> 
> Why is this exported?
> 

This function is exported to machine driver to do calibration when
registering. The interface between MT6359 and MTK SoC is a special
interface that named MTKAIF. Therefore, if MT6359 is to be used
normally, it needs to rely on the platform for calibration when
registering.

> > +static void hp_aux_feedback_loop_gain_ramp(struct mt6359_priv *priv, bool up)
> > +{
> > +	int i = 0, stage = 0;
> > +
> > +	/* Reduce HP aux feedback loop gain step by step */
> > +	for (i = 0; i <= 0xf; i++) {
> > +		stage = up ? i : 0xf - i;
> 
> Please write normal conditional statements, it helps legibility.
> 

/* Increase/Reduce HP aux feedback loop gain step by step */
This is to prevent sudden changes in volume.

> > +static int mt6359_put_volsw(struct snd_kcontrol *kcontrol,
> > +			    struct snd_ctl_elem_value *ucontrol)
> > +{
> > +	struct snd_soc_component *component =
> > +			snd_soc_kcontrol_component(kcontrol);
> > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(component);
> > +	struct soc_mixer_control *mc =
> > +			(struct soc_mixer_control *)kcontrol->private_value;
> > +	unsigned int reg;
> > +	int index = ucontrol->value.integer.value[0];
> > +	int ret;
> > +
> > +	ret = snd_soc_put_volsw(kcontrol, ucontrol);
> > +	if (ret < 0)
> > +		return ret;
> 
> So we make the volume change actually take effect...
> 
> > +	switch (mc->reg) {
> > +	case MT6359_ZCD_CON2:
> > +		regmap_read(priv->regmap, MT6359_ZCD_CON2, &reg);
> > +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] =
> > +			(reg >> RG_AUDHPLGAIN_SFT) & RG_AUDHPLGAIN_MASK;
> > +		priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] =
> > +			(reg >> RG_AUDHPRGAIN_SFT) & RG_AUDHPRGAIN_MASK;
> > +		break;
> 
> ..then read the value that was set and store it elsewhere.  What's
> going on here?
> 

Because we can adjust the volume at two points, before and during the
playback or capture. In addition, we will do volume ramp when opening
the driver. If we set the volume before opening the driver, we need get
the value to do volume ramp during opening the driver.

> > +/*HP MUX */
> > +static const char * const hp_in_mux_map[] = {
> > +	"Open",
> > +	"LoudSPK Playback",
> > +	"Audio Playback",
> > +	"Test Mode",
> > +	"HP Impedance",
> > +	"undefined1",
> > +	"undefined2",
> > +	"undefined3",
> > +};
> 
> Why expose undefined (and presumably out of spec) values to userspace?
> 

Removed them.

> > +static int mt_clksq_event(struct snd_soc_dapm_widget *w,
> > +			  struct snd_kcontrol *kcontrol,
> > +			  int event)
> > +{
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> > +
> > +	dev_dbg(priv->dev, "%s(), event = 0x%x\n", __func__, event);
> > +
> > +	switch (event) {
> > +	case SND_SOC_DAPM_PRE_PMU:
> > +		/* audio clk source from internal dcxo */
> > +		regmap_update_bits(priv->regmap, MT6359_AUDENC_ANA_CON23,
> > +				   RG_CLKSQ_IN_SEL_TEST_MASK_SFT,
> > +				   0x0);
> 
> This also appeared to be controlled in _set_clkseq() - are we sure that
> things couldn't get confused about the state?
> 

_set_clkseq() will only be run at the time of mtkaif calibraiton, and
the calibration only run once when the codec driver probe. So it won't
get confused about the state. But it really only needs to be set up once
at mt6359_codec_init_reg(). I will remove the unnecesary settings.

> > +	/* HP damp circuit enable */
> > +	/*Enable HPRN/HPLN output 4K to VCM */
> 
> Spaces around the /* */
> 

Sorry, fixed it.

> > +static int mt_hp_event(struct snd_soc_dapm_widget *w,
> > +		       struct snd_kcontrol *kcontrol,
> > +		       int event)
> > +{
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> > +	unsigned int mux = dapm_kcontrol_get_value(w->kcontrols[0]);
> > +	int device = DEVICE_HP;
> > +
> > +	dev_dbg(priv->dev, "%s(), event 0x%x, dev_counter[DEV_HP] %d, mux %u\n",
> > +		__func__, event, priv->dev_counter[device], mux);
> > +
> > +	switch (event) {
> > +	case SND_SOC_DAPM_PRE_PMU:
> > +		priv->dev_counter[device]++;
> > +		if (priv->dev_counter[device] > 1)
> > +			break;	/* already enabled, do nothing */
> > +		else if (priv->dev_counter[device] <= 0)
> 
> Why are we doing additional refcounting on top of what DAPM is doing?
> This seems like there should be at least one widget representing the
> shared bits of the audio path.
> 

We have "HPL Mux" and "HPR Mux", there will be two paths enabled when
playback throuh HP. But actually they share the same control sequences.
So in order to prevent setting it one more time we do additional
refcouting.


> > +#define MT6359_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
> > +			SNDRV_PCM_FMTBIT_U16_LE | SNDRV_PCM_FMTBIT_U16_BE |\
> > +			SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\
> > +			SNDRV_PCM_FMTBIT_U24_LE | SNDRV_PCM_FMTBIT_U24_BE |\
> > +			SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE |\
> > +			SNDRV_PCM_FMTBIT_U32_LE | SNDRV_PCM_FMTBIT_U32_BE)
> 
> The driver doesn't appear to configure anything except the sample rate -
> how are all these formats supported?
> 

The interface between MT6359 and MTK SoC is a special interface that
named MTKAIF. Because of its unique hardware settings, it really doesn't
to set format argument. The capability of pcm driver mainly depends on
cpu-dai driver to limit.

> > +	/* hp gain ctl default choose ZCD */
> > +	priv->hp_gain_ctl = HP_GAIN_CTL_ZCD;
> > +	hp_gain_ctl_select(priv, priv->hp_gain_ctl);
> 
> Why not use the hardware default?
> 

We have two ways to control the volume, there is to select ZCD. Because
the other one it not often used, ZCD is used by default. 

> > +	mt6359_codec_init_reg(cmpnt);
> > +
> > +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTL] = 8;
> > +	priv->ana_gain[AUDIO_ANALOG_VOLUME_HPOUTR] = 8;
> > +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP1] = 3;
> > +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP2] = 3;
> > +	priv->ana_gain[AUDIO_ANALOG_VOLUME_MICAMP3] = 3;
> 
> Same here.
> 

Removed them.

> > +	ret = regulator_enable(priv->avdd_reg);
> > +	if (ret) {
> > +		dev_err(priv->dev, "%s(), failed to enable regulator!\n",
> > +			__func__);
> > +		return ret;
> > +	}
> 
> Perhaps make this a DAPM widget?
> 

"vaud18" needs to be always on so we enable it when codec probe.

> > +	priv->avdd_reg = devm_regulator_get(&pdev->dev, "vaud18");
> > +	if (IS_ERR(priv->avdd_reg)) {
> > +		dev_err(&pdev->dev, "%s(), have no vaud18 supply", __func__);
> > +		return PTR_ERR(priv->avdd_reg);
> > +	}
> 
> It's better to print error codes to help people debugging problems.
> 

Yes, I will print the error codes.

> > +static const struct of_device_id mt6359_of_match[] = {
> > +	{.compatible = "mediatek,mt6359-sound",},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mt6359_of_match);
> 
> We don't need a compatible here, we know that this device is here since
> it's part of the parent device and isn't something that might appear in
> another device.  This is reflecting the Linux driver model, not the
> hardware.

Removed them.
Mark Brown Aug. 12, 2020, 12:05 p.m. UTC | #4
On Wed, Aug 12, 2020 at 03:29:13PM +0800, Jiaxin Yu wrote:
> On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote:
> > On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote:

> > > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt)
> > > +{
> > > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);

> > What are these, should they not be managed through gpiolib and/or
> > pinctrl?

> These are the functions that control the mux of input or output
> signal. I refer to the other codec drivers, most of them are manipulate
> the regs in their codec drivers also. Maybe it's easier to control?

These functions are exported for other drivers to use rather than
something done internally by the driver - if this were internal to the
driver it'd not be a big deal but when it's for use by another driver
it'd be better to go through standard interfaces.

> > > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> > > +{

> > > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);

> > Why is this exported?

> This function is exported to machine driver to do calibration when
> registering. The interface between MT6359 and MTK SoC is a special
> interface that named MTKAIF. Therefore, if MT6359 is to be used
> normally, it needs to rely on the platform for calibration when
> registering.

So this needs the SoC to do something as part of callibration?

> > > +	switch (event) {
> > > +	case SND_SOC_DAPM_PRE_PMU:
> > > +		priv->dev_counter[device]++;
> > > +		if (priv->dev_counter[device] > 1)
> > > +			break;	/* already enabled, do nothing */
> > > +		else if (priv->dev_counter[device] <= 0)

> > Why are we doing additional refcounting on top of what DAPM is doing?
> > This seems like there should be at least one widget representing the
> > shared bits of the audio path.

> We have "HPL Mux" and "HPR Mux", there will be two paths enabled when
> playback throuh HP. But actually they share the same control sequences.
> So in order to prevent setting it one more time we do additional
> refcouting.

That sounds like you should just have one output path defined in DAPM
and merge the left and right signals together rather than maintaining
them separately.

> > > +	/* hp gain ctl default choose ZCD */
> > > +	priv->hp_gain_ctl = HP_GAIN_CTL_ZCD;
> > > +	hp_gain_ctl_select(priv, priv->hp_gain_ctl);

> > Why not use the hardware default?

> We have two ways to control the volume, there is to select ZCD. Because
> the other one it not often used, ZCD is used by default. 

Why not just let the user pick at runtime?

> > > +	ret = regulator_enable(priv->avdd_reg);
> > > +	if (ret) {
> > > +		dev_err(priv->dev, "%s(), failed to enable regulator!\n",
> > > +			__func__);
> > > +		return ret;
> > > +	}

> > Perhaps make this a DAPM widget?

> "vaud18" needs to be always on so we enable it when codec probe.

If it is just supposed to be left on all the time it's better to do this
in the main device model probe rather than the component probe.  The
component probe should usually only be used for things that need the
rest of the card to be there.
Jiaxin Yu Aug. 13, 2020, 3:40 p.m. UTC | #5
On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote:
> On Wed, Aug 12, 2020 at 03:29:13PM +0800, Jiaxin Yu wrote:
> > On Mon, 2020-08-10 at 19:59 +0100, Mark Brown wrote:
> > > On Mon, Aug 10, 2020 at 11:05:53AM +0800, Jiaxin Yu wrote:
> 
> > > > +void mt6359_set_playback_gpio(struct snd_soc_component *cmpnt)
> > > > +{
> > > > +	struct mt6359_priv *priv = snd_soc_component_get_drvdata(cmpnt);
> 
> > > What are these, should they not be managed through gpiolib and/or
> > > pinctrl?
> 
> > These are the functions that control the mux of input or output
> > signal. I refer to the other codec drivers, most of them are manipulate
> > the regs in their codec drivers also. Maybe it's easier to control?
> 
> These functions are exported for other drivers to use rather than
> something done internally by the driver - if this were internal to the
> driver it'd not be a big deal but when it's for use by another driver
> it'd be better to go through standard interfaces.
> 

Can we move this part of the operation to the codec dai driver ops, such
as .startup and .shutdown? Because originally these functions are
exported to machine driver's dai_link .startup and .shutdown ops.

> > > > +int mt6359_mtkaif_calibration_enable(struct snd_soc_pcm_runtime *rtd)
> > > > +{
> 
> > > > +EXPORT_SYMBOL_GPL(mt6359_mtkaif_calibration_enable);
> 
> > > Why is this exported?
> 
> > This function is exported to machine driver to do calibration when
> > registering. The interface between MT6359 and MTK SoC is a special
> > interface that named MTKAIF. Therefore, if MT6359 is to be used
> > normally, it needs to rely on the platform for calibration when
> > registering.
> 
> So this needs the SoC to do something as part of callibration?
> 

Yes, the side of MTKAIF in SoC part named adda, we need config it before
calibration. We will also upstream machine/platform driver that use this
codec driver later.

> > > > +	switch (event) {
> > > > +	case SND_SOC_DAPM_PRE_PMU:
> > > > +		priv->dev_counter[device]++;
> > > > +		if (priv->dev_counter[device] > 1)
> > > > +			break;	/* already enabled, do nothing */
> > > > +		else if (priv->dev_counter[device] <= 0)
> 
> > > Why are we doing additional refcounting on top of what DAPM is doing?
> > > This seems like there should be at least one widget representing the
> > > shared bits of the audio path.
> 
> > We have "HPL Mux" and "HPR Mux", there will be two paths enabled when
> > playback throuh HP. But actually they share the same control sequences.
> > So in order to prevent setting it one more time we do additional
> > refcouting.
> 
> That sounds like you should just have one output path defined in DAPM
> and merge the left and right signals together rather than maintaining
> them separately.
> 

Yes, it's would better if they are combined into one mixer control that
named "HP Mux".

> > > > +	/* hp gain ctl default choose ZCD */
> > > > +	priv->hp_gain_ctl = HP_GAIN_CTL_ZCD;
> > > > +	hp_gain_ctl_select(priv, priv->hp_gain_ctl);
> 
> > > Why not use the hardware default?
> 
> > We have two ways to control the volume, there is to select ZCD. Because
> > the other one it not often used, ZCD is used by default. 
> 
> Why not just let the user pick at runtime?
> 

For another adjustment method, we didn't upstream it, so only ZCD
reserved. And the hardware default setting is ZCD, so we can removed
these lines.

> > > > +	ret = regulator_enable(priv->avdd_reg);
> > > > +	if (ret) {
> > > > +		dev_err(priv->dev, "%s(), failed to enable regulator!\n",
> > > > +			__func__);
> > > > +		return ret;
> > > > +	}
> 
> > > Perhaps make this a DAPM widget?
> 
> > "vaud18" needs to be always on so we enable it when codec probe.
> 
> If it is just supposed to be left on all the time it's better to do this
> in the main device model probe rather than the component probe.  The
> component probe should usually only be used for things that need the
> rest of the card to be there.

Ok, I will correct it.
Mark Brown Aug. 13, 2020, 3:44 p.m. UTC | #6
On Thu, Aug 13, 2020 at 11:40:00PM +0800, Jiaxin Yu wrote:
> On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote:

> > These functions are exported for other drivers to use rather than
> > something done internally by the driver - if this were internal to the
> > driver it'd not be a big deal but when it's for use by another driver
> > it'd be better to go through standard interfaces.

> Can we move this part of the operation to the codec dai driver ops, such
> as .startup and .shutdown? Because originally these functions are
> exported to machine driver's dai_link .startup and .shutdown ops.

If it's internal to the driver sure.

> > So this needs the SoC to do something as part of callibration?

> Yes, the side of MTKAIF in SoC part named adda, we need config it before
> calibration. We will also upstream machine/platform driver that use this
> codec driver later.

It would probably be better to just leave this out for now then add the
required bits to the CODEC driver as part of a patch series that also
adds the machine driver that uses it so it's clear how it all fits
together.  It sounds like it should be fine but this'd be easier to
review.
Jiaxin Yu Aug. 14, 2020, 10:27 a.m. UTC | #7
On Thu, 2020-08-13 at 16:44 +0100, Mark Brown wrote:
> On Thu, Aug 13, 2020 at 11:40:00PM +0800, Jiaxin Yu wrote:
> > On Wed, 2020-08-12 at 13:05 +0100, Mark Brown wrote:
> 
> > > These functions are exported for other drivers to use rather than
> > > something done internally by the driver - if this were internal to the
> > > driver it'd not be a big deal but when it's for use by another driver
> > > it'd be better to go through standard interfaces.
> 
> > Can we move this part of the operation to the codec dai driver ops, such
> > as .startup and .shutdown? Because originally these functions are
> > exported to machine driver's dai_link .startup and .shutdown ops.
> 
> If it's internal to the driver sure.
> 
> > > So this needs the SoC to do something as part of callibration?
> 
> > Yes, the side of MTKAIF in SoC part named adda, we need config it before
> > calibration. We will also upstream machine/platform driver that use this
> > codec driver later.
> 
> It would probably be better to just leave this out for now then add the
> required bits to the CODEC driver as part of a patch series that also
> adds the machine driver that uses it so it's clear how it all fits
> together.  It sounds like it should be fine but this'd be easier to
> review.

Ok, I'll remove the functions that related to the calibration, and
upstream them again as part of a patch series that adds the machine
driver later.