ASoC: Add driver for CX2072X CODEC
Subsystems (2)
cover ASoC: Add driver for CX2072X CODEC
From: Simon Ho <[email protected]>
Re-send the patch because my previous email was sent by a non-member email account and are being held. So please disregard my previous email if you saw it. Sorry if I caused any trouble.
This patch adds support for Conexant CX2072X codec driver.
The CX2072X is a ultra low power stereo audio codec supports I2S/TDM host interface with EQ, DRC features in playback mode.
Featues of CX2072X codec:
* Two 24 bits DACs and DACs.
* Stereo Headphone AMP.
* 2.8W per channel class-D output.
* Integrated seven bands per channel EQ and DRC.
* Fully integrated headset support with detect/switch.
* Stereo digital microphone for array applications.
* S/PDIF output.
* Bi-directional GPIOs.
* Support analog and digital PC Beeep.
* One headset button support.
* Supports a wide variety of host interfaces. -wide variety of I2S and similar bit-stream formats with word lengths of up to 24 bits.
-TDM stream supports up to 4 channels.
* AEC loopback support.
Simon Ho (2):
ASoC: cx2072x: Add DT bingings documentation for CX2072X CODEC
ASoC: cx2072x Add driver for CX2072X CODEC
.../devicetree/bindings/sound/cx2072x.txt | 36 + sound/soc/codecs/Kconfig | 5 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/cx2072x.c | 2080 ++++++++++++++++++++ sound/soc/codecs/cx2072x.h | 322 +++ 5 files changed, 2445 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cx2072x.txt create mode 100644 sound/soc/codecs/cx2072x.c create mode 100644 sound/soc/codecs/cx2072x.h
Signature · 1 line
2.7.4
PATCH 1/2 ASoC: cx2072x: Add DT bingings documentation for CX2072X CODEC
From: Simon Ho <[email protected]>
Initial version of CX2072X device tree bindings document.
--- .../devicetree/bindings/sound/cx2072x.txt | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/cx2072x.txt
Documentation/devicetree/bindings/sound/cx2072x.txt new file +36 -0
@@ -0,0 +1,36 @@ +Conexant CX20721/CX20723/CX7601 audio CODEC + +The devices support I2C only. + +Required properties: + + - compatible : One of "cnxt,cx20721", "cnxt,cx20723", "cnxt,cx7601". + + - reg : the I2C address of the device for I2C, it should be <0x33> + +Optional properties: + + - clocks : phandle and clock specifier for codec MCLK. + - clock-names : Clock name string for 'clocks' attribute, should be "mclk". + +CODEC output pins: + "PORTA" - Headphone + "PORTG" - Class-D output + "PORTE" - Line out + +CODEC output pins for Conexant DSP chip: + "AEC REF" - AEC reference signal + +CODEC input pins: + "PORTB" - Analog mic + "PORTC" - Digital mic + "PORTD" - Headset mic + +Example: + +codec: cx20721@33 { + compatible = "cnxt,cx20721"; + reg = <0x33>; + clocks = <&sco>; + clock-names = "mclk"; +};
Signature · 1 line
2.7.4
A message that is not in the archive (sent off-list or not yet received): 233120a71738fcef7b56f27fade0942f432e38ea.1481903061.git.simon.ho@conexant.com
Mark Brown
maintainerRe: [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC
Quoted 1 line · On Sat, Dec 17, 2016 at 03:52:33PM +0800, [email protected] 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.
Review
Quoted 13 lines
(quoted diff) +13
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+#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.
Review
Quoted 1 line
(quoted diff) +1
-0+ { 36864000, 7 },/* Don't use div 7*/
Does something enforce that? Also coding style, missing space.
Review
Quoted 2 lines
(quoted diff) +2
-0+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?
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 13 lines
(quoted diff) +13
-0+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?
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* 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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8, + (unsigned char)int_div & 0xffff);
This cast is worrying, why is it needed?
Review
Quoted 6 lines
(quoted diff) +6
-0+ + /* 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.
Review
Quoted 1 line
(quoted diff) +1
-0+ regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);
Please don't use Hungarian notation.
Review
Quoted 7 lines
(quoted diff) +7
-0+ /*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.
Quoted 5 lines
(quoted diff) +5
-0+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); +
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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?
Review
Quoted 4 lines
(quoted diff) +4
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 4 lines
(quoted diff) +4
-0+ 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),
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 5 lines
(quoted diff) +4
-0+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.
Quoted 5 lines
(quoted diff) +5
-0+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);
Review
Quoted 10 lines
(quoted diff) +10
-0+ + 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...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+#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?
Review
Quoted 4 lines
(quoted diff) +4
-0+static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x) +{ + + regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);
Coding style, random blank line hre.
Review
Quoted 3 lines
(quoted diff) +3
-0+ /* reduce the jack monitor time*/ + regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15, + 0x00, 0x06);
Coding style, please intent contiuation lines...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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 :(
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 9 lines
(quoted diff) +9
-0+ 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; + } + }
Review
Quoted 10 lines
(quoted diff) +10
-0+ 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...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+#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.
Review
Quoted 3 lines
(quoted diff) +3
-0+static bool cx2072x_readable_register(struct device *dev, unsigned int reg) +{ + switch (reg) {
Why is this nowhere near the rest of the regmap code?
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (cx2072x == NULL) { + dev_err(&i2c->dev, "Out of memory!\n");
kzalloc() will already be very noisy if this happens.
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ +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?
Review
Quoted 3 lines
(quoted diff) +3
-0+#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(cx2072x_acpi_match), +#endif
No need for the defines here.
Re: [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC
Thanks Simon, this will help fix https://bugzilla.kernel.org/show_bug.cgi?id=115531
It looks like this patch never reached the mailing list? the archives show patches 0 and 1 but not patch 2/2.
Quoted 315 lines · Mark Brown
Quoted 1 line · On Sat, Dec 17, 2016 at 03:52:33PM +0800, [email protected] 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.
Review
Quoted 13 lines
(quoted diff) +13
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+#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.
Review
Quoted 1 line
(quoted diff) +1
-0+ { 36864000, 7 },/* Don't use div 7*/Does something enforce that? Also coding style, missing space.
Review
Quoted 2 lines
(quoted diff) +2
-0+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?
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 13 lines
(quoted diff) +13
-0+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?
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* 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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8, + (unsigned char)int_div & 0xffff);This cast is worrying, why is it needed?
Review
Quoted 6 lines
(quoted diff) +6
-0+ + /* 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.
Review
Quoted 1 line
(quoted diff) +1
-0+ regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);Please don't use Hungarian notation.
Review
Quoted 7 lines
(quoted diff) +7
-0+ /*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.
Quoted 5 lines
(quoted diff) +5
-0+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); +Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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?Review
Quoted 4 lines
(quoted diff) +4
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 4 lines
(quoted diff) +4
-0+ 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),Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 4 lines
(quoted diff) +4
-0+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.
Quoted 5 lines
(quoted diff) +5
-0+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);Review
Quoted 10 lines
(quoted diff) +10
-0+ + 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...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+#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?
Review
Quoted 4 lines
(quoted diff) +4
-0+static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x) +{ + + regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);Coding style, random blank line hre.
Review
Quoted 3 lines
(quoted diff) +3
-0+ /* reduce the jack monitor time*/ + regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15, + 0x00, 0x06);Coding style, please intent contiuation lines...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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 :(
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 9 lines
(quoted diff) +9
-0+ 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; + } + }Review
Quoted 10 lines
(quoted diff) +10
-0+ 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
_OFFand when coming out of_STANDBY? Why (and note that we're missing some disables here...Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+#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.
Review
Quoted 3 lines
(quoted diff) +3
-0+static bool cx2072x_readable_register(struct device *dev, unsigned int reg) +{ + switch (reg) {Why is this nowhere near the rest of the regmap code?
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (cx2072x == NULL) { + dev_err(&i2c->dev, "Out of memory!\n");
kzalloc()will already be very noisy if this happens.Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ +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?
Review
Quoted 3 lines
(quoted diff) +3
-0+#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(cx2072x_acpi_match), +#endifNo need for the defines here.
_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC
Hi Pierre,
Long story, I made a mistake I registered mailing list with my company email but
I sent the patch by a non-member gmail account.
I re-send patch after registration, but I still got the following message. O_O
----
Is being held until the list moderator can review it for approval.
The reason it is being held:
Message body is too big: 79630 bytes with a limit of 60 KB
Either the message will get posted to the list, or you will receive notification of the moderator's decision. If you would like to cancel this posting, please visit the following URL
--
Thanks
Simon
Quoted 398 lines · Pierre-Louis Bossart
Thanks Simon, this will help fix https://bugzilla.kernel.org/
show_bug.cgi?id=115531
It looks like this patch never reached the mailing list? the archives show patches 0 and 1 but not patch 2/2.On 12/19/2016 10:20 AM, Mark Brown wrote:
On Sat, Dec 17, 2016 at 03:52:33PM +0800, [email protected] 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.
(quoted diff) +13
-0+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.
(quoted diff) +2
-0+#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.
(quoted diff) +1
-0+ { 36864000, 7 },/* Don't use div 7*/Does something enforce that? Also coding style, missing space.
(quoted diff) +2
-0+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?
(quoted diff) +6
-0+ 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.
(quoted diff) +6
-0+ 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.
(quoted diff) +13
-0+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?
(quoted diff) +7
-0+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.
(quoted diff) +8
-0+ /* 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.
(quoted diff) +2
-0+ regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8, + (unsigned char)int_div & 0xffff);This cast is worrying, why is it needed?
(quoted diff) +6
-0+ + /* 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.
(quoted diff) +1
-0+ regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);Please don't use Hungarian notation.
(quoted diff) +7
-0+ /*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.
(quoted diff) +15
-0+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.
(quoted diff) +3
-0+ 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?(quoted diff) +4
-0+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.
(quoted diff) +2
-0+ 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.
(quoted diff) +14
-0+ 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.
(quoted diff) +4
-0+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.
(quoted diff) +15
-0+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...
(quoted diff) +4
-0+ 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.
(quoted diff) +6
-0+#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?
(quoted diff) +4
-0+static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x) +{ + + regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);Coding style, random blank line hre.
(quoted diff) +3
-0+ /* reduce the jack monitor time*/ + regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15, + 0x00, 0x06);Coding style, please intent contiuation lines...
(quoted diff) +4
-0+ 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 :(
(quoted diff) +6
-0+ 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.
(quoted diff) +19
-0+ 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
_OFFand when coming out of_STANDBY? Why (and note that we're missing some disables here...(quoted diff) +4
-0+ 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.
(quoted diff) +6
-0+#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.
(quoted diff) +3
-0+static bool cx2072x_readable_register(struct device *dev, unsigned int reg) +{ + switch (reg) {Why is this nowhere near the rest of the regmap code?
(quoted diff) +8
-0+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.
(quoted diff) +2
-0+ if (cx2072x == NULL) { + dev_err(&i2c->dev, "Out of memory!\n");
kzalloc()will already be very noisy if this happens.(quoted diff) +7
-0+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.
(quoted diff) +6
-0+ +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?
(quoted diff) +3
-0+#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(cx2072x_acpi_match), +#endifNo need for the defines here.
_______________________________________________
Alsa-devel mailing [email protected]://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Re: [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC
In reply to Mark BrownQuoted 7 lines · Mark Brown
Quoted 1 line · On Sat, Dec 17, 2016 at 03:52:33PM +0800, [email protected] 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.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+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},
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+}; + +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.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+#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.
Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ { 36864000, 7 },/* Don't use div 7*/Does something enforce that? Also coding style, missing space.
No, we can remove the unecessary comment here.
Quoted 7 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+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.
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+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) {
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+static int cx2072x_config_headset_det(struct cx2072x_priv *cx2072x) +{ + const int interrupt_gpio_pin = 1; +
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ /* 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.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ 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.
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ + /* 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.
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);Please don't use Hungarian notation.
Sorry, I was not aware of this error.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+ /*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.
Quoted 10 lines
Quoted 10 lines
(quoted diff) +9
-0+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;
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ default: + break; + } + return 0; +} +static int cx2072x_plbk_dsp_info(struct snd_kcontrol *kcontrol,Blank lines between functions.
Okay, will do.
Quoted 5 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
Quoted 10 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+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.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+ 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),
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-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"?
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +4
-0+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?
Quoted 9 lines
Quoted 9 lines
(quoted diff) +9
-0+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; +
Quoted 9 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ + 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.
Quoted 9 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+#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.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+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.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ /* reduce the jack monitor time*/ + regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15, + 0x00, 0x06);Coding style, please intent contiuation lines...
Understood, thanks.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Quoted 10 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 14 lines
Quoted 14 lines
(quoted diff) +14
-0+ 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) {
Quoted 9 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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
_OFFand when coming out of_STANDBY? Why (and note that we're missing some disables here...
Oops, thanks for caching this.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+#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.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+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.
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (cx2072x == NULL) { + dev_err(&i2c->dev, "Out of memory!\n");
kzalloc()will already be very noisy if this happens.
Understood.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +6
-0+ +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.
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +3
-0+#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(cx2072x_acpi_match), +#endifNo need for the defines here.
Understood and many thanks for your comments. I Will send another patch upon I fix above *errors*
Re: [PATCH 2/2] ASoC: cx2072x Add driver for CX2072X CODEC
In reply to Mark BrownQuoted 358 lines · Mark Brown
Quoted 1 line · On Sat, Dec 17, 2016 at 03:52:33PM +0800, [email protected] 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.
Quoted 1 line
+static unsigned char cx2072x_eq_coeff_array[MAX_EQ_BAND][MAC_EQ_COEFF]
= {
Review
Quoted 12 lines
(quoted diff) +12
-0+ {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.
Review
Quoted 2 lines
(quoted diff) +2
-0+#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.
Quoted 1 line
+ { 36864000, 7 },/* Don't use div 7*/
Does something enforce that? Also coding style, missing space.
Review
Quoted 2 lines
(quoted diff) +2
-0+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?
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Review
Quoted 13 lines
(quoted diff) +13
-0+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?
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Quoted 1 line
+ /* support both nokia and apple headset set. Monitor time = 275
ms*/
Review
Quoted 7 lines
(quoted diff) +7
-0+ 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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ regmap_write(cx2072x->regmap, CX2072X_ANALOG_TEST8, + (unsigned char)int_div & 0xffff);This cast is worrying, why is it needed?
Review
Quoted 6 lines
(quoted diff) +6
-0+ + /* 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.
Quoted 1 line
+
regmap_write(cx2072x->regmap, CX2072X_I2SPCM_CONTROL1, reg1.ulVal);Please don't use Hungarian notation.
Review
Quoted 3 lines
(quoted diff) +3
-0+ /*Enables bclk and EAPD pin*/ + if (cx2072x->rev_id == CX2072X_REV_A2) + regmap_update_bits(cx2072x->regmap,
CX2072X_DIGITAL_BIOS_TEST2,Review
Quoted 4 lines
(quoted diff) +4
-0+ 0x84, 0xFF); + else + regmap_write(cx2072x->regmap, CX2072X_DIGITAL_BIOS_TEST2, + regDBT2.ulVal);Again, switch statement.
Quoted 5 lines
(quoted diff) +5
-0+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); +Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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?Review
Quoted 4 lines
(quoted diff) +4
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ 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.
Quoted 4 lines
(quoted diff) +4
-0+ 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),Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Review
Quoted 5 lines
(quoted diff) +4
-0+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.
Quoted 1 line
+static int cx2072x_set_tdm_slot(struct
snd_soc_dai*dai, unsigned int
tx_mask,Quoted 4 lines
(quoted diff) +4
-0+ 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);Review
Quoted 10 lines
(quoted diff) +10
-0+ + 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...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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.
Quoted 1 line
+#define CX2072X_DAPM_SUPPLY_S(wname, wsubseq, wreg, wshift, wmask,
won_val, \Review
Quoted 5 lines
(quoted diff) +5
-0+ 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?
Review
Quoted 4 lines
(quoted diff) +4
-0+static void cx2072x_sw_reset(struct cx2072x_priv *cx2072x) +{ + + regmap_write(cx2072x->regmap, CX2072X_AFG_FUNCTION_RESET, 0x01);Coding style, random blank line hre.
Review
Quoted 3 lines
(quoted diff) +3
-0+ /* reduce the jack monitor time*/ + regmap_update_bits(cx2072x->regmap, CX2072X_DIGITAL_TEST15, + 0x00, 0x06);Coding style, please intent contiuation lines...
Review
Quoted 4 lines
(quoted diff) +4
-0+ 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 :(
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 9 lines
(quoted diff) +9
-0+ 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; + } + }Review
Quoted 10 lines
(quoted diff) +10
-0+ 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
_OFFand when coming out of_STANDBY? Why (and note that we're missing some disables here...Review
Quoted 2 lines
(quoted diff) +2
-0+ regmap_read(cx2072x->regmap, CX2072X_VENDOR_ID, &ven_id); + regmap_read(cx2072x->regmap, CX2072X_REVISION_ID,
&cx2072x->rev_id);Review
Quoted 2 lines
(quoted diff) +2
-0+ dev_dbg(codec->dev, "codec version: %08x,%08x\n", + ven_id, cx2072x->rev_id);Log this at info level.
Review
Quoted 6 lines
(quoted diff) +6
-0+#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.
Quoted 1 line
+static bool cx2072x_readable_register(struct device *dev, unsigned int
reg)
Review
Quoted 2 lines
(quoted diff) +2
-0+{ + switch (reg) {Why is this nowhere near the rest of the regmap code?
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (cx2072x == NULL) { + dev_err(&i2c->dev, "Out of memory!\n");
kzalloc()will already be very noisy if this happens.Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Review
Quoted 6 lines
(quoted diff) +6
-0+ +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?
Review
Quoted 3 lines
(quoted diff) +3
-0+#ifdef CONFIG_ACPI + .acpi_match_table = ACPI_PTR(cx2072x_acpi_match), +#endifNo need for the defines here.