Re: [PATCH v1 02/10] ASoC: codecs: Add support for ES8323

[Binbin Zhou <zhoubb.aaron@xxxxxxxxx>]

Hi Mark:

Thanks for your detailed review.

On Thu, Sep 5, 2024 at 8:05 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:15PM +0800, Binbin Zhou wrote:
>
> This looks like it was based on some *extremely* old code and needs
> quite a few updates to bring it up to modern standards.
>
> > + * Author: Mark Brown <will@xxxxxxxxxxxxxxxx>
>
> Oh?
>
> > +/*
> > + * es8323 register cache
> > + * We can't read the es8323 register space when we
> > + * are using 2 wire for device control, so we cache them instead.
> > + */
> > +static u16 es8323_reg[] = {
> > +     0x06, 0x1C, 0xC3, 0xFC, /*  0 */
> > +     0xC0, 0x00, 0x00, 0x7C, /*  4 */
>
> There's a bunch of regmap reimplementation in the driver, the cache and
> I/O code all looks totally generic.  Just use regmap.

Sorry, I don't really understand this point.
Do you mean to use regmap_read()/regmap_write() instead of
snd_soc_component_read()/snd_soc_component_write()?

If so, are there any rules for using snd_soc_component_xxx()?
>
> > +static const struct soc_enum es8323_enum[] = {
> > +     SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 3, 7, ARRAY_SIZE(es8323_line_texts),
> > +                           es8323_line_texts, es8323_line_values),   /* LLINE */
> > +     SOC_VALUE_ENUM_SINGLE(ES8323_DACCONTROL16, 0, 7, ARRAY_SIZE(es8323_line_texts),
> > +                           es8323_line_texts, es8323_line_values),   /* RLINE */
>
> Use named variables for the enums rather than putting them into an array
> that's not otherwise used...

OK, I will use macro like SOC_ENUM_SINGLE_DECL() to define them.
>
> > +static const struct snd_kcontrol_new es8323_snd_controls[] = {
> > +     SOC_ENUM("3D Mode", es8323_enum[4]),
> > +     SOC_ENUM("ALC Capture Function", es8323_enum[5]),
>
> ...it's *vastly* more readable and maintainable.
>
> > +     SOC_SINGLE("Capture Mute", ES8323_ADC_MUTE, 2, 1, 0),
>
> On/off switches should end in Switch, see control-names.rst.
>
> > +     /* gModify.Cmmt Implement when suspend/startup */
> > +     SND_SOC_DAPM_DAC("Right DAC", "Right Playback", SND_SOC_NOPM, 6, 1),
>
> gModify.Cmmt?
>
> > +/*
> > + * Note that this should be called from init rather than from hw_params.
> > + */
> > +static int es8323_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> > +                              int clk_id, unsigned int freq, int dir)
>
> Why?
>
> > +     /* set master/slave audio interface */
> > +     switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> > +     case SND_SOC_DAIFMT_CBM_CFM:    /* MASTER MODE */
> > +             iface |= 0x80;
> > +             break;
> > +     case SND_SOC_DAIFMT_CBS_CFS:    /* SLAVE MODE */
>
> Please use the modern naming with _CBP_CFP and so on.

Get. SND_SOC_DAIFMT_BC_FP/SND_SOC_DAIFMT_BC_FC will be used.
>
> > +static int es8323_mute(struct snd_soc_dai *dai, int mute,  int stream)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct es8323_priv *es8323 = snd_soc_component_get_drvdata(component);
> > +
> > +     if (mute) {
> > +             es8323_set_gpio(ES8323_CODEC_SET_SPK,
> > +                             !es8323->spk_gpio_level, es8323->spk_ctl_gpio);
> > +             usleep_range(2000, 3000);
> > +             snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> > +     } else {
> > +             snd_soc_component_write(component, ES8323_DAC_MUTE, 0x02);
> > +             usleep_range(2000, 3000);
> > +             if (!es8323->hp_inserted)
> > +                     es8323_set_gpio(ES8323_CODEC_SET_SPK,
> > +                                     es8323->spk_gpio_level, es8323->spk_ctl_gpio);
>
> This appears to be controlling the speaker based on jack detection.
> Unless there is some hardware restriction this should be done via DAPM
> and the user allowed to manage when the speaker is used depending on
> their use case.

In this version of the patch, I plan to remove the jack related
functions, and I will add them later.
>
> > +static int es8323_suspend(struct snd_soc_component *component)
> > +{
> > +     snd_soc_component_write(component, ES8323_DAC_MUTE, 0x06);
> > +     snd_soc_component_write(component, ES8323_LOUT1_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_ROUT1_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_LOUT2_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_ROUT2_VOL, 0x00);
>
> This will overwrite user settings when suspending, the controls should
> be unaffected by suspend.  If the device needs this then use cache
> bypass to do the writes during suspend and resync the cache on resume.

regcache_cache_only() and regcache_sync() will be used.
>
> > +     snd_soc_component_write(component, ES8323_CONTROL1, 0x06);
> > +     snd_soc_component_write(component, ES8323_CONTROL2, 0x58);
> > +     usleep_range(18000, 20000);
>
> Use fsleep().
>
> > +static void es8323_init_component_regs(struct snd_soc_component *component)
> > +{
>
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL1, 0x88);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL2, 0xF0);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL3, 0x02);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL4, 0x0C);
> > +     snd_soc_component_write(component, ES8323_ADCCONTROL5, 0x02);
> > +     snd_soc_component_write(component, ES8323_LADC_VOL, 0x00);
> > +     snd_soc_component_write(component, ES8323_RADC_VOL, 0x00);
>
> User visible controls should use the chip defaults as standard,
> userspace can configure what it needs and this avoids us worrying about
> individual use cases in the driver.
>
> > +static int es8323_resume(struct snd_soc_component *component)
> > +{
> > +     es8323_init_component_regs(component);
> > +     /* open dac output */
> > +     snd_soc_component_write(component, ES8323_DACPOWER, 0x3c);
> > +
> > +     return 0;
> > +}
>
> This doesn't restore any user settings.
>
> > +static int es8323_i2c_probe(struct i2c_client *i2c)
> > +{
> > +     struct es8323_priv *es8323;
> > +     struct i2c_adapter *adapter = to_i2c_adapter(i2c->dev.parent);
> > +
> > +     if (!i2c_check_functionality(adapter, I2C_FUNC_I2C)) {
> > +             dev_warn(&adapter->dev,
> > +                      "I2C-Adapter doesn't support I2C_FUNC_I2C\n");
> > +             return -EIO;
> > +     }
>
> Does the device really need this?  It seems to be doing very basic I/O
> which should be SMBus compatible.  In any case when you move to regmap
> this should be redundant.

Emm, I will drop it.

Thanks.
Binbin