[PATCH 2/2] ASoC: codec: tlv320adc3xxx: New codec driver

[Mark Brown]

On Mon, Oct 04, 2021 at 11:19:21AM +0200, Ricard Wanderlof wrote:

There's an awful lot of code in here that just doesn't really look like
a normal Linux driver or follow conventions for ASoC, just from a quick
visual overview without actually reading anything it's fairly clear the
driver needs quite a bit of a refresh for mainline.

> +config SND_SOC_TLV320ADC3XXX
> +	tristate "Texas Instruments TLV320ADC3001/3101 audio ADC"
> +        help
> +	 Enable support for Texas Instruments TLV320ADC3001 and TLV320ADC3101
> +	 ADCs.

Indentation seems weird here?

> +++ b/sound/soc/codecs/tlv320adc3xxx.c
> @@ -0,0 +1,1239 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Based on sound/soc/codecs/tlv320aic3x.c by  Vladimir Barinov

Please make the entire comment a C++ one so things look more
intentional.

> +/***************************** INCLUDES ************************************/
> +

These section markers aren't idiomatic.

> +#define PLL_MODE_TEXT(mode) (mode == ADC3XXX_PLL_ENABLE ? "PLL enable" : \
> +			     (mode == ADC3XXX_PLL_BYPASS ? "PLL bypass" : \
> +							   "PLL auto"))

If you need this please just write it as a function with normal
conditional statements for better legibility and type safety.

> +static bool adc3xxx_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case PAGE_SELECT: /* required by regmap implementation */

This is not required by regmap.

