LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Nicolas Frattaroli <frattaroli.nicolas@gmail.com>
To: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>, Heiko Stuebner <heiko@sntech.de>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: linux-rockchip@lists.infradead.org, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller
Date: Sat, 21 Aug 2021 22:45:52 +0200	[thread overview]
Message-ID: <3469189.PC3msRC2N5@archbook> (raw)
In-Reply-To: <66d6bd43-ee43-eff4-7a68-333fbb996787@linux.intel.com>

On Freitag, 20. August 2021 21:02:16 CEST Pierre-Louis Bossart wrote:
> > +	regmap_read(i2s_tdm->regmap, I2S_CLR, &val);
> > +	/* Wait on the clear operation to finish */
> > +	while (val) {
> 
> delay needed here?
> 

The rockchip_i2s.c code doesn't have a delay here either, but I can
add one of 1 usec for good measure, it seems weird to retry the
read as fast as it can.

> > +static int rockchip_i2s_tdm_clk_set_rate(struct rk_i2s_tdm_dev *i2s_tdm,
> > +					 struct clk *clk, unsigned long rate,
> > +					 int ppm)
> > +{
> > +	unsigned long rate_target;
> > +	int delta, ret;
> > +
> > +	if (ppm == i2s_tdm->clk_ppm)
> > +		return 0;
> > +
> > +	delta = (ppm < 0) ? -1 : 1;
> > +	delta *= (int)div64_u64((u64)rate * (u64)abs(ppm) + 500000,
> > +				1000000);
> 
> formula looks odd? looks like you are implementing a round to nearest
> operation, but that shouldn't require this multiplication?
> 

I believe the multiplication is there to compensate for clock drift.
ppm is a value between -1000 and 1000 that specifies the clock drift
in presumably parts per million, going by the variable name.

> > +	pm_runtime_enable(&pdev->dev);
> > +	if (!pm_runtime_enabled(&pdev->dev)) {
> > +		ret = i2s_tdm_runtime_resume(&pdev->dev);
> 
> that looks like dead code? you've just enabled pm_runtime, why would
> this fail?
> 

I've had a look at the upstream rockchip_i2s.c code which does the
same thing, and I believe the idea here is that we need to manually
prepare and enable the master clocks (mclk_rx/mclk_tx) if pm_runtime
is not available. Otherwise, pm_runtime will presumably call our
resume callback at some point.

If runtime power management is disabled in the kernel config then 
pm_runtime_enabled is always going to return false.

> > +err_suspend:
> > +	if (!pm_runtime_status_suspended(&pdev->dev))
> > +		i2s_tdm_runtime_suspend(&pdev->dev);
> 
> why is this necessary?

I believe this is the same kind of situation as before, and the
other driver does this too: if pm_runtime is not available, we
need to stop our clocks manually on probe failure.
 
> > +err_pm_disable:
> > +	pm_runtime_disable(&pdev->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rockchip_i2s_tdm_remove(struct platform_device *pdev)
> > +{
> > +	struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(&pdev->dev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +	if (!pm_runtime_status_suspended(&pdev->dev))
> > +		i2s_tdm_runtime_suspend(&pdev->dev);
> 
> this looks backwards, if you disable pm_runtime first what is the
> expectation for the rest.

I'm not well versed in the PM code but if my theory of this being
related to unavailable PM is correct, then my best guess is that
pm_runtime_disable does suspend the device, so if it's not
suspended then we don't have pm_runtime and therefore need to call
it manually.

> > +
> > +	if (!IS_ERR(i2s_tdm->mclk_tx))
> > +		clk_prepare_enable(i2s_tdm->mclk_tx);
> > +	if (!IS_ERR(i2s_tdm->mclk_rx))
> > +		clk_prepare_enable(i2s_tdm->mclk_rx);
> > +	if (!IS_ERR(i2s_tdm->hclk))
> > +		clk_disable_unprepare(i2s_tdm->hclk);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> 
> use __maybe_unused

You mean instead of the ifdef stuff to just add this attribute to
the following functions like this?

static int rockchip_i2s_tdm_suspend(struct device *dev) __maybe_unused

> 
> > +static int rockchip_i2s_tdm_suspend(struct device *dev)
> > +{
> > +	struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev);
> > +
> > +	regcache_mark_dirty(i2s_tdm->regmap);
> > +
> > +	return 0;
> > +}
> > +
> > +static int rockchip_i2s_tdm_resume(struct device *dev)
> > +{
> > +	struct rk_i2s_tdm_dev *i2s_tdm = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = pm_runtime_get_sync(dev);
> > +	if (ret < 0)
> > +		return ret;
> > +	ret = regcache_sync(i2s_tdm->regmap);
> > +	pm_runtime_put(dev);
> > +
> > +	return ret;
> > +}
> > +#endif

Thank you for your review!

Regards,
Nicolas Frattaroli




  reply	other threads:[~2021-08-21 20:46 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 18:27 [PATCH v2 0/4] Rockchip I2S/TDM controller Nicolas Frattaroli
2021-08-20 18:27 ` [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller Nicolas Frattaroli
2021-08-20 19:02   ` Pierre-Louis Bossart
2021-08-21 20:45     ` Nicolas Frattaroli [this message]
2021-08-23 11:19       ` Mark Brown
2021-08-23 15:02       ` Pierre-Louis Bossart
2021-08-23 12:03   ` Philipp Zabel
2021-08-23 14:35     ` Nicolas Frattaroli
2021-08-23 16:03       ` Philipp Zabel
2021-08-20 18:27 ` [PATCH v2 2/4] dt-bindings: sound: add rockchip i2s-tdm binding Nicolas Frattaroli
2021-08-24 16:42   ` Rob Herring
2021-08-20 18:27 ` [PATCH v2 3/4] arm64: dts: rockchip: add i2s1 on rk356x Nicolas Frattaroli
2021-08-20 18:27 ` [PATCH v2 4/4] arm64: dts: rockchip: add analog audio on Quartz64 Nicolas Frattaroli

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3469189.PC3msRC2N5@archbook \
    --to=frattaroli.nicolas@gmail.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=heiko@sntech.de \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=perex@perex.cz \
    --cc=pierre-louis.bossart@linux.intel.com \
    --cc=tiwai@suse.com \
    --subject='Re: [PATCH v2 1/4] ASoC: rockchip: add support for i2s-tdm controller' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).