Re: [PATCH 5/5] ASoC: codecs: Add WCD939x Codec driver

On Thu, Nov 23, 2023 at 03:49:15PM +0100, Neil Armstrong wrote:

• if (wcd939x->variant == WCD9390) {

• if (mode_val == CLS_H_HIFI || mode_val == CLS_AB_HIFI) {
  

• 	dev_info(component->dev, "%s: Invalid HPH Mode, default to CLS_H_ULP\n",
  

• 		 __func__);
  

• 	mode_val = CLS_H_ULP;
  

• }
  

• }
• if (mode_val == CLS_H_NORMAL) {

• dev_info(component->dev, "%s: Invalid HPH Mode, default to class_AB\n",
  

• 	 __func__);
  

• mode_val = CLS_H_ULP;
  

• }

This lets people spam the logs from userspace, why not just return an error?

• if (hphr)

• wcd939x->comp2_enable = value;
  

• else

• wcd939x->comp1_enable = value;
  

Are there really no constraints on the values for these things?

+static const struct snd_kcontrol_new wcd9390_snd_controls[] = {

• SOC_ENUM_EXT("EAR PA GAIN", wcd939x_ear_pa_gain_enum,

•      wcd939x_ear_pa_gain_get, wcd939x_ear_pa_gain_put),
  

Gains should be be named Volume. Is there no TLV information?

+static irqreturn_t wcd939x_wd_handle_irq(int irq, void *data) +{

• return IRQ_HANDLED;

+}

Why even request the interrupt if it's just going to be ignored?