> +
> +/* The global Register Initialization sequence Array. During the Audio Driver
> + * Initialization, this array will be utilized to perform the default
> + * initialization of the audio Driver.
> + */
> +static const struct adc3xxx_configs adc3xxx_reg_init[] = {
> +	/* The default (out-of-reset) values in the x_PGA_SEL_x registers
> +	 * disable the inputs by default, but also set the input attenuation
> +	 * to -6 dB by default, so we leave the inputs disabled but set
> +	 * the attenuation to a more natural 0 dB.
> +	 */

These are all perfectly fine defaults, just leave them as they are and
expose the configuration to userspace - what makes sense for one system
may not make sense for another so we just leave things at the hardware
defaults and let userspace configure what it needs.  All the code
relating to setting new defaults should be removed.

> + *----------------------------------------------------------------------------
> + * Function : adc3xxx_get_divs
> + * Purpose  : This function is to get required divisor from the "adc3xxx_divs"
> + *            table.
> + *
> + *----------------------------------------------------------------------------
> + */

If you must include comments like this please follow the standard kernel
documentation style, but I'm really struggling to see any value in them.

> +	dev_info(dev, "mclk = %d, rate = %d, clock mode %u\n",
> +		 mclk, rate, pll_mode);

This is way too verbose, it should at most be dev_dbg().  Same for most
dev_info() in the driver.

> +static const char * const micbias_voltage[] = { "off", "2V", "2.5V", "AVDD" };

This should be configured in the DT, it's going to be a property of the
electrical design of the system.

> +static const char * const linein_attenuation[] = { "0db", "-6dB" };

This is a volume control, it should be a standard volume control with
TLV information.

> +static const char * const dither_dc_offset[] = {
> +	"0mV", "15mV", "30mV", "45mV", "60mV", "75mV", "90mV", "105mV",
> +	 "reserved", "-15mV", "-30mV", "-45mV", "-60mV", "-75mV", "-90mV", "-105mV"
> +};

Use a VALUE_ENUM to prevent the selection of invalid values.

> +/* Creates an array of the Single Ended Widgets*/
> +static const struct soc_enum adc3xxx_enum[] = {
> +	SOC_ENUM_SINGLE(MICBIAS_CTRL, 5, 4, micbias_voltage),
> +	SOC_ENUM_SINGLE(MICBIAS_CTRL, 3, 4, micbias_voltage),

Putting all these into an array just makes everything more error prone
and hard to maintain for no benefit.  Just declare them as variables.

> +static const struct snd_kcontrol_new adc3xxx_snd_controls[] = {
> +	SOC_DOUBLE_R_TLV("PGA Gain Volume", LEFT_APGA_CTRL, RIGHT_APGA_CTRL,
> +			 0, 80, 0, pga_tlv),

s/Gain //

> +	SOC_DOUBLE_R("AGC Enable", LEFT_CHN_AGC_1,
> +		     RIGHT_CHN_AGC_1, 7, 1, 0),

On/off controls should use the standard ALSA naming - Switch.

> +/* Left input selection, Single Ended inputs and Differential inputs */
> +static const struct snd_kcontrol_new left_input_mixer_controls[] = {
> +	SOC_DAPM_SINGLE("IN_1L switch", LEFT_PGA_SEL_1, 1, 0x1, 1),

s/switch/Switch/

> +/* Right input selection from switches */
> +	{"Right Input Selection", "IN_1R switch", "IN_1R"},

Indentation is weird here.

> +/* GPIO control. These are only used when the corresponding GPIO pin is
> + * configured accordingly.
> + */
> +static const struct snd_kcontrol_new adc3xxx_gpio1_out_control[] = {
> +	SOC_SINGLE("GPIO1 Output", GPIO1_CTRL, GPIO_CTRL_OUTPUT_CTRL_SHIFT, 1, 0)
> +};

Remove these, if control is needed for the GPIO implement gpiolib support
for it and let the system control it like any other GPIO.

> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		adc3xxx->master = 1;
> +		clkdir = BCLK_MASTER | WCLK_MASTER;
> +		break;
> +	case SND_SOC_DAIFMT_CBS_CFS:

Please use the modern _CBP_CFP and _CPC_CFC defines.

> +	default:
> +		dev_err(component->dev, "Invalid DAI master/slave interface\n");

Provider/consumer.

> +		/* Switch on NADC Divider */
> +		snd_soc_component_update_bits(component, ADC_NADC,
> +					      ENABLE_NADC, ENABLE_NADC);
> +
> +		/* Switch on MADC Divider */
> +		snd_soc_component_update_bits(component, ADC_MADC,
> +					      ENABLE_MADC, ENABLE_MADC);

Why are these not managed through DAPM?

> +struct snd_soc_dai_driver adc3xxx_dai = {
> +	.name = "tlv320adc3xxx-hifi",
> +	.capture = {
> +		    .stream_name = "Capture",
> +		    .channels_min = 1,
> +		    .channels_max = 2,
> +		    .rates = ADC3xxx_RATES,
> +		    .formats = ADC3xxx_FORMATS,
> +		    },
> +	.ops = &adc3xxx_dai_ops,
> +};
> +EXPORT_SYMBOL_GPL(adc3xxx_dai);

Why is this exported?

> +#define REGISTER_INIT(CODEC, MAP) \
> +	do { \
> +		int i; \
> +		for (i = 0; i < ARRAY_SIZE(MAP); i++) \
> +			snd_soc_component_write(CODEC, MAP[i].reg_offset, \
> +				      MAP[i].reg_val); \
> +	} while (0)

Like I said this code should be removed so it's moot but why s this not
written as a normal function?

> +	}
> +
> +	//adc3xxx_set_bias_level(component, SND_SOC_BIAS_STANDBY);
> +

Just remove commented out code.

> +static int adc3xxx_probe(struct snd_soc_component *component)
> +{
> +	struct adc3xxx_priv *adc3xxx = snd_soc_component_get_drvdata(component);
> +	int ret = 0;
> +
> +	ret = devm_gpio_request(component->dev, adc3xxx->rst_pin, "adc3xxx reset");
> +	if (ret < 0) {

Request resources that are needed at the I2C layer probe so that basic
chip initialisation can happen sooner in boot and so that any probe
deferral doesn't cause us to have to set up and tear down the card.

> +static int adc3xxx_resume(struct snd_soc_component *component)
> +{
> +	snd_soc_component_cache_sync(component);
> +	adc3xxx_set_bias_level(component, SND_SOC_BIAS_STANDBY);

You need to mark the cache as dirty to get the cache sync to do
anything.

> +#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
> +/*

Why is this conditional?

> +	adc3xxx->mclk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(adc3xxx->mclk)) {
> +		if (PTR_ERR(adc3xxx->mclk) != -ENOENT)
> +			return PTR_ERR(adc3xxx->mclk);
> +		/* Make a note that there is no mclk specified. */
> +		adc3xxx->mclk = NULL;

Does the device work without a MCLK?

> +static const struct i2c_device_id adc3xxx_i2c_id[] = {
> +	{"tlv320adc3001", ADC3001},
> +	{"tlv320adc3101", ADC3101},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, adc3xxx_i2c_id);

Extra blank line here.

> +static int __init tlv320adc3xxx_init(void)
> +{
> +	return i2c_add_driver(&adc3xxx_i2c_driver);
> +}
> +
> +static void __exit tlv320adc3xxx_exit(void)
> +{
> +	i2c_del_driver(&adc3xxx_i2c_driver);
> +}
> +
> +module_init(tlv320adc3xxx_init);
> +module_exit(tlv320adc3xxx_exit);

module_i2c_driver().

> +/* Page select register */
> +#define PAGE_SELECT			ADC3xxx_REG(0, 0)
> +/* Software reset register */
> +#define RESET				ADC3xxx_REG(0, 1)

These defines pretty much all need namespacing, especially things like
RESET are way too collision prone.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20211004/0da43b92/attachment-0001.sig>