Re: [PATCH v1 2/4] ASoC: Add codec driver for ST TDA7802

[Mark Brown]

On Tue, Jun 11, 2019 at 06:49:07PM +0100, Thomas Preston wrote:

> +++ b/sound/soc/codecs/tda7802.c
> @@ -0,0 +1,580 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * tda7802.c  --  codec driver for ST TDA7802
> + *

Make the entire comment a C++ one so this looks more intentional.

> + * Copyright (C) 2016-2019 Tesla Motors, Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.

You've got a SPDX header, you don't also need GPL boilerplate (this
doesn't match your SPDX header either...).

> +enum tda7802_type {
> +	tda7802_base,
> +};
> +
> +#define I2C_BUS				   4
> +
> +#define CHANNELS_PER_AMP		   4
> +#define MAX_NUM_AMPS			   4
> +

These are concerning but also unused.

> +
> +static const u8 IB3_FORMAT[4][4] = {
> +	/* 1x amp, 4 channels */
> +	{ IB3_FORMAT_TDM4 },
> +	/* 2x amp, 8 channels */
> +	{ IB3_FORMAT_TDM8_DEV1,
> +		IB3_FORMAT_TDM8_DEV2 },
> +	/* 3x amp not supported */
> +	{ },
> +	/* 4x amp, 16 channels */
> +	{ IB3_FORMAT_TDM16_DEV1,
> +		IB3_FORMAT_TDM16_DEV2,
> +		IB3_FORMAT_TDM16_DEV3,
> +		IB3_FORMAT_TDM16_DEV4 },
> +};

There are indentation issues here and this is also rather magic
numberish.  I *think* you should be implement set_tdm_slot().

> +#ifdef DEBUG
> +static int tda7802_dump_regs(struct tda7802_priv *tda7802)
> +{

There's already facilities for viewing the register map in regmap, no
need to open code anything.

> +	/* All channels out of tri-state mode, all channels in Standard Class
> +	 * AB mode (not High-efficiency)
> +	 */
> +	data[0] = IB0_CH4_CLASS_AB | IB0_CH3_CLASS_AB | IB0_CH2_CLASS_AB |
> +		IB0_CH1_CLASS_AB;

The AB mode selection should be a user visible control not hard coded.

> +	/* Rear channel load impedance set to 2-Ohm, default diagnostic timing,
> +	 * GV1 gain on all channels (default), no digital gain increase
> +	 */
> +	data[1] = IB1_REAR_IMPEDANCE_2_OHM | IB1_LONG_DIAG_TIMING_x1;
> +	switch (tda7802->gain_ch13) {

The impedance should be a DT property, the gains should be user
controllable.

> +	case 2:
> +		data[1] |= IB1_GAIN_CH13_GV2;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown gain for channel 1,3 %d, setting to 1\n",
> +				tda7802->gain_ch13);
> +	case 1:
> +		data[1] |= IB1_GAIN_CH13_GV1;
> +		break;

It would be more normal to have the default case as the last thing in the
switch statement and the fall through is obviously a bug, if the driver
doesn't know how to handle the configuration it should return an error.

> +	}
> +	switch (tda7802->gain_ch24) {

Blank line please.

> +	/* Mute timing 1.45ms, all channels un-muted, digital mute enabled,
> +	 * 5.3V undervoltage threshold, front-channel load impedance set to
> +	 * 2-Ohms
> +	 */
> +	data[2] = IB2_MUTE_TIME_1_MS | IB2_CH13_UNMUTED | IB2_CH24_UNMUTED |
> +		IB2_AUTOMUTE_THRESHOLD_5V3 | IB2_FRONT_IMPEDANCE_2_OHM;

More stuff that shouldn't be hard coded in the driver.

> +	if (mute) {
> +		/* digital mute */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB2,
> +				IB2_DIGITAL_MUTE_DISABLED,
> +				~IB2_DIGITAL_MUTE_DISABLED);
> +		if (err < 0)
> +			dev_err(dev, "Cannot mute amp %d\n", err);
> +
> +		/* amp off */
> +		err = snd_soc_component_update_bits(dai->component, TDA7802_IB5,
> +				IB5_AMPLIFIER_ON, ~IB5_AMPLIFIER_ON);
> +		if (err < 0)
> +			dev_err(dev, "Cannot amp off %d\n", err);

Turning off the amplifier is clearly power management and should be
handled via DAPM.

> +static int tda7802_set_tdm_slot(struct snd_soc_dai *dai, unsigned int tx_mask,
> +		unsigned int rx_mask, int slots, int slot_width)
> +{

> +	/* 48kHz sample rate, TDM configuration, 64-bit I2S frame period, PLL
> +	 * clock dither disabled, high-pass filter enabled (blocks DC output)
> +	 */
> +	data = IB3_SAMPLE_RATE_48_KHZ | IB3_FORMAT[width_index][slot_index] |

The sample rate should be set using hw_params(), the high pass filter
should be controllable via a control.

I've stopped reading here, it's fairly clear from what I've seen up to
here that the code is not well integrated with the framework with lots
of hard coding and things scattered around in places where I'd not
expect to find them.  A lot of things should be moved out of hard coding
into ALSA controls, stream configuration should be done via hw_params()
and power management via DAPM - the hardware looks fairly simple so I'd
expect this to all be fairly straightforward to fix.

Attachment: signature.asc
Description: PGP signature