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.
