Re: [PATCH] ASoC: codecs: add support for the TI SRC4392 codec

On Mon, Aug 08, 2022 at 05:19:52PM +1000, Matt Flax wrote:

The src4xxx keyword is used for future capability to integrate other codecs similar to the src4392 to the same code base.

Formating here is really weird.

• tristate "Texas Instruments SRC4XXX DIR/DIT and SRC codecs"
• help

• Enable support for the TI SRC4XXX family of codecs. These include the
  

• scr4392 which has digital receivers, transmitters, and
  

• a sample rate converter, including numerous ports.
  

Please keep this to 80 columns.

@@ -0,0 +1,55 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Driver for SRC4XXX codecs
• • Copyright 2021-2022 Deqx Pty Ltd
• • Author: Natt Flax flatmax@flatmax.com

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

+static const struct of_device_id src4xxx_of_match[] = {

• { .compatible = "ti,src4392", },
• { }

+}; +MODULE_DEVICE_TABLE(of, src4xxx_of_match);

This is adding a DT binding, that binding should be documented.

+static const struct snd_kcontrol_new dit_mux_control =

• SOC_DAPM_ENUM("Dig. Out src", dit_mux_enum);

Please write words out fully as is idiomatic for ALSA.

+static const char * const src_mclk_text[] = {

• "Master (MCLK)", "Master (RXCLKI)", "Recovered receiver clk",

+}; +static SOC_ENUM_SINGLE_DECL(src_mclk_enum, SRC4XXX_SCR_CTL_2D, 2,

• src_mclk_text);

+static const struct snd_kcontrol_new src_mclk_control =

• SOC_DAPM_ENUM("SRC master clock select", src_mclk_enum);

I would normally expect this to be controlled by the machine driver - why expose it to userspace?

+static int src4xxx_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt) +{

• struct snd_soc_component *component = dai->component;
• struct src4xxx *src4xxx = snd_soc_component_get_drvdata(component);
• unsigned int ctrl;
• dev_info(dai->dev, "__func__ enter 0x%x id=%d\n",

• fmt, dai->id);
  

This is way too noisy, in general this sort of print is redundant and it certainly shouldn't be something like dev_info() which is often printed to the console by default. In general the driver should be silent in normal operation, especially on the console.

• switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
• case SND_SOC_DAIFMT_I2S:
• ctrl |= SRC4XXX_BUS_I2S;

• break;
  

• case SND_SOC_DAIFMT_LEFT_J:

• ctrl |= SRC4XXX_BUS_LEFT_J;
  

The indentation here is weird and inconsistent...

• int reg = SRC4XXX_PORTA_CTL_04;
• int ret;
• if (dai->id == SRC4XXX_PORTB)

• reg = SRC4XXX_PORTB_CTL_06;
  

Write a switch statement to select the register, it would be a lot clearer.

• if (ret) {
  

• 	dev_err(component->dev,
  

• "Couldn't set the TX's div register to %d << %d = 0x%x\n",
  

Again really strange indentation.

• /* enable the BTI and TSLIP interrupts */
• ret = regmap_update_bits(src4xxx->regmap, SRC4XXX_SRC_DIT_IRQ_MSK_0B,

• SRC4XXX_SRC_BTI_EN | SRC4XXX_SRC_TSLIP_EN,
  

• SRC4XXX_SRC_BTI_EN | SRC4XXX_SRC_TSLIP_EN);
  

• if (ret < 0)

• dev_err(dev,
  

• 	"Failed to enable BTI and TSLIP interrupts : %d\n",
  

• 	ret);
  

The driver never requests an interrupt?

• if (ret == 0)

• dev_info(dev, "src4392 probe ok %d\n", ret);
  

• return ret;

+} +EXPORT_SYMBOL(src4xxx_probe);

This is using _GPL() APIs from ASoC and regmap, it should be _GPL too.

+void src4xxx_remove(struct device *dev) +{

• dev_info(dev, "__func__\n");

+} +EXPORT_SYMBOL(src4xxx_remove);

This is just noise, remove the function entirely.