Re: [alsa-devel] [PATCH] ASoC: nau8825: Add driver for headset chip Nuvoton 8825

On Fri, Aug 28, 2015 at 04:37:50PM -0700, Anatol Pomozov wrote:

• SOC_SINGLE_TLV("Frontend PGA Gain", NAU8825_REG_POWER_UP_CONTROL,

• 8, 37, 0, fepga_gain_tlv),
  

All volume controls need to end in Volume as per ControlNames.txt.

• default:

• return -EINVAL;
  

• }
• regmap_update_bits(nau8825->regmap, NAU8825_REG_I2S_PCM_CTRL1,

Blank line here.

• /* Bias and clock are needed for button detection */
• regmap_update_bits(nau8825->regmap, NAU8825_REG_BIAS_ADJ,

• NAU8825_BIAS_VMID, NAU8825_BIAS_VMID);
  

• regmap_update_bits(nau8825->regmap, NAU8825_REG_BOOST,

• NAU8825_GLOBAL_BIAS_EN, NAU8825_GLOBAL_BIAS_EN);
  

These sound like DAPM widgets which should be being eneabled when jack detection is enabled.

• /* Ground HP Outputs[1:0], needed for headset auto detection

• * Enable Automatic Mic/Gnd switching reading on insert interrupt[6] */
  

• regmap_update_bits(regmap, NAU8825_REG_HSD_CTRL,

• NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L,
  

• NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L);
  

I'm still not happy to see the unconditional enabling of detection and switching.

• if (!nau8825->mclk_freq)
  

• 	clk_prepare_enable(nau8825->mclk);
  

You should be checking the return value here.

• if (nau8825->mclk_freq != freq) {
  

• 	nau8825->mclk_freq = freq;
  

• 	freq = clk_round_rate(nau8825->mclk, freq);
  

• 	clk_set_rate(nau8825->mclk, freq);
  

Likewise here and in other cases where you're setting clocks.

• nau8825->jkdet_enable = device_property_read_bool(dev,

• "nuvoton,jkdet-enable");
  

This DT parsing stuff should really be moved out into a separate function which is only called (or only does anything) if there is actually DT data (if there is an of_node set).

• if (nau8825->irq) {

• int ret = devm_request_threaded_irq(dev, nau8825->irq, NULL,
  

• 	nau8825_interrupt, IRQF_TRIGGER_LOW | IRQF_ONESHOT,
  

• 	"nau8825", nau8825);
  

• if (ret) {
  

• 	dev_err(dev, "Cannot request irq %d (%d)\n",
  

• 		nau8825->irq, ret);
  

• 	return ret;
  

• }
  

• }

So we actually handle systems with no interrupt wired up? That seems to make all the non-optional enabling of the jack detection interrupts a bit odd.

+static struct i2c_driver nau8825_driver = {

• .driver = {

• .name = "nau8825",
  

• .owner = THIS_MODULE,
  

• },
• .probe = nau8825_i2c_probe,
• .remove = nau8825_i2c_remove,
• .id_table = nau8825_i2c_ids,

+};

This has a DT binding but no OF IDs defined. It is true that Linux will currently try to guess a suitable driver based on the I2C ID table but it is bad practice to rely on this behaviour, we do see ID collisions sometimes (for example both WonderMedia and Wolfson using WM for their parts).