Re: [PATCH 2/2] ASoC: max98390: Added Amplifier Driver

(The patch passed 2 round review in https://crrev.com/c/2083354)

On Sun, May 10, 2020 at 4:23 PM Steve Lee steves.lee.maxim@gmail.com wrote:

+static int max98390_dai_set_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt) +{

•   struct snd_soc_component *component = codec_dai->component;
  

•   struct max98390_priv *max98390 =
  

•           snd_soc_component_get_drvdata(component);
  

•   unsigned int mode;
  

•   unsigned int format;
  

•   unsigned int invert;
  

•   dev_dbg(component->dev, "%s: fmt 0x%08X\n", __func__, fmt);
  

•   switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
  

•   case SND_SOC_DAIFMT_CBS_CFS:
  

•           mode = MAX98390_PCM_MASTER_MODE_SLAVE;
  

•           break;
  

•   case SND_SOC_DAIFMT_CBM_CFM:
  

•           max98390->master = true;
  

•           mode = MAX98390_PCM_MASTER_MODE_MASTER;
  

•           break;
  

•   default:
  

•           dev_err(component->dev, "DAI clock mode unsupported\n");
  

•           return -EINVAL;
  

•   }
  

•   regmap_update_bits(max98390->regmap,
  

•           MAX98390_PCM_MASTER_MODE,
  

•           MAX98390_PCM_MASTER_MODE_MASK,
  

•           mode);
  

•   switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
  

•   case SND_SOC_DAIFMT_NB_NF:
  

•           break;
  

•   case SND_SOC_DAIFMT_IB_NF:
  

•           invert = MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE;
  

•           break;
  

•   default:
  

•           dev_err(component->dev, "DAI invert mode unsupported\n");
  

•           return -EINVAL;
  

•   }
  

•   regmap_update_bits(max98390->regmap,
  

•           MAX98390_PCM_MODE_CFG,
  

•           MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE,
  

•           invert);
  

invert will be uninitialized in the SND_SOC_DAIFMT_NB_NF case.

+static int max98390_dai_hw_params(struct snd_pcm_substream *substream,

•           struct snd_pcm_hw_params *params,
  

•   struct snd_soc_dai *dai)
  

Lack of 1 tab indent.

+static int max98390_adaptive_rdc_get(struct snd_kcontrol *kcontrol,

•           struct snd_ctl_elem_value *ucontrol)
  

+{

•   int rdc, rdc0;
  

•   struct snd_soc_component *component =
  

•           snd_soc_kcontrol_component(kcontrol);
  

•   struct max98390_priv *max98390 =
  

•           snd_soc_component_get_drvdata(component);
  

•   regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE1, &rdc);
  

•   regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE0, &rdc0);
  

•   rdc0 |= rdc << 8;
  

•   ucontrol->value.integer.value[0] = rdc0;
  

ucontrol->value.integer.value[0] = rdc0 | (rdc << 8);

+static int max98390_dsm_init(struct snd_soc_component *component) +{

•   int ret;
  

•   const char *filename;
  

•   struct max98390_priv *max98390 =
  

•           snd_soc_component_get_drvdata(component);
  

•   const struct firmware *fw = NULL;
  

•   char *dsm_param = NULL;
  

Don't need to initialize fw and dsm_param in the case.

•   filename = "dsm_param.bin";
  

Either: - initialize when declaring the variable - remove the variable and inline into request_firmware() call

•   ret = request_firmware(&fw, filename, component->dev);
  

•   if (ret) {
  

•           dev_err(component->dev,
  

•                   "Failed to acquire dsm params: %d\n", ret);
  

•           goto err;
  

•   }
  

•   dev_info(component->dev,
  

•           "max98390: param fw size %d\n",
  

•           fw->size);
  

•   dsm_param = (char *)fw->data;
  

•   dsm_param += MAX98390_DSM_PAYLOAD_OFFSET;
  

•   regmap_bulk_write(max98390->regmap, DSM_EQ_BQ1_B0_BYTE0,
  

•           dsm_param,
  

•           fw->size - MAX98390_DSM_PAYLOAD_OFFSET);
  

•   release_firmware(fw);
  

•   regmap_write(max98390->regmap, MAX98390_R23E1_DSP_GLOBAL_EN, 0x01);
  

+err:

•   return ret;
  

+}

+static int max98390_probe(struct snd_soc_component *component) +{

•   struct max98390_priv *max98390 =
  

•           snd_soc_component_get_drvdata(component);
  

•   /* Update dsm bin param */
  

This comment makes more sense if before max98390_dsm_init().

•   regmap_write(max98390->regmap, MAX98390_SOFTWARE_RESET, 0x01);
  

•   /* Sleep reset settle time */
  

•   msleep(20);
  

•   max98390_dsm_init(component);
  

•   /* Check Revision ID */
  

•   ret = regmap_read(max98390->regmap,
  

•           MAX98390_R24FF_REV_ID, &reg);
  

•   if (ret < 0) {
  

if (ret)