Date: Mon, 8 Aug 2022 12:32:27 +0100
From: Mark Brown <broonie@kernel.org>
To: Matt Flax <flatmax@flatmax.com>
Subject: Re: [PATCH] ASoC: codecs: add support for the TI SRC4392 codec
Message-ID: <YvD0Sy7SJsiPaZPd@sirena.org.uk>
References: <20220808071952.2432253-1-flatmax@flatmax.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
 protocol="application/pgp-signature"; boundary="VOmgWxGw8XkhSlTc"
Content-Disposition: inline
In-Reply-To: <20220808071952.2432253-1-flatmax@flatmax.com>
Cc: alsa-devel@alsa-project.org
Precedence: list
Errors-To: alsa-devel-bounces@alsa-project.org
Sender: "Alsa-devel" <alsa-devel-bounces@alsa-project.org>


--VOmgWxGw8XkhSlTc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--VOmgWxGw8XkhSlTc
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmLw9EsACgkQJNaLcl1U
h9D+4gf/Ztbp4VhuAU4zlZqKoPmJ1Aoj3eBOZ/7vh//H+ZYtNEYcYrf4jxx5kUzM
fg5BagWVgLTQrG5icS85ogCVvII7D8JtUhKwyeBdSQe60ZjKyGeIrryrXs67jXrX
4ouoDlpFD1vey2AcNTggGCKoL0dX/ZTBRouz8a4tuwv4zDupW95WVQ2gXunukVSf
zgnGPEQPA4R/YXKjPNQfRriHMXwidZmjf8X1YEqqGXIpLmNZ5HVVXg0ZWbpJhr61
JYme3ExCVFPn798lH3S2l9TCVSlHgfxWs1xR6AwC43K4u3fDXc64jdy2KJck3wt9
bKiJRimxhsGWSl/c5KX2CTw+zDtU/Q==
=bGCd
-----END PGP SIGNATURE-----

--VOmgWxGw8XkhSlTc--