Re: [PATCH v2 2/7] ASoC: sophgo: add CV1800B I2S/TDM controller driver

["Anton D. Stavinskii" <stavinsky@gmail.com>]

> > +++ b/sound/soc/sophgo/Kconfig
> > @@ -0,0 +1,20 @@
> > +menu "Sophgo"
> 
> Missing SPDX. Did you run checkpatch?

Actually I did. I will add SPDX and recheck again. 
Also I will take a look why checkpatch missed this. 

> 
> > +	depends on COMPILE_TEST || ARCH_SOPHGO
> > +
> > +config SND_SOC_CV1800B_TDM
> > +	tristate "Sophgo CV1800B I2S/TDM support"
> > +	depends on SND_SOC && OF
> > +	select SND_SOC_GENERIC_DMAENGINE_PCM
> > +	help
> > +	  This option enables the I2S/TDM audio controller found in Sophgo
> > +	  CV1800B / SG2002 SoCs. The controller supports standard I2S
> > +	  audio modes for playback and capture.
> > +
> > +	  The driver integrates with the ASoC framework and uses the DMA
> > +	  engine for audio data transfer. It is intended to be configured
> > +	  via Device Tree along with simple-audio-card module.
> > +
> > +	  To compile the driver as a module, choose M here: the module will
> > +	  be called cv1800b_tdm.
> > +
> 
> ...
> 
> > +static const struct snd_soc_dai_ops cv1800b_i2s_dai_ops = {
> > +	.probe = cv1800b_i2s_dai_probe,
> > +	.startup = cv1800b_i2s_startup,
> > +	.hw_params = cv1800b_i2s_hw_params,
> > +	.trigger = cv1800b_i2s_trigger,
> > +	.set_fmt = cv1800b_i2s_dai_set_fmt,
> > +	.set_bclk_ratio = cv1800b_i2s_dai_set_bclk_ratio,
> > +	.set_sysclk = cv1800b_i2s_dai_set_sysclk,
> > +};
> > +
> > +static struct snd_soc_dai_driver cv1800b_i2s_dai_template = {
> 
> Missing const
> 
> > +	.name = "cv1800b-i2s",
> > +	.playback = {
> > +		.stream_name = "Playback",
> > +		.channels_min = 1,
> > +		.channels_max = 2,
> > +		.rates = SNDRV_PCM_RATE_8000_192000,
> > +		.formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S16_LE,
> > +	},
> > +	.capture = {
> > +		.stream_name = "Capture",
> > +		.channels_min = 1,
> > +		.channels_max = 2,
> > +		.rates = SNDRV_PCM_RATE_8000_192000,
> > +		.formats = SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S16_LE,
> > +	},
> > +	.ops = &cv1800b_i2s_dai_ops,
> > +};
> 
> ....
> 
> > +static int cv1800b_i2s_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct cv1800b_i2s *i2s;
> > +	struct resource *res;
> > +	void __iomem *regs;
> > +	struct snd_soc_dai_driver *dai;
> > +	int ret;
> > +
> > +	i2s = devm_kzalloc(dev, sizeof(*i2s), GFP_KERNEL);
> > +	if (!i2s)
> > +		return -ENOMEM;
> > +
> > +	regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +	i2s->dev = &pdev->dev;
> > +	i2s->base = regs;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +	cv1800b_setup_dma_struct(i2s, res->start);
> > +
> > +	i2s->clk = devm_clk_get_enabled(dev, "i2s");
> > +	if (IS_ERR(i2s->clk))
> > +		return dev_err_probe(dev, PTR_ERR(i2s->clk),
> > +				     "failed to get+enable i2s\n");
> > +	i2s->sysclk = devm_clk_get_enabled(dev, "mclk");
> > +	if (IS_ERR(i2s->sysclk))
> > +		return dev_err_probe(dev, PTR_ERR(i2s->sysclk),
> > +				     "failed to get+enable mclk\n");
> > +
> > +	platform_set_drvdata(pdev, i2s);
> > +	cv1800b_i2s_setup_tdm(i2s);
> > +
> > +	dai = devm_kmemdup(dev, &cv1800b_i2s_dai_template, sizeof(*dai),
> > +			   GFP_KERNEL);
> > +	if (!dai)
> > +		return -ENOMEM;
> > +
> > +	ret = devm_snd_soc_register_component(dev, &cv1800b_i2s_component, dai,
> > +					      1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_snd_dmaengine_pcm_register(dev, &cv1800b_i2s_pcm_config, 0);
> > +	if (ret) {
> > +		dev_err(dev, "dmaengine_pcm_register failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(dev, "cv1800b I2S probed:\n");
> 
> Drop, drivers should be silent and probe success message does not
> warrant even debug. What are you debugging - that you wrote correct DTS?
> You can check in sysfs that device probed.

Will do, thanks. 

> 
> Best regards,
> Krzysztof
>