Received-SPF: pass (google.com: domain of linux-kernel+bounces-209659-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 11 Jun 2024 11:36:31 +0100
From: Mark Brown <broonie@kernel.org>
To: Piotr Wojtaszczyk <piotr.wojtaszczyk@timesys.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>, Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Vladimir Zapolskiy <vz@mleia.com>,
	Russell King <linux@armlinux.org.uk>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Chancel Liu <chancel.liu@nxp.com>, Arnd Bergmann <arnd@arndb.de>,
	Michael Ellerman <mpe@ellerman.id.au>, linux-sound@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, alsa-devel@alsa-project.org,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [Patch v2 1/2] ASoC: fsl: Add i2s and pcm drivers for LPC32xx
 CPUs
Message-ID: <Zmgor8accyAiUkUO@finisterre.sirena.org.uk>
References: <[PATCH]ASoC:fsl:Addi2sandpcmdriversforLPC32xxCPUs>
 <20240611094810.27475-1-piotr.wojtaszczyk@timesys.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="WW949k97zQLQvi69"
Content-Disposition: inline
In-Reply-To: <20240611094810.27475-1-piotr.wojtaszczyk@timesys.com>


--WW949k97zQLQvi69
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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@timesys.com>
> +L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
> +L:	linuxppc-dev@lists.ozlabs.org
> +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) +=3D snd-soc-fsl-xcvr.o
>  obj-$(CONFIG_SND_SOC_FSL_AUD2HTX) +=3D snd-soc-fsl-aud2htx.o
>  obj-$(CONFIG_SND_SOC_FSL_RPMSG) +=3D snd-soc-fsl-rpmsg.o
>  obj-$(CONFIG_SND_SOC_POWERPC_QMC_AUDIO) +=3D snd-soc-fsl-qmc-audio.o
> +obj-$(CONFIG_SND_SOC_FSL_LPC3XXX) +=3D snd-soc-fsl-lpc3xxx.o
> =20
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@nxp.com>
> + *

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 =3D snd_soc_dai_get_drvdata(cpu_dai=
);
> +	struct device *dev =3D i2s_info_p->dev;
> +
> +	if ((fmt & SND_SOC_DAIFMT_FORMAT_MASK) !=3D 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 =3D 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 =3D clk_get_rate(i2s_info_p->clk);
> +	if (i2s_info_p->clkrate =3D=3D 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.

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

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

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmZoKKsACgkQJNaLcl1U
h9DNgQf/bVmcCufP8jvhRnYGwhJ74PYfe8uGNqVDcvOPBqyxwbAHEcBkY4os9h6R
GoQMz21Qh+ZtEks0KjHFDl3Lnd51Kac5YBeyl5s/WRciIBOxe+jSqmW0GpRYZwZ+
YEMXUIyS6zA8Tik8onJLOtFrl4wLHXafkWOOPulK5v8+mZXYqPypwKq87c43IkrI
MviOLdW4TB0CVCKvk590+1YPY+ZBBAELM6eO/D9du/AyvN+BkUnZLO/VOwQ4enRQ
r2xnb2uSXMbDtKZKr4PbNw+9NbNaw9axfSzHbwXCu1Z0y+eOEKS1Vu2613VY9Al0
VKMHNg0tTYcWMk0XTzihz60mgPSf0A==
=srbB
-----END PGP SIGNATURE-----

--WW949k97zQLQvi69--