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.
