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@xxxxxxxxxxx>
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.
Attachment:
signature.asc
Description: PGP signature
