Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Nicolin Chen <Guangyu.Chen@freescale.com>
To: Markus Pargmann <mpa@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>,
	alsa-devel@alsa-project.org, Alexander Shiyan <shc_work@mail.ru>,
	Mark Brown <broonie@kernel.org>, Timur Tabi <timur@tabi.org>,
	"Li.Xiubo@freescale.com" <Li.Xiubo@freescale.com>,
	kernel@pengutronix.de, Sascha Hauer <s.hauer@pengutronix.de>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 09/18] ASoC: fsl-ssi: Only enable baudclk when used
Date: Wed, 16 Apr 2014 16:40:46 +0800
Message-ID: <20140416084045.GB13449@MrMyself> (raw)
In-Reply-To: <20140416084331.GD6159@pengutronix.de>

On Wed, Apr 16, 2014 at 10:43:31AM +0200, Markus Pargmann wrote:
> Hi Nicolin,
> 
> On Wed, Apr 16, 2014 at 04:08:29PM +0800, Nicolin Chen wrote:
> > Hi Markus,
> > 
> > Please allow me to drop some content.
> > 
> > On Wed, Apr 16, 2014 at 09:27:38AM +0200, Markus Pargmann wrote:
> > > On Mon, Apr 14, 2014 at 11:28:51PM +0800, Nicolin Chen wrote:
> > > > On Mon, Apr 14, 2014 at 03:35:39PM +0200, Markus Pargmann wrote:
> > > > > +static bool fsl_ssi_is_i2s_master(struct fsl_ssi_private *ssi_private)
> > > > > +{
> > > > > +	return (ssi_private->dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) ==
> > > > > +		SND_SOC_DAIFMT_CBS_CFS;
> > > > > +}
> > 
> > > > > @@ -496,6 +503,12 @@ static int fsl_ssi_startup(struct snd_pcm_substream *substream,
> > > > >  		spin_unlock_irqrestore(&ssi_private->baudclk_lock, flags);
> > > > >  	}
> > > > >  
> > > > > +	if (fsl_ssi_is_i2s_master(ssi_private)) {
> > > > > +		ret = clk_prepare_enable(ssi_private->baudclk);
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +	}
> > 
> > > > > +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
> > > > > +		struct snd_soc_dai *dai)
> > > > > +{
> > > > > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > > > > +	struct fsl_ssi_private *ssi_private =
> > > > > +		snd_soc_dai_get_drvdata(rtd->cpu_dai);
> > > > > +
> > > > > +	if (fsl_ssi_is_i2s_master(ssi_private))
> > > > > +		clk_disable_unprepare(ssi_private->baudclk);
> > > > > +}
> > 
> > > > > @@ -576,6 +600,11 @@ static int fsl_ssi_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt)
> > > > >  
> > > > >  	ssi_private->dai_fmt = fmt;
> > > > >  
> > > > > +	if (fsl_ssi_is_i2s_master(ssi_private) && IS_ERR(ssi_private->baudclk)) {
> > > > > +		dev_err(cpu_dai->dev, "no baudclk needed for master mode\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > 
> > > > I was wondering what if machine driver doesn't set fmt to master during
> > > > its probe(), in another word -- before fsl_ssi_startup(), but leave that
> > > > into its hw_params() via snd_soc_dai_set_fmt() which then would run after
> > > > fsl_ssi_startup() while having no baud clock enabled in this case.
> > > > 
> > > > A better solution may be to wrap clk_prepare_enable() and master mode
> > > > clock dividing code into fsl_ssi_hw_params(), a bit like the ESAI driver
> > > > even though it doesn't contain the clk_prepare_enable() part currently,
> > > > and then to put clk_disable_unprepare() into hw_free() for symmetry.
> > > > 
> > > > Any suggestion?
> > > 
> > > Yes this is a problem. Although I am not really convinced of the concept
> > > of setting up the DAIs in hw_params, we should support it as there are
> > > some users.
> > 
> > I think it might be an old fashion way. I saw quite a lot machine drivers
> > doing the DAI format setting in its hw_params(). Even in the current tree
> > we can also grep some out by using:
> > 
> > find ./sound/soc/ -name "*.c" |xargs grep "snd_soc_dai_set_fmt"
> > 
> > So it should be plausible considering there might be some special cases,
> > using CBS_CFS for 44.1KHz groups and CBM_CFM for 48KHz groups for example.
> > 
> > > Is there always exactly one hw_free call for one hw_params call? There
> > > is a comment above the function:
> > > "Frees resources allocated by hw_params, can be called multiple times",
> > 
> > That's a good question. IIRC, snd_pcm_release_substream() would call its
> > hw_free() right before calling snd_pcm_close(). So there would be at least
> > twice for hw_free()'s execution.
> > 
> > > so I am not sure if we can directly use clk_prepare_enable or if we need
> > > to remember the clock state?
> > 
> > We need to if applying that. Or put them into trigger() as an alternative
> > approach even if it sounds weird to me.
> 
> Unfortunately trigger() is not an option, it also may be called multiple
> times for one command due to strange error handling in the layers above.
> For example a failing START command in the DMA driver may lead to a
> STOP command in the fsl-ssi driver without a previous START command.

Nice judgement. Okay, just forget about trigger() then.

> So it seems the only option is to save the clock state? I will move it
> to hw_params() and hw_free().

I think we might wait other's opinion on this topic or try to send an extra
patch for it meanwhile you can just drop the single patch from this series
so that the other patches will be easily applied since they are really useful
and do fix a few problems.

Cheers,
Nicolin

> 
> Thanks,
> 
> Markus
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

  reply index

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-14 13:35 [PATCH v3 00/18] ASoC: fsl-ssi: Driver cleanup Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 01/18] ASoC: fsl-ssi: Fix register values when disabling Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 02/18] ASoC: fsl-ssi: Move debugging to seperate file Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 03/18] ASoC: fsl-ssi: Use dev_name for DAI driver struct Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 04/18] ASoC: fsl-ssi: Move imx-specific probe to seperate function Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 05/18] ASoC: fsl-ssi: Remove useless DMA code Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 06/18] ASoC: fsl-ssi: Cleanup probe function Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 07/18] ASoC: fsl-ssi: Remove unnecessary variables from ssi_private Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 08/18] ASoC: fsl-ssi: introduce SoC specific data Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 09/18] ASoC: fsl-ssi: Only enable baudclk when used Markus Pargmann
2014-04-14 15:28   ` Nicolin Chen
2014-04-16  7:27     ` Markus Pargmann
2014-04-16  8:08       ` Nicolin Chen
2014-04-16  8:43         ` Markus Pargmann
2014-04-16  8:40           ` Nicolin Chen [this message]
2014-04-16 17:42         ` Mark Brown
2014-04-17 13:46       ` Timur Tabi
2014-04-14 13:35 ` [PATCH v3 10/18] ASoC: fsl-ssi: make fsl,mode property optional Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 11/18] ASoC: fsl-ssi: Transmit enable synchronization Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 12/18] ASoC: fsl-ssi: Move fsl_ssi_set_dai_sysclk above fsl_ssi_hw_params Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 13/18] ASoC: fsl-ssi: set bitclock in master mode from hw_params Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 14/18] ASoC: fsl-ssi: remove unnecessary spinlock Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 15/18] ASoC: fsl-ssi: Allow first stream to set the bitclock Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 16/18] ASoC: fsl-ssi: Set framerate divider correctly for i2s master mode Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 17/18] ASoC: fsl-ssi: reorder and document fsl_ssi_private Markus Pargmann
2014-04-14 13:35 ` [PATCH v3 18/18] ASoC: fsl-ssi: Use regmap Markus Pargmann
2014-04-24 11:44 ` [PATCH v3 00/18] ASoC: fsl-ssi: Driver cleanup Mark Brown
2014-04-28  8:54   ` Markus Pargmann
2014-04-29 16:22     ` Mark Brown
2014-04-30  2:01       ` Li.Xiubo

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=20140416084045.GB13449@MrMyself \
    --to=guangyu.chen@freescale.com \
    --cc=Li.Xiubo@freescale.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=fabio.estevam@freescale.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=mpa@pengutronix.de \
    --cc=s.hauer@pengutronix.de \
    --cc=shc_work@mail.ru \
    --cc=timur@tabi.org \
    /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

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git