Re: [PATCH v3] ASoC: adds component driver for TAS575xM digital amplifiers

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-01-10 9:45 AM, Joerg Schambacher wrote:
> Adds a minimum component driver to run the amplifier in I2S master
> mode only from standard audio clocks. Therefore, it only allows
> 44.1, 88.2, 176.4, 48, 96 and 192ksps with 16, 20, 24 and 32 bits
> sample size. Digital volume control and the -6dB and +0.8dB switches
> are supported.

Couple nitpicks and suggestions below.

(...)

> +static int tas5754m_set_bias_level(struct snd_soc_component *component,
> +					enum snd_soc_bias_level level)
> +{
> +	struct tas5754m_priv *tas5754m =
> +				snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_ON:
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +
> +	case SND_SOC_BIAS_STANDBY:
> +		ret = regmap_update_bits(tas5754m->regmap,
> +				TAS5754M_POWER, TAS5754M_RQST, 0);
> +		if (ret != 0) {

I believe we are dealing here with standard API function i.e. 0 on 
success and negative value on error. And thus, 'if (ret)' suffices.

> +			dev_err(component->dev,
> +				"Failed to remove standby: %d\n", ret);
> +			return ret;
> +		}
> +		break;
> +
> +	case SND_SOC_BIAS_OFF:
> +		ret = regmap_update_bits(tas5754m->regmap,
> +				TAS5754M_POWER, TAS5754M_RQST, TAS5754M_RQST);
> +		if (ret != 0) {

Ditto. This also goes for every single usage of regmap_xxx() in this file.

> +			dev_err(component->dev,
> +				"Failed to request standby: %d\n", ret);
> +			return ret;
> +		}
> +		break;
> +	}
> +
> +	return 0;

You could also drop the 'return ret' from the if-statements above - 
granting you also ability to drop the brackets - and instead return 
'ret' instead of '0' here. Of course that means 'ret' needs to be 
initialized appropriately at the top of the function.

> +}
> +
> +int tas5754m_set_clock_tree_master(struct snd_soc_dai *dai,
> +					struct snd_pcm_hw_params *params)

Indentation seems off.

> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tas5754m_priv *tas5754m =
> +			snd_soc_component_get_drvdata(component);
> +	static const struct reg_sequence pll_settings[] = {
> +		{ TAS5754M_PLL_COEFF_P,		0x01 },	// P=2
> +		{ TAS5754M_PLL_COEFF_J,		0x08 },	// J=8
> +		{ TAS5754M_PLL_COEFF_DL,	0x00 },	// D12-8 = 0
> +		{ TAS5754M_PLL_COEFF_DH,	0x00 },	// D7-0 = 0
> +		{ TAS5754M_PLL_COEFF_R,		0x00 },	// R=1
> +	};
> +	int ret;
> +
> +	/* disable PLL before any clock tree change */
> +	ret = regmap_update_bits(tas5754m->regmap, TAS5754M_PLL_EN,
> +				 TAS5754M_PLLE, 0);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to disable PLL: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* set DAC clock source to MCLK */
> +	ret = regmap_write(tas5754m->regmap, TAS5754M_DAC_REF, 0x30);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set DAC ref\n");
> +		return ret;
> +	}
> +
> +	/* run PLL at fixed ratio to MCLK */
> +	ret = regmap_multi_reg_write(tas5754m->regmap, pll_settings,
> +					ARRAY_SIZE(pll_settings));
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set PLL ratio\n");
> +		return ret;
> +	}
> +
> +	/* set DSP divider to 2 => reg 0x01 */
> +	ret = regmap_write(tas5754m->regmap, TAS5754M_DSP_CLKDIV, 1);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set DSP divider\n");
> +		return ret;
> +	}
> +	/* set DAC divider to 4 => reg 0x03*/
> +	ret = regmap_write(tas5754m->regmap, TAS5754M_DAC_CLKDIV, 3);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set OSDACR divider\n");
> +		return ret;
> +	}
> +	/* set OSR divider to 1 */
> +	ret = regmap_write(tas5754m->regmap, TAS5754M_OSR_CLKDIV, 0);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set OSR divider\n");
> +		return ret;
> +	}
> +	/* set CP divider to 4 => reg 0x03*/
> +	ret = regmap_write(tas5754m->regmap, TAS5754M_NCP_CLKDIV, 3);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to set CP divider\n");
> +		return ret;
> +	}
> +	/* finally enable PLL */
> +	ret = regmap_update_bits(tas5754m->regmap, TAS5754M_PLL_EN,
> +				 TAS5754M_PLLE, 1);
> +	if (ret != 0) {
> +		dev_err(component->dev, "Failed to enable PLL: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I'd suggest to keep the logical block organization cohesive. Especially 
if there are several of them all located within a single function. Some 
of the do/check/error-out blocks above are separated by a newline from 
the following ones, and some are not.

Another point is the cohesiveness of the error-message format. Some of 
the above print value of 'ret' i.e. carry additional value whereas other 
skip that part. Is this intentional?

> +
> +int tas5754m_set_dai_mode(struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tas5754m_priv *tas5754m =
> +			snd_soc_component_get_drvdata(component);
> +	int fmt = tas5754m->fmt;
> +
> +	/* only I2S MASTER mode implemented */
> +	if (((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S)) {

Maybe I'm missing something but the most outter pair of brackets is 
redundant.

> +		dev_err(component->dev,
> +			"DAI format not supported (I2S master only)\n");
> +		return -EINVAL;
> +	}
> +	/* TAS5754/6m do not support inverted clocks in MASTER mode */

A newline before the comment would make this more readabile - that's a 
new logical block afterall.

> +	if (((fmt & SND_SOC_DAIFMT_CLOCK_MASK) != SND_SOC_DAIFMT_NB_NF)) {

Again, I may be missing something, but this looks like outter brackets 
are redundant.

> +		dev_err(component->dev,	"Inverted clocks not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		regmap_update_bits(tas5754m->regmap,
> +				TAS5754M_BCLK_LRCLK_CFG,
> +				TAS5754M_LRKO | TAS5754M_BCKO,
> +				TAS5754M_LRKO | TAS5754M_BCKO);
> +		/* reset CLK dividers */
> +		regmap_update_bits(tas5754m->regmap,
> +				TAS5754M_MASTER_MODE,
> +				0x00,
> +				TAS5754M_RLRK | TAS5754M_RBCK);
> +		/* ignore all clock error detection but MCLK */
> +		regmap_update_bits(tas5754m->regmap,
> +				TAS5754M_ERROR_DETECT,
> +				TAS5754M_IPLK | TAS5754M_DCAS |
> +				TAS5754M_IDCM | TAS5754M_IDSK |
> +				TAS5754M_IDBK | TAS5754M_IDFS,
> +				TAS5754M_IPLK | TAS5754M_DCAS |
> +				TAS5754M_IDCM | TAS5754M_IDSK |
> +				TAS5754M_IDBK | TAS5754M_IDFS);
> +		break;
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +	case SND_SOC_DAIFMT_CBM_CFS:
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int tas5754m_set_dividers_master(struct snd_soc_dai *dai,
> +				struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tas5754m_priv *tas5754m =
> +			snd_soc_component_get_drvdata(component);
> +	unsigned long bclk;
> +	unsigned long mclk;
> +	int bclk_div;
> +	int lrclk_div;
> +	int osr;
> +	int ret;
> +
> +	mclk = clk_get_rate(tas5754m->sclk);
> +	bclk = tas5754m->sample_len * 2 * params_rate(params);
> +	bclk_div = mclk / bclk;
> +	lrclk_div = tas5754m->sample_len * 2;
> +	osr = mclk / 4 / params_rate(params) / 16;

Is there a specific reason as to why these magic numbers aren't 
defines/constants?

> +
> +	// stop LR / SCLK clocks

Formatting of this comment looks odd. Please align with the recommended one.


(...)


Regards,
Czarek