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

[Mark Brown]

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 at 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.
-------------- 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/20220808/e6df6f2b/attachment.sig>