Re: [PATCH 2/2] ASoC: spacemit: i2s: add support for K1 SoC

[Troy Mitchell <troy.mitchell@xxxxxxxxxxxxxxxxxx>]

On Thu, Aug 14, 2025 at 12:36:45PM +0200, Philipp Zabel wrote:
> On Do, 2025-08-14 at 16:54 +0800, Troy Mitchell wrote:
> > Add ASoC platform driver for the SpacemiT K1 SoC full-duplex I2S
> > controller.
> > 
> > Co-developer: Jinmei Wei <weijinmei@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Jinmei Wei <weijinmei@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Troy Mitchell <troy.mitchell@xxxxxxxxxxxxxxxxxx>
> > ---
> >  sound/soc/Kconfig           |   1 +
> >  sound/soc/Makefile          |   1 +
> >  sound/soc/spacemit/Kconfig  |  14 ++
> >  sound/soc/spacemit/Makefile |   5 +
> >  sound/soc/spacemit/k1_i2s.c | 444 ++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 465 insertions(+)
> > 
> [...]
> > diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..9e41525afbf3f08ab9a26b562772861d86f39cd7
> > --- /dev/null
> > +++ b/sound/soc/spacemit/k1_i2s.c
> > @@ -0,0 +1,444 @@
> [...]
> 
> > +static int spacemit_i2s_probe(struct platform_device *pdev)
> > +{
> > +	struct snd_soc_dai_driver *dai;
> > +	struct spacemit_i2s_dev *i2s;
> > +	struct resource *res;
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
> > +	if (!i2s)
> > +		return -ENOMEM;
> > +
> > +	i2s->dev = &pdev->dev;
> > +
> > +	i2s->sysclk = devm_clk_get_enabled(i2s->dev, "sysclk");
> > +	if (IS_ERR(i2s->sysclk))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(i2s->sysclk),
> > +				     "failed to enable sysbase clock\n");
> > +
> > +	i2s->bclk = devm_clk_get_enabled(i2s->dev, "bclk");
> > +	if (IS_ERR(i2s->bclk))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(i2s->bclk), "failed to enable bit clock\n");
> > +
> > +	clk = devm_clk_get_enabled(i2s->dev, "sspa_bus");
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(clk), "failed to enable sspa_bus clock\n");
> > +
> > +	i2s->sspa_clk = devm_clk_get_enabled(i2s->dev, "sspa");
> > +	if (IS_ERR(clk))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(clk), "failed to enable sspa clock\n");
> > +
> > +	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> > +	if (IS_ERR(i2s->base))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(i2s->base), "failed to map registers\n");
> > +
> > +	i2s->reset =  devm_reset_control_get(&pdev->dev, NULL);
> 
> Please use devm_reset_control_get_exclusive() directly.
Tnx. I will use in the next version.

> 
> > +	if (IS_ERR(i2s->reset))
> > +		return dev_err_probe(i2s->dev, PTR_ERR(i2s->reset),
> > +				     "failed to get reset control");
> > +
> > +	dev_set_drvdata(i2s->dev, i2s);
> > +
> > +	spacemit_i2s_init_dai(i2s, &dai, res->start + SSDATR);
> > +
> > +	ret = devm_snd_soc_register_component(i2s->dev,
> > +					      &spacemit_i2s_component,
> > +					      dai, 1);
> > +	if (ret)
> > +		return dev_err_probe(i2s->dev, ret, "failed to register component");
> > +
> > +	return devm_snd_dmaengine_pcm_register(&pdev->dev, &spacemit_dmaengine_pcm_config, 0);
> > +}
> > +
> > +static void spacemit_i2s_remove(struct platform_device *pdev)
> > +{
> > +	struct spacemit_i2s_dev *i2s = dev_get_drvdata(&pdev->dev);
> > +
> > +	reset_control_assert(i2s->reset);
> 
> I'd move the reset_control_assert() be moved to snd_soc_dai_ops::remove
> for symmetry.
>
It makes sense.

deassert in ops::probe and assert in ops::remove.

                - Troy
> 
> regards
> Philipp
>