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
