Re: [PATCH] ASoC: fsl: Add i2s and pcm drivers for LPC32xx CPUs

[Krzysztof Kozlowski]

On 10/06/2024 12:24, Piotr Wojtaszczyk wrote:
> This driver was ported from an old version in linux 2.6.27 and adjusted
> for the new ASoC framework and DMA API.
> 
> Signed-off-by: Piotr Wojtaszczyk <piotr.wojtaszczyk@xxxxxxxxxxx>
> ---
>  .../bindings/sound/nxp,lpc3220-i2s.yaml       |  50 +++
>  arch/arm/boot/dts/lpc32xx.dtsi                |   4 +
>  arch/arm/mach-lpc32xx/phy3250.c               |  60 +++
>  sound/soc/fsl/Kconfig                         |   7 +
>  sound/soc/fsl/Makefile                        |   2 +
>  sound/soc/fsl/lpc3xxx-i2s.c                   | 411 ++++++++++++++++++
>  sound/soc/fsl/lpc3xxx-i2s.h                   |  94 ++++
>  sound/soc/fsl/lpc3xxx-pcm.c                   |  75 ++++
>  8 files changed, 703 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
>  create mode 100644 sound/soc/fsl/lpc3xxx-i2s.c
>  create mode 100644 sound/soc/fsl/lpc3xxx-i2s.h
>  create mode 100644 sound/soc/fsl/lpc3xxx-pcm.c
> 
> diff --git a/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
> new file mode 100644

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.


> index 000000000000..e41330b6775c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/nxp,lpc3220-i2s.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/nxp,lpc3220-i2s.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP LPC32XX I2S Controller
> +
> +description:
> +  The block adds I2S and PCM drivers for LPC32XX

Please describe the hardware. What is "this block"? What is a driver?
Like a Linux driver? Then not, describe the hardware.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - nxp,lpc3220-i2s
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: input clock of the peripheral.
> +
> +  clock-names:
> +    items:
> +      - const: i2s_clk

Drop _clk. Or actually drop entire clock-names, obvious and not needed.

> +
> +  interrupts:
> +    maxItems: 1

That's not a DAI?

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/lpc32xx-clock.h>
> +
> +    i2s0: i2s@20094000 {
> +      compatible = "nxp,lpc3220-i2s";
> +      reg = <0x20094000 0x1000>;
> +      clocks = <&clk LPC32XX_CLK_I2S0>;
> +      clock-names = "i2s_clk";

Make the example complete - missing interrupts.


> +    };
> +
> +...
> diff --git a/arch/arm/boot/dts/lpc32xx.dtsi b/arch/arm/boot/dts/lpc32xx.dtsi
> index c87066d6c995..dc5738f2b42d 100644
> --- a/arch/arm/boot/dts/lpc32xx.dtsi
> +++ b/arch/arm/boot/dts/lpc32xx.dtsi


? DTS is not ASoC. This MUST go via entirely different tree.


