[alsa-devel] [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC

[Mark Brown]

On Sat, Dec 17, 2016 at 03:52:33PM +0800, simon.ho.cnxt at gmail.com wrote:

> +/*FIXME: need to move the default settings to device tree*/

No, we'd expect things to be configured from userspace via a binary
control.

> +static unsigned char cx2072x_eq_coeff_array[MAX_EQ_BAND][MAC_EQ_COEFF] = {
> +	{0x77, 0x26, 0x13, 0xb3, 0x76, 0x26, 0x0a, 0x3d, 0xd4, 0xe2, 0x04},
> +	{0x97, 0x3e, 0xb3, 0x86, 0xc2, 0x3b, 0x4d, 0x79, 0xa7, 0xc5, 0x03},
> +	{0x0f, 0x39, 0x76, 0xa3, 0x1b, 0x2b, 0x89, 0x5c, 0xd7, 0xdb, 0x03},
> +	{0x21, 0x46, 0xfe, 0xa6, 0xec, 0x24, 0x01, 0x59, 0xf4, 0xd4, 0x03},
> +	{0xe9, 0x78, 0x9c, 0xb0, 0x8a, 0x56, 0x64, 0x4f, 0x8d, 0xb0, 0x02},
> +	{0x60, 0x6e, 0x57, 0xee, 0xec, 0x18, 0xa8, 0x11, 0xb5, 0xf8, 0x02},
> +	{0x5a, 0x14, 0x68, 0xe9, 0x1d, 0x06, 0xb9, 0x5f, 0x68, 0xdc, 0x03},
> +};
> +
> +static unsigned char cx2072x_drc_array[MAX_DRC_REGS] = {
> +	0x65, 0x55, 0x3C, 0x01, 0x05, 0x39, 0x76, 0x1A, 0x00
> +};

Just use the chip defaults, this avoids any confusion or debate about
the values in the kernel.

> +#define get_cx2072x_priv(_codec_) \
> +	((struct cx2072x_priv *) snd_soc_codec_get_drvdata(_codec_))

There is no need to cast void pointers, this will at most hide actual
errors, and there's no real need for the macro at all really.

> +	{ 36864000, 7 },/* Don't use div 7*/

Does something enforce that?  Also coding style, missing space.

> +static const struct reg_default cx2072x_reg_defaults[] = {
> +	{ 0x0414, 0x00000003 },	/*2072X_AFG_POWER_STATE */

We have defines for the register names which are helpfully commented
here, why not just actually use the defines?

> +	if (reg == CX2072X_UM_INTERRUPT_CRTL_E) {
> +		/* Update the MSB byte only */
> +		reg += 3;
> +		size = 1;
> +		value >>= 24;
> +	}

Use a switch here in case you find any more magic registers.

> +	ret = i2c_master_send(client, buf, size + 2);
> +	if (ret == size + 2) {
> +		ret =  0;
> +	} else if (ret < 0) {
> +		dev_err(dev,
> +			"I2C write address failed\n");

Please print the error code, it makes life easier for people looking at
logs with errors.

> +static unsigned int get_div_from_mclk(unsigned int mclk)
> +{
> +	unsigned int div = 8;
> +	int i = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(MCLK_PRE_DIV); i++) {
> +		if (mclk <= MCLK_PRE_DIV[i].mclk) {
> +			div = MCLK_PRE_DIV[i].div;
> +			break;
> +		}
> +	}
> +	return div;
> +}

Why is this function in the middle of all the register I/O functions?

> +static int cx2072x_config_headset_det(struct cx2072x_priv *cx2072x)
> +{
> +	const int interrupt_gpio_pin = 1;
> +
> +	dev_dbg(cx2072x->dev,
> +		"Configure interrupt pin: %d\n", interrupt_gpio_pin);
> +	/*No-sticky input type*/

Coding style.  There's *lots* of really obvious and bad coding style
problems throughout the driver, this is really not good - as well as
making things harder to read poor coding style is normally a good
indicator that there are other problems.

> +	/* support both nokia and apple headset set. Monitor time = 275 ms*/
> +	regmap_write(cx2072x->regmap, CX2072X_DIGITAL_TEST15, 0x73);
> +
> +	/* Disable TIP detection*/
> +	regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST12, 0x300);
> +
> +	/* Switch MusicD3Live pin to GPIO */
> +	regmap_write(cx2072x->regmap, CX2072X_DIGITAL_TEST1, 0);

These look awfully like they should be configured by the system
integrator via DT.

> +	regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8,
> +		(unsigned char)int_div & 0xffff);

This cast is worrying, why is it needed?

> +
> +	/* clock inversion */
> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	case SND_SOC_DAIFMT_NB_NF:
> +		is_frame_inv = is_i2s ? 1 : 0;
> +		is_bclk_inv = is_i2s ? 1 : 0;

Please don't abuse the ternery operator like this, write normal logic
statements with straightforward assignments.  Readers will thank you.

> +	regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);

