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

[Mark Brown]

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).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150830/e42030f1/attachment-0001.sig>