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.