On Tue, Dec 20, 2016 at 12:20 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Sat, Dec 17, 2016 at 03:52:33PM +0800, simon.ho.cnxt@xxxxxxxxx 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.
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
