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

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@timesys.com

.../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@nxp.com"); +MODULE_AUTHOR("Piotr Wojtaszczyk piotr.wojtaszczyk@timesys.com"); +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