Re: [PATCH v2 3/4] ASoC: starfive: Add JH7110 TDM driver

[Walker Chen]

On 2023/4/20 22:30, Mark Brown wrote:
> On Thu, Apr 20, 2023 at 10:41:17AM +0800, Walker Chen wrote:
>> Add tdm driver support for the StarFive JH7110 SoC.
> 
> This is mostly fine, though the code all feels a bit messy somehow.
> A lot of this is just coding style, I've highlighted a bunch of things
> below.  There's also a couple of more substantial issues.

Hey Mark,
Firstly thanks for your patient review and detailed comments.

> 
>> @@ -0,0 +1,579 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TDM driver for the StarFive JH7110 SoC
>> + *
>> + * Copyright (C) 2023 StarFive Technology Co., Ltd.
> 
> Please make the entire comment a C++ one so things look more
> intentional.

OK, can reference to other platform's format.

> 
>> +static int jh7110_tdm_syncdiv(struct jh7110_tdm_dev *tdm)
>> +{
>> +	u32 sl, sscale, syncdiv;
>> +
>> +	sl = (tdm->rx.sl >= tdm->tx.sl) ? tdm->rx.sl : tdm->tx.sl;
>> +	sscale = (tdm->rx.sscale >= tdm->tx.sscale) ? tdm->rx.sscale : tdm->tx.sscale;
> 
> Please write normal conditional statements to improve legibility.

Will be modified.

> 
>> +static int jh7110_tdm_clk_enable(struct jh7110_tdm_dev *tdm)
>> +{
> 
>> +       ret = clk_set_parent(tdm->clk_tdm, tdm->clk_tdm_ext);
>> +       if (ret) {
>> +               dev_err(tdm->dev, "Can't set clock source for clk_tdm: %d\n",
>> +ret);
>> +               goto dis_tdm_clk;
>> +       }
> 
> It's a bit weird to enable clocks and then reparent afterwards, that
> seems more likely to run into issues with glitches doing something bad
> than reparenting with the clock disabled.

This TDM module ultimately uses an external clock. It firstly must uses internal clock 
before being enabled, and then is switched to external clock, otherwise failed to reset.
This limitation is determined by the chip.

> 
> This parenting looks like a system specific configuration (what if
> the SoC is driving the audio bus?), and might be better done by using
> the clock bindings.  It's also strange that the driver is reparenting
> every single time it enables the clocks rather than doing that once on
> init.

To save power consumption, need to disable clock in suspend() and enable clock in
resume(). As mentioned above, the internal clock must be selected before enabling 
clock every time, and then switch to external clock.

> 
>> +static int jh7110_tdm_suspend(struct snd_soc_component *component)
>> +{
>> +	return pm_runtime_force_suspend(component->dev);
>> +}
>> +
>> +static int jh7110_tdm_resume(struct snd_soc_component *component)
>> +{
>> +	struct jh7110_tdm_dev *tdm = snd_soc_component_get_drvdata(component);
>> +
>> +	/* restore context */
>> +	jh7110_tdm_writel(tdm, TDM_PCMGBCR, tdm->saved_pcmgbcr);
>> +	jh7110_tdm_writel(tdm, TDM_PCMDIV, tdm->saved_pcmdiv);
>> +
>> +	return pm_runtime_force_resume(component->dev);
>> +}
> 
> It is weird that we restore context that we don't save on suspend, the
> code *works* but it looks off.

Should be pairing operation in suspend() and resume().

> 
>> +static int jh7110_tdm_hw_params(struct snd_pcm_substream *substream,
>> +				struct snd_pcm_hw_params *params,
>> +				struct snd_soc_dai *dai)
>> +{
>> +	struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
>> +	int chan_wl, chan_sl, chan_nr;
>> +	unsigned int data_width;
>> +	unsigned int dma_bus_width;
>> +	struct snd_dmaengine_dai_dma_data *dma_data = NULL;
>> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
>> +	struct snd_soc_dai_link *dai_link = rtd->dai_link;
>> +
>> +	dai_link->stop_dma_first = 1;
> 
> A driver shouldn't be forcing dai_link settings, and hw_params is
> claerly the wrong place to be configuring something like this which
> never varies at runtime - it should be done on init().  If the DAI
> really needs this you should extend the core so there's a flag in the
> dai_driver which gets checked.

Yes, should be done at startup of dai_driver, doing that once on initialize stage.

> 
>> +	switch (chan_nr) {
>> +	case ONE_CHANNEL_SUPPORT:
>> +	case TWO_CHANNEL_SUPPORT:
>> +	case FOUR_CHANNEL_SUPPORT:
>> +	case SIX_CHANNEL_SUPPORT:
>> +	case EIGHT_CHANNEL_SUPPORT:
> 
> I am having a *really* hard time finding these definitions (which aren't
> namespaced...) helpful.  Just write the numbers directly.

OK, will be changed.

> 
>> +static int jh7110_tdm_trigger(struct snd_pcm_substream *substream,
>> +			      int cmd, struct snd_soc_dai *dai)
>> +{
>> +	struct jh7110_tdm_dev *tdm = snd_soc_dai_get_drvdata(dai);
>> +	int ret = 0;
>> +
>> +	switch (cmd) {
>> +	case SNDRV_PCM_TRIGGER_START:
>> +	case SNDRV_PCM_TRIGGER_RESUME:
>> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>> +		/* restore context */
>> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>> +			jh7110_tdm_writel(tdm, TDM_PCMTXCR, tdm->saved_pcmtxcr);
>> +		else
>> +			jh7110_tdm_writel(tdm, TDM_PCMRXCR, tdm->saved_pcmrxcr);
>> +
>> +		jh7110_tdm_start(tdm, substream);
> 
> Why is the write to CR not part of start()?

OK, will be changed.

> 
>> +static void jh7110_tdm_init_params(struct jh7110_tdm_dev *tdm)
>> +{
>> +	tdm->clkpolity = TDM_TX_RASING_RX_FALLING;
>> +	if (tdm->frame_mode == SHORT_LATER) {
>> +		tdm->elm = TDM_ELM_LATE;
>> +		tdm->syncm = TDM_SYNCM_SHORT;
>> +	} else if (tdm->frame_mode == SHORT_EARLY) {
>> +		tdm->elm = TDM_ELM_EARLY;
>> +		tdm->syncm = TDM_SYNCM_SHORT;
>> +	} else {
>> +		tdm->elm = TDM_ELM_EARLY;
>> +		tdm->syncm = TDM_SYNCM_LONG;
>> +	}
> 
> This looks like it should be a switch statement, and the defintiions
> namespaced.  I can't see anyhwere where this ever gets configured to
> anything other than SHORT_LATER ever being used so might be better to
> just delete.

Will be modified according to your suggestion.

> 
>> +	tdm->ms_mode = TDM_AS_SLAVE;
> 
> Please use the modern provider/consumer terminology for clocking.
> 
>> +	tdm->clk_tdm_ahb = clks[0].clk;
>> +	tdm->clk_tdm_apb = clks[1].clk;
>> +	tdm->clk_tdm_internal = clks[2].clk;
>> +	tdm->clk_tdm = clks[3].clk;
>> +	tdm->clk_mclk_inner = clks[4].clk;
>> +	tdm->clk_tdm_ext = clks[5].clk;
> 
> Given that the driver only ever interacts with the clocks en masse is
> there any point in having all the specific named variables, that'd mean
> that the enable/disable could just use loops.

Will be changed.

> 
>> +/*  DMA registers */
>> +#define TDM_FIFO			0x170c0000
>> +#define TDM_FIFO_DEPTH			32
> 
> None of the defines in the header are namespaced and some of them (like
> the above) seem generic enough to be likely to result in conflicts.

Will add unified JH7110_TDM_ prefix.

Thank you very much for your suggestion.

Best regards,
Walker