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
