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) += 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@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 = 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.