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

[Simon Ho]

On Mon, Dec 19, 2016 at 04:20:41PM +0000, Mark Brown wrote:
> 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.
> 

Understood, will remove the following settings from code.

> > +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.
> 

Undrstood, use the chip defaults instead.

> > +#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.
> 

Okay, will remove this.

> > +	{ 36864000, 7 },/* Don't use div 7*/
> 
> Does something enforce that?  Also coding style, missing space.

No, we can remove the unecessary comment here.   

> 
> > +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?
> 

Okay, will use names instead.

> > +	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.
> 

Aggreed, but there is only one magic register.

> > +	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.
> 

Okay, will remove it.

> > +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?
> 

Will move it to a better place.

> > +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.
> 

Sorry. I copied the code from Windows... and I will check it more carefully.

> > +	/* 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.

They are optimzed values for all scenarios. Don't need be configured by
the system integrator.

> 
> > +	regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8,
> > +		(unsigned char)int_div & 0xffff);
> 
> This cast is worrying, why is it needed?
> 

No, we don't need the cast.

> > +
> > +	/* 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.
> 

You are right.

> > +	regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);
> 
> Please don't use Hungarian notation.
> 

Sorry, I was not aware of this error.

> > +	/*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.

A2 is a pre-release version. There is no any product use this chip version, so 
that I will the A2 support from code.

> 
> > +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.
> 

Okay, will do.

> > +	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?

Good idea. will add a check here.

> 
> > +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.
> 

Understood, thanks for suggestion.

> > +	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.
> 

Understood, thanks for your suggestion.

> > +	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.
> 

Sorry, I can't get it. There are 7 bands EQ for each channels. Do you
mean "one per channel"?

> > +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.
> 

I thought the jack detection should be platfrom dependent. So that codec
driver just expose the callback function to machine driver to allow machine
driver can get current jack status from codec and report to user space.

Do you meant the codec driver need to proccess the jack detetction, ISR and
jack report?

> > +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...
> 

As I mentioned in my commit comment, the TDM support is not implemented yet.
I will remove this from code.

> > +	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.
> 

Thanks, a good suggestion.

> > +#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?
> 

The code was developed on an old kernel version which has no this DAPM macro.
Anyway, I will use generic supply type instead.

> > +static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x)
> > +{
> > +
> > +	regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);
> 
> Coding style, random blank line hre.
> 

Sorry, will fix it.

> > +	/* reduce the jack monitor time*/
> > +	regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15,
> > +	0x00, 0x06);
> 
> Coding style, please intent contiuation lines...
> 

Understood, thanks.

> > +	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 :(
> 

Sorry, I was not aware of that.

> > +	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.
> 

Yes, you are right. It doesn't do anything. Wll remove it.

> > +	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...
> 

Oops, thanks for caching this.

> > +	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.

Okay.

> 
> > +#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.

Understood, thanks.

> 
> > +static bool cx2072x_readable_register(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> 
> Why is this nowhere near the rest of the regmap code?
> 

Okay, will move it to a right place.

> > +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.
> 

Understood.

> > +	if (cx2072x == NULL) {
> > +		dev_err(&i2c->dev, "Out of memory!\n");
> 
> kzalloc() will already be very noisy if this happens.
> 

Understood.

> > +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.

Oops, will remove the ACPI one.

> 
> > +
> > +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?

Thanks for catching this. It should be one of I2C ID.

> 
> > +#ifdef CONFIG_ACPI
> > +		.acpi_match_table = ACPI_PTR(cx2072x_acpi_match),
> > +#endif
> 
> No need for the defines here.

Understood and many thanks for your comments. I Will send another patch
upon I fix above *errors*