Re: [PATCH v1 04/10] ASoC: codecs: Add uda1342 codec driver

[Binbin Zhou <zhoubb.aaron@xxxxxxxxx>]

Hi Mark:

Thanks for your detailed review.

On Thu, Sep 5, 2024 at 8:28 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Thu, Sep 05, 2024 at 03:02:53PM +0800, Binbin Zhou wrote:
>
> > The UDA1342 is an NXP audio codec, support 2x Stereo audio ADC (4x PGA
> > mic inputs), stereo audio DAC, with basic audio processing.
>
> This looks basically fine overall, there's some issues below but they're
> mostly fairly small and stylistic rather than anything major.
>
> > --- /dev/null
> > +++ b/sound/soc/codecs/uda1342.c
> > @@ -0,0 +1,397 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * uda1342.c  --  UDA1342 ALSA SoC Codec driver
>
> Please keep the entire comment a C++ one so things look more
> intentional.
>
> > +static int uda1342_mute(struct snd_soc_dai *dai, int mute, int direction)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +     unsigned int mask;
> > +     unsigned int val = 0;
> > +
> > +     dev_info(&uda1342->i2c->dev, "mute: %d\n", mute);
>
> This is far too noisy to be logged and will DoS the logs, please just
> remove it.

OK, I will drop it.
>
> > +
> > +     /* Master mute */
> > +     mask = BIT(5);
> > +     val = mute ? mask : 0;
>
> Please use normal conditional statements to improve legibility.

OK, the code as following:

if (mute)
                val = mask;
>
> > +static void uda1342_shutdown(struct snd_pcm_substream *substream,
> > +                          struct snd_soc_dai *dai)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +
> > +     if (uda1342->master_substream == substream)
> > +             uda1342->master_substream = uda1342->slave_substream;
>
> Please avoid using master/slave terminology, we've tended to go for
> provider/consumer in ASoC.

OK, I see, I will  rename it.
>
> > +static int uda1342_hw_params(struct snd_pcm_substream *substream,
> > +                          struct snd_pcm_hw_params *params, struct snd_soc_dai *dai)
> > +{
> > +     struct snd_soc_component *component = dai->component;
> > +     struct uda1342_priv *uda1342 = snd_soc_component_get_drvdata(component);
> > +     struct device *dev = &uda1342->i2c->dev;
> > +     unsigned int hw_params = 0;
> > +
> > +     if (substream == uda1342->slave_substream) {
> > +             dev_info(dev, "ignoring hw_params for slave substream\n");
> > +             return 0;
> > +     }
>
> This is also too noisy, it should be _dbg() at most.

OK, I will drop it.
>
> > +     /* codec supports only full slave mode */
> > +     if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS) {
> > +             dev_err(dev, "unsupported slave mode.\n");
> > +             return -EINVAL;
> > +     }
>
> Use the more modern _CBC_CFC.

OK, it should be replaced by SND_SOC_DAIFMT_BC_FC.
>
> > +     /*
> > +      * We can't setup DAI format here as it depends on the word bit num,
> > +      * so let's just store the value for later
> > +      */
> > +     uda1342->dai_fmt = fmt;
>
> No need to even store it if only one value is supported.

Emm, I will put the fmt setting here from the hw_param function.
Also, the dai_fmt could be dropped.
>
> > +static int uda1342_set_bias_level(struct snd_soc_component *component,
> > +                               enum snd_soc_bias_level level)
> > +{
> > +     switch (level) {
> > +     case SND_SOC_BIAS_ON:
> > +             break;
> > +     case SND_SOC_BIAS_PREPARE:
> > +             break;
> > +     case SND_SOC_BIAS_STANDBY:
> > +             break;
> > +     case SND_SOC_BIAS_OFF:
> > +             break;
> > +     }
> > +
> > +     return 0;
> > +}
>
> This does nothing so it can just be removed.

I will drop it.
>
> > +static const struct soc_enum uda1342_mixer_enum[] = {
> > +     SOC_ENUM_SINGLE(0x10, 3, 0x04, uda1342_deemph),
> > +     SOC_ENUM_SINGLE(0x10, 0, 0x04, uda1342_mixmode),
> > +};
>
> This doesn't seem to be referenced anywhere so can be removed, or should
> be added to the controls or DAPM?

I will drop it.
>
> > +static int uda1342_soc_probe(struct snd_soc_component *component)
> > +{
> > +     struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> > +
> > +     snd_soc_add_component_controls(component, uda1342_snd_controls,
> > +                                    ARRAY_SIZE(uda1342_snd_controls));
> > +     snd_soc_dapm_new_controls(dapm, uda1342_dapm_widgets,
> > +                               ARRAY_SIZE(uda1342_dapm_widgets));
> > +     snd_soc_dapm_add_routes(dapm, uda1342_dapm_routes,
> > +                             ARRAY_SIZE(uda1342_dapm_routes));
>
> You can point to these arrays from the component struct and have the
> core register them for you.

OK, I will do it.

Thanks.
Binbin
>
> > +static const struct regmap_config uda1342_regmap = {
> > +     .reg_bits = 8,
> > +     .val_bits = 16,
> > +     .max_register = 0x21,
> > +     .reg_defaults = uda1342_reg_defaults,
> > +     .num_reg_defaults = ARRAY_SIZE(uda1342_reg_defaults),
> > +     .cache_type = REGCACHE_RBTREE,
>
> In general _MAPLE is preferred for new things unless you have a specific
> reason to use _RBTREE, it uses a more modern data structure and should
> generally do better.