Re: [PATCH v1 1/1] Add a new kcontrol for phase calib, remove unnecessary header file, make code more comply with linux coding style

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4354 bytes --]

On Mon, Aug 01, 2022 at 10:59:39AM +0800, Kevin Lu wrote:

This looks mostly good however there are a few things that need looking
at:

> Signed-off-by: Kevin Lu <luminlong@139.com>
> ---
>  tlv320adcx140.c | 1218 +++++++++++++++++++++++++++++++++++++++++++++++
>  tlv320adcx140.h |  157 ++++++
>  2 files changed, 1375 insertions(+)
>  create mode 100644 tlv320adcx140.c
>  create mode 100644 tlv320adcx140.h

This is a new driver which isn't what the changelog says at all, and it
lacks any Kconfig or Makefile updates so the new driver won't be built.

Please format your changelog as covered in submitting-patches.rst, both
accurately describing what's in the patch and following the subject line
style for the subsystem.

> +static const char * const resistor_text[] = {
> +	"2.5 kOhm", "10 kOhm", "20 kOhm"
> +};
> +
> +static SOC_ENUM_SINGLE_DECL(in1_resistor_enum, ADCX140_CH1_CFG0, 2,
> +			    resistor_text);
> +static SOC_ENUM_SINGLE_DECL(in2_resistor_enum, ADCX140_CH2_CFG0, 2,
> +			    resistor_text);
> +static SOC_ENUM_SINGLE_DECL(in3_resistor_enum, ADCX140_CH3_CFG0, 2,
> +			    resistor_text);
> +static SOC_ENUM_SINGLE_DECL(in4_resistor_enum, ADCX140_CH4_CFG0, 2,
> +			    resistor_text);

Is this something that can usefully be varied at runtime or does it
depend on the board design?

> +	SND_SOC_DAPM_MIXER("Output Mixer", SND_SOC_NOPM, 0, 0,
> +		&adcx140_output_mixer_controls[0],
> +		ARRAY_SIZE(adcx140_output_mixer_controls)),

Don't put all the mixer controls into an array and then index into it by
magic numbers, just declare variables for all the individual controls.
This is much more readable and less error prone.

> +static const char * const phase_calib_text[] = {
> +	"Disable",
> +	"Enable"
> +};
> +
> +static const struct soc_enum phase_calib_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(phase_calib_text), phase_calib_text),
> +};

This is a simple on/off switch, it should be a normal control with a
name ending in Switch not an enum.

> +static int adcx140_phase_calib_get(struct snd_kcontrol *pKcontrol,
> +	struct snd_ctl_elem_value *pValue)
> +{
> +	struct snd_soc_component *codec =
> +		snd_soc_kcontrol_component(pKcontrol);
> +	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(codec);
> +
> +	pValue->value.integer.value[0] = adcx140->phase_calib_on;

Please follow the normal kernel coding style for variable and parameter
names, don't use hungarian notation.

> +static int adcx140_phase_calib_put(struct snd_kcontrol *pKcontrol,
> +	struct snd_ctl_elem_value *pValue)
> +{
> +	struct snd_soc_component *codec
> +		= snd_soc_kcontrol_component(pKcontrol);
> +	struct adcx140_priv *adcx140 = snd_soc_component_get_drvdata(codec);
> +
> +	adcx140->phase_calib_on = pValue->value.integer.value[0];
> +
> +	return 0;
> +}

This should return 1 if the value changes, the mixer-test selftest will
check for this and a number of other issues - you should ensure your
driver passes that cleanly.

> +	/* signal polarity */
> +	switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> +	case SND_SOC_DAIFMT_IB_NF:
> +	case SND_SOC_DAIFMT_IB_IF:
> +		inverted_bclk = !inverted_bclk;
> +		break;
> +	case SND_SOC_DAIFMT_NB_IF:
> +		iface_reg1 |= ADCX140_FSYNCINV_BIT;
> +		break;
> +	case SND_SOC_DAIFMT_NB_NF:
> +		break;

This is treating _IB_IF and _IB_NF identically which is clearly wrong.
It looks like the ADCX140_FSYNCINV_BIT setting needs to be handled.

> +	adcx140->supply_areg = devm_regulator_get_optional(adcx140->dev,
> +							   "areg");
> +	if (IS_ERR(adcx140->supply_areg)) {
> +		if (PTR_ERR(adcx140->supply_areg) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +
> +		adcx140->supply_areg = NULL;
> +	} else {
> +		ret = regulator_enable(adcx140->supply_areg);
> +		if (ret) {
> +			dev_err(adcx140->dev, "Failed to enable areg\n");
> +			return ret;
> +		}
> +
> +		ret = devm_add_action_or_reset(&i2c->dev,
> +			adcx140_disable_regulator, adcx140);
> +		if (ret)
> +			return ret;
> +	}

Unless the hardware can work without power this is buggy.  The driver
should request and unconditionally use all supplies the device
physically has, it should only use _get_optional() for supplies which
may be physically absent in the system for some reason.  For example
with the TLV320ADC6140 it looks like the driver shuld be requesting
AVDD and IOVDD and that AREG is an output from the device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]