On Sat, Dec 17, 2016 at 03:52:33PM +0800, simon.ho.cnxt(a)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.