> @@ -221,6 +221,8 @@ spi2: spi@20090000 {


> +
> +const struct snd_soc_dai_ops lpc3xxx_i2s_dai_ops = {
> +	.startup = lpc3xxx_i2s_startup,
> +	.shutdown = lpc3xxx_i2s_shutdown,
> +	.prepare = lpc3xxx_i2s_prepare,
> +	.trigger = lpc3xxx_i2s_trigger,
> +	.hw_params = lpc3xxx_i2s_hw_params,
> +	.set_sysclk = lpc3xxx_i2s_set_dai_sysclk,
> +	.set_fmt = lpc3xxx_i2s_set_dai_fmt,
> +};
> +
> +static int lpc3xxx_i2s_dai_probe(struct snd_soc_dai *dai)
> +{
> +	struct lpc3xxx_i2s_info *i2s_info_p = snd_soc_dai_get_drvdata(dai);
> +
> +	snd_soc_dai_init_dma_data(dai, &i2s_info_p->playback_dma_config,
> +							&i2s_info_p->capture_dma_config);
> +	return 0;
> +}
> +
> +struct snd_soc_dai_driver lpc3xxx_i2s_dai_driver = {
> +	 .probe	= lpc3xxx_i2s_dai_probe,
> +	 .playback = {
> +		      .channels_min = 1,
> +		      .channels_max = 2,
> +		      .rates = LPC3XXX_I2S_RATES,
> +		      .formats = LPC3XXX_I2S_FORMATS,
> +		      },
> +	 .capture = {
> +		     .channels_min = 1,
> +		     .channels_max = 2,
> +		     .rates = LPC3XXX_I2S_RATES,
> +		     .formats = LPC3XXX_I2S_FORMATS,
> +		     },
> +	 .ops = &lpc3xxx_i2s_dai_ops,
> +	 .symmetric_rate = 1,
> +	 .symmetric_channels = 1,
> +	 .symmetric_sample_bits = 1,
> +};
> +
> +static const struct snd_soc_component_driver lpc32xx_i2s_component = {
> +	.name = "lpc32xx-i2s",
> +};
> +
> +static const struct regmap_config lpc32xx_i2s_regconfig = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = I2S_RX_RATE,
> +};
> +
> +static int lpc32xx_i2s_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lpc3xxx_i2s_info *i2s_info_p;
> +	struct resource *res;
> +	void __iomem *iomem;
> +	int ret;
> +
> +	i2s_info_p = devm_kzalloc(dev, sizeof(*i2s_info_p), GFP_KERNEL);
> +	if (!i2s_info_p)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, i2s_info_p);
> +	i2s_info_p->dev = dev;
> +
> +	iomem = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(iomem)) {
> +		dev_err(dev, "Can't map registers\n");

return dev_err_probe()

> +		return PTR_ERR(iomem);
> +	}
> +
> +	i2s_info_p->regs = devm_regmap_init_mmio(dev, iomem, &lpc32xx_i2s_regconfig);
> +	if (IS_ERR(i2s_info_p->regs)) {
> +		ret = PTR_ERR(i2s_info_p->regs);
> +		dev_err(dev, "failed to init register map: %d\n", ret);

return dev_err_probe()

> +		return ret;
> +	}
> +
> +	i2s_info_p->clk = devm_clk_get(dev, "i2s_clk");
> +	if (IS_ERR(i2s_info_p->clk)) {
> +		dev_err(dev, "Can't get clock\n");

return dev_err_probe()


> +		return PTR_ERR(i2s_info_p->clk);
> +	}
> +
> +	i2s_info_p->clkrate = clk_get_rate(i2s_info_p->clk);
> +	if (i2s_info_p->clkrate == 0) {
> +		dev_err(dev, "Invalid returned clock rate\n");
> +		goto err_clk_disable;
> +	}
> +
> +	mutex_init(&i2s_info_p->lock);
> +
> +	ret = devm_snd_soc_register_component(dev, &lpc32xx_i2s_component,
> +					 &lpc3xxx_i2s_dai_driver, 1);
> +	if (ret) {
> +		dev_err(dev, "Can't register cpu_dai component\n");
> +		goto err_clk_disable;

Where is the clock disable? It's just return dev_err_probe.

> +	}
> +
> +	i2s_info_p->playback_dma_config.addr = (dma_addr_t)(res->start + I2S_TX_FIFO);
> +	i2s_info_p->playback_dma_config.maxburst = 4;
> +	i2s_info_p->playback_dma_config.filter_data = "i2s-tx";
> +	i2s_info_p->capture_dma_config.addr = (dma_addr_t)(res->start + I2S_RX_FIFO);
> +	i2s_info_p->capture_dma_config.maxburst = 4;
> +	i2s_info_p->capture_dma_config.filter_data = "i2s-rx";
> +
> +	ret = lpc3xxx_pcm_register(pdev);
> +	if (ret) {
> +		dev_err(dev, "Can't register pcm component\n");
> +		goto err_clk_disable;
> +	}
> +
> +	return 0;
> +
> +err_clk_disable:
> +	return ret;


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

Why do you need empty function?

> +
> +static const struct of_device_id lpc32xx_i2s_match[] = {
> +	{ .compatible = "nxp,lpc3220-i2s" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, lpc32xx_i2s_match);
> +
> +static struct platform_driver lpc32xx_i2s_driver = {
> +	.probe = lpc32xx_i2s_probe,
> +	.remove = lpc32xx_i2s_remove,
> +	.driver		= {
> +		.name	= "lpc3xxx-i2s",
> +		.of_match_table = of_match_ptr(lpc32xx_i2s_match),

Drop of_match_ptr, you will have here warnings.

> +	},
> +};
> +
> +module_platform_driver(lpc32xx_i2s_driver);
> +
> +MODULE_AUTHOR("Kevin Wells <kevin.wells@xxxxxxx>");
> +MODULE_AUTHOR("Piotr Wojtaszczyk <piotr.wojtaszczyk@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("ASoC LPC3XXX I2S interface");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/fsl/lpc3xxx-i2s.h b/sound/soc/fsl/lpc3xxx-i2s.h
> new file mode 100644
> index 000000000000..f88ab74cfe41


Best regards,
Krzysztof