Re: [Patch v2 1/2] ASoC: fsl: Add i2s and pcm drivers for LPC32xx CPUs

[Mark Brown]

On Tue, Jun 11, 2024 at 11:47:51AM +0200, Piotr Wojtaszczyk wrote:

>  arch/arm/boot/dts/nxp/lpc/lpc32xx.dtsi |   4 +
>  arch/arm/mach-lpc32xx/phy3250.c        |  60 ++++

These architecture changes are separate and should at least be separate
commits, copied to the architecture maintainers.

> +FREESCALE SOC LPC32XX SOUND DRIVERS
> +M:	Piotr Wojtaszczyk <piotr.wojtaszczyk@xxxxxxxxxxx>
> +L:	alsa-devel@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> +L:	linuxppc-dev@xxxxxxxxxxxxxxxx
> +S:	Orphan
> +F:	sound/soc/fsl/lpc3xxx-*
> +

It seems a bit odd to add yourself as a maintainer while also marking
the driver as orphan?

> +config SND_SOC_FSL_LPC3XXX
> +	tristate "SoC Audio for NXP LPC32XX CPUs"
> +	depends on ARCH_LPC32XX && SND_SOC

On a quick scan I can't see any architecture dependency for build,
please add an || COMPILE_TEST for improved coverage.  As for all the
other things enabled in this Kconfig file there is no need to explicitly
depend on SND_SOC.

> @@ -42,6 +43,7 @@ obj-$(CONFIG_SND_SOC_FSL_XCVR) += snd-soc-fsl-xcvr.o
>  obj-$(CONFIG_SND_SOC_FSL_AUD2HTX) += snd-soc-fsl-aud2htx.o
>  obj-$(CONFIG_SND_SOC_FSL_RPMSG) += snd-soc-fsl-rpmsg.o
>  obj-$(CONFIG_SND_SOC_POWERPC_QMC_AUDIO) += snd-soc-fsl-qmc-audio.o
> +obj-$(CONFIG_SND_SOC_FSL_LPC3XXX) += snd-soc-fsl-lpc3xxx.o
>  
Please try to keep these files sorted alphabetically (it's not 100% at
the minute but no need to make it worse).

> --- /dev/null
> +++ b/sound/soc/fsl/lpc3xxx-i2s.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Author: Kevin Wells <kevin.wells@xxxxxxx>
> + *

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

> +static u32 absd32(u32 v1, u32 v2)
> +{
> +	if (v1 > v2)
> +		return v1 - v2;
> +	return v2 - v1;
> +}

Just use abs()?

> +static int lpc3xxx_i2s_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> +{
> +	struct lpc3xxx_i2s_info *i2s_info_p = snd_soc_dai_get_drvdata(cpu_dai);
> +	struct device *dev = i2s_info_p->dev;
> +
> +	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) != SND_SOC_DAIFMT_I2S) {
> +		dev_warn(dev, "unsupported bus format %d\n", fmt);
> +		return -EINVAL;
> +	}
> +	return 0;
> +}

If we're validating for I2S we should probably validate for clock
provider too.  Or just remove the function, it's not really needed?

> +	i2s_info_p->clk = devm_clk_get(dev, "i2s_clk");
> +	if (IS_ERR(i2s_info_p->clk))
> +		return dev_err_probe(dev, PTR_ERR(i2s_info_p->clk), "Can't get clock\n");
> +
> +	i2s_info_p->clkrate = clk_get_rate(i2s_info_p->clk);
> +	if (i2s_info_p->clkrate == 0)
> +		return dev_err_probe(dev, -EINVAL, "Invalid returned clock rate\n");

Nothing ever enables this clock.

> +static int lpc32xx_i2s_remove(struct platform_device *pdev)
> +{
> +	return 0;
> +}

Remove empty functions, if they can legitimately be empty the framework
will support them being absent.

> +#define _SBF(f, v) ((v) << (f))

FIELD_PREP()

> +#define _BIT(n) _SBF(n, 1)

BIT().

> +/* I2S controller register offsets */
> +#define I2S_DAO		0x00
> +#define I2S_DAI		0x04
> +#define I2S_TX_FIFO	0x08
> +#define I2S_RX_FIFO	0x0C
> +#define I2S_STAT	0x10
> +#define I2S_DMA0	0x14
> +#define I2S_DMA1	0x18
> +#define I2S_IRQ		0x1C
> +#define I2S_TX_RATE	0x20
> +#define I2S_RX_RATE	0x24

Add a prefix to all these I2S_ names in case of collisions.

Attachment: signature.asc
Description: PGP signature