Please don't use Hungarian notation.

> +	/*Enables bclk and EAPD pin*/
> +	if (cx2072x->rev_id == CX2072X_REV_A2)
> +		regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_BIOS_TEST2,
> +			0x84, 0xFF);
> +	else
> +		regmap_write(cx2072x->regmap, CX2072X_DIGITAL_BIOS_TEST2,
> +			regDBT2.ulVal);

Again, switch statement.

> +static int portg_power_ev(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		cx2072x_update_dsp(codec);
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +static int cx2072x_plbk_dsp_info(struct snd_kcontrol *kcontrol,

Blank lines between functions.

> +	mutex_lock(&cx2072x->eq_coeff_lock);
> +	memcpy(cache, param, CX2072X_PLBK_EQ_COEF_LEN);
> +	cx2072x->plbk_dsp_changed = true;

Shouldn't this involve a memcmp() to check if the new value is different?

> +static const struct snd_kcontrol_new cx2072x_snd_controls[] = {
> +
> +	SOC_DOUBLE_R_TLV("PortD Boost", CX2072X_PORTD_GAIN_LEFT,
> +	CX2072X_PORTD_GAIN_RIGHT, 0, 3, 0, boost_tlv),

All volume controls should end in Volume so userspace knows how to
handle them.  Also please use normal kernel indentation for the
continuation lines.

> +	SOC_DOUBLE_R("DAC1 Mute", CX2072X_DAC1_AMP_GAIN_LEFT,
> +		CX2072X_DAC1_AMP_GAIN_RIGHT, 7,  1, 0),

All boolean controls should end in Switch so userspace knows how to
handle them.

> +	CX2072X_PLBK_EQ_COEF("DACL EQ 0", 0, 0),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 1", 0, 1),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 2", 0, 2),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 3", 0, 3),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 4", 0, 4),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 5", 0, 5),
> +	CX2072X_PLBK_EQ_COEF("DACL EQ 6", 0, 6),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 0", 1, 0),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 1", 1, 1),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 2", 1, 2),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 3", 1, 3),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 4", 1, 4),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 5", 1, 5),
> +	CX2072X_PLBK_EQ_COEF("DACR EQ 6", 1, 6),

This looks like it should be two binary controls, one per EQ.

> +int cx2072x_hs_jack_report(struct snd_soc_codec *codec)
> +{

> +}
> +EXPORT_SYMBOL_GPL(cx2072x_hs_jack_report);

We have a standard jack detection interface in the kernel, the driver
should be using it rather than writing its own.

> +static int cx2072x_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +	unsigned int rx_mask, int slots, int slot_width)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct cx2072x_priv *cx2072x = snd_soc_codec_get_drvdata(codec);
> +
> +	if (slots == 0)
> +		goto out;
> +
> +
> +	switch (rx_mask) {
> +	case 1 ... 0xf:
> +	default:
> +		return -EINVAL;
> +	}

So all possible values are invalid?  That doesn't seemw right...

> +	switch (clk_id) {
> +	case CX2072X_MCLK_EXTERNAL_PLL:
> +		if (cx2072x->mclk && clk_set_rate(cx2072x->mclk, freq))
> +			return -EINVAL;

Please write sensible error handling, log what went wrong if you get an
error and pass back error codes you get from other APIs.  This will make
it a lot easier for users to figure out what's happened.

> +#define CX2072X_DAPM_SUPPLY_S(wname, wsubseq, wreg, wshift, wmask,  won_val, \
> +	woff_val, wevent, wflags) \
> +	{.id = snd_soc_dapm_supply, .name = wname, .kcontrol_news = NULL, \
> +	.num_kcontrols = 0, .reg = wreg, .shift = wshift, .mask = wmask, \
> +	.on_val = won_val, .off_val = woff_val, \
> +	.subseq = wsubseq, .event = wevent, .event_flags = wflags}

