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.
> +
> + /* Master mute */
> + mask = BIT(5);
> + val = mute ? mask : 0;
Please use normal conditional statements to improve legibility.
> +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.
> +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.
> + /* 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.
> + /*
> + * 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.
> +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.
> +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?
> +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.
> +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.
Attachment:
signature.asc
Description: PGP signature
