Re: [PATCHv2 1/8] ALSA: Add SAI SoC Digital Audio Interface driver.

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2209 bytes --]

On Mon, Nov 04, 2013 at 07:35:12AM +0000, Li Xiubo wrote:

> From the ASoC subsystem comments we can see that:
> ++++++
> Configures the clock dividers. This is used to derive the best DAI bit and
> frame clocks from the system or master clock. It's best to set the DAI bit
> and frame clocks as low as possible to save system power.
> ------

You should never use this unless you have to, there is no point in every
single machine driver using your driver having to duplicate the same
calculations.

> 
> > > +static int fsl_sai_dai_probe(struct snd_soc_dai *dai) {
> > > +	int ret;
> > > +	struct fsl_sai *sai = dev_get_drvdata(dai->dev);
> > > +
> > > +	ret = clk_prepare_enable(sai->clk);
> > > +	if (ret)
> > > +		return ret;

> > It'd be nicer to only enable the clock while the device is in active use.

> While if the module clock is not enabled here, the followed registers cannot read/write in the same function.
> And this _probe function is the _dai_probe not the driver's module _probe.

So you can enable the clock when you explicitly need to write to the
registers...

> If the clk_prepare_enable(sai->clk) is not here, where should it be will be nicer ?
> One of the following functions ?
>         .set_sysclk     = fsl_sai_set_dai_sysclk,
>         .set_clkdiv     = fsl_sai_set_dai_clkdiv,
>         .set_fmt        = fsl_sai_set_dai_fmt,
>         .set_tdm_slot   = fsl_sai_set_dai_tdm_slot,
>         .hw_params      = fsl_sai_hw_params,
>         .trigger        = fsl_sai_trigger,

It could be in any or all of them except trigger (where the core should
hold a runtime PM reference anyway).  You can always take a reference
for the duration of the function if you're concerned it may be called
when the referent isn't otherwise held.

> > > +	ret = snd_dmaengine_pcm_register(&pdev->dev, NULL,
> > > +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);
> > > +	if (ret)
> > > +		return ret;

> > We should have a devm_ version of this.

> Sorry, is there one patch for adding the devm_ version of snd_dmaengine_pcm_register() already ?
> In the -next and other topics branches I could not find it.

No, there isn't one but there should be one.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel