| Message ID | 1597028754-7732-1-git-send-email-jiaxin.yu@mediatek.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add mediatek codec mt6359 driver | expand |
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
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, ®); > + 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.
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, ®); > > + 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.
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.
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.
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.
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.