There appears to be nothing device specific in here, why is this not
being added as a generic supply type?

> +static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x)
> +{
> +
> +	regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);

Coding style, random blank line hre.

> +	/* reduce the jack monitor time*/
> +	regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15,
> +	0x00, 0x06);

Coding style, please intent contiuation lines...

> +	cx2072x_config_headset_det(cx2072x);
> +	/* configre PortC as input device */
> +	regmap_update_bits(cx2072x->regmap, CX2072X_PORTC_PIN_CTRL,
> +		0x20, 0x20);

...this isn't even consistent with the rest of the same function :(

> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +		/* Enable Headset Mic Bias */
> +		if (cx2072x->is_biason == 0)
> +			cx2072x->is_biason = 1;
> +		break;

This looks worrying but since nothing else uses that driver data I'm
guessing it doesn't actually do anything.

> +	case SND_SOC_BIAS_PREPARE:
> +		if (old_level == SND_SOC_BIAS_STANDBY) {
> +			if (cx2072x->mclk) {
> +				dev_dbg(cx2072x->dev, "Turn on MCLK\n");
> +				ret = clk_prepare_enable(cx2072x->mclk);
> +				if (ret)
> +					return ret;
> +			}
> +		}
> +		break;
> +	case SND_SOC_BIAS_STANDBY:
> +
> +		if (old_level == SND_SOC_BIAS_OFF) {
> +			if (cx2072x->mclk) {
> +				dev_dbg(cx2072x->dev, "Turn on MCLK\n");
> +				ret = clk_prepare_enable(cx2072x->mclk);
> +				if (ret)
> +					return ret;
> +			}

So we enable MCLK both when coming out of _OFF and when coming out of
_STANDBY?  Why (and note that we're missing some disables here...

> +	regmap_read(cx2072x->regmap, CX2072X_VENDOR_ID, &ven_id);
> +	regmap_read(cx2072x->regmap, CX2072X_REVISION_ID, &cx2072x->rev_id);
> +	dev_dbg(codec->dev, "codec version: %08x,%08x\n",
> +		ven_id, cx2072x->rev_id);

Log this at info level.

> +#ifdef CONFIG_PM
> +static int cx2072x_runtime_suspend(struct device *dev)
> +{
> +	struct cx2072x_priv *cx2072x = dev_get_drvdata(dev);
> +
> +	cx2072x_set_bias_level(cx2072x->codec, SND_SOC_BIAS_OFF);

Let the framework take care of managing bias level for you.

> +static bool cx2072x_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {

Why is this nowhere near the rest of the regmap code?

> +static int cx2072x_i2c_probe(struct i2c_client *i2c,
> +	const struct i2c_device_id *id)
> +{
> +	int ret = -1;
> +	struct cx2072x_priv *cx2072x;
> +
> +	cx2072x = (struct cx2072x_priv *)devm_kzalloc(
> +		&i2c->dev, sizeof(struct cx2072x_priv), GFP_KERNEL);

No need to cast away from void.

> +	if (cx2072x == NULL) {
> +		dev_err(&i2c->dev, "Out of memory!\n");

kzalloc() will already be very noisy if this happens.

> +static const struct i2c_device_id cx2072x_i2c_id[] = {
> +	{ "cx20721", 0 },
> +	{ "cx20723", 0 },
> +	{ "14F10720", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, cx2072x_i2c_id);

One of those looks like an ACPI ID not an I2C ID.

> +
> +static const struct of_device_id cx2072x_of_match[] = {
> +	{ .compatible = "cnxt,cx20721", },
> +	{ .compatible = "cnxt,cx20723", },
> +	{ .compatible = "cnxt,cx7601", },
> +	{}

Why is cx7601 not an I2C ID?

> +#ifdef CONFIG_ACPI
> +		.acpi_match_table = ACPI_PTR(cx2072x_acpi_match),
> +#endif

No need for the defines here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20161219/c95c4db4/attachment.